Attachment 42381 Details for Bug 33342 – DBusLoop: move OOM watches to a secondary list instead of flagging them

[patch] DBusLoop: move OOM watches to a secondary list instead of flagging them

DBusLoop-move-OOM-watches-to-a-secondary-list-inst.patch (text/plain), 7.03 KB, created by Simon McVittie on 2011-01-24 07:51:30 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2011-01-24 07:51:30 UTC

Size: 7.03 KB

patch

obsolete

>From 310940bdcfa4f53c83711184bfe1c29fc1dc8833 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Mon, 24 Jan 2011 14:48:58 +0000
>Subject: [PATCH] DBusLoop: move OOM watches to a secondary list instead of flagging them
>
>This will eventually let us maintain a DBusPollFD[] of just the active
>watches, between several iterations. The more immediate benefit is that
>WatchCallback can go away, because it only contains a refcount, a
>now-useless type, and a watch that already has its own refcount.
>
>Bug: https://bugs.freedesktop.org/show_bug.cgi?id=33342
>---
> dbus/dbus-mainloop.c |  110 +++++++++++++++++++++++++++++++++-----------------
> 1 files changed, 73 insertions(+), 37 deletions(-)
>
>diff --git a/dbus/dbus-mainloop.c b/dbus/dbus-mainloop.c
>index e96010d..df2ced5 100644
>--- a/dbus/dbus-mainloop.c
>+++ b/dbus/dbus-mainloop.c
>@@ -56,10 +56,11 @@ watch_flags_to_string (int flags)
> struct DBusLoop
> {
>   int refcount;
>-  DBusList *watches;
>+  DBusList *watches; /**< watches that were not OOM on the last attempt */
>+  DBusList *oom_watches; /**< watches that were OOM on the last attempt */
>   DBusList *timeouts;
>   int callback_list_serial;
>-  int watch_count;
>+  int watch_count; /**< combined length of watches and oom_watches */
>   int timeout_count;
>   int depth; /**< number of recursive runs */
>   DBusList *need_dispatch;
>@@ -111,8 +112,6 @@ typedef struct
> {
>   Callback callback;
>   DBusWatch *watch;
>-  /* last watch handle failed due to OOM */
>-  unsigned int last_iteration_oom : 1;
> } WatchCallback;
> 
> typedef struct
>@@ -136,7 +135,6 @@ watch_callback_new (DBusWatch         *watch)
>     return NULL;
> 
>   cb->watch = watch;
>-  cb->last_iteration_oom = FALSE;
>   cb->callback.refcount = 1;
>   cb->callback.type = CALLBACK_WATCH;
>   
>@@ -322,6 +320,26 @@ _dbus_loop_remove_watch (DBusLoop         *loop,
>       link = next;
>     }
> 
>+  /* Or maybe it was OOM last time? We do this last, because it should be
>+   * unlikely */
>+  link = _dbus_list_get_first_link (&loop->oom_watches);
>+  while (link != NULL)
>+    {
>+      DBusList *next = _dbus_list_get_next_link (&loop->oom_watches, link);
>+      WatchCallback *this = link->data;
>+
>+      _dbus_assert (this->callback.type == CALLBACK_WATCH);
>+
>+      if (this->watch == watch)
>+        {
>+          remove_callback (loop, &loop->oom_watches, link);
>+
>+          return;
>+        }
>+
>+      link = next;
>+    }
>+
>   _dbus_warn ("could not find watch %p to remove\n", watch);
> }
> 
>@@ -535,7 +553,7 @@ _dbus_loop_iterate (DBusLoop     *loop,
>   int n_ready;
>   int initial_serial;
>   long timeout;
>-  dbus_bool_t oom_watch_pending;
>+  DBusList *oom_last_time;
>   int orig_depth;
>   
>   retval = FALSE;      
>@@ -543,15 +561,17 @@ _dbus_loop_iterate (DBusLoop     *loop,
>   fds = NULL;
>   watches_for_fds = NULL;
>   n_fds = 0;
>-  oom_watch_pending = FALSE;
>+  oom_last_time = NULL;
>+
>   orig_depth = loop->depth;
>   
> #if MAINLOOP_SPEW
>   _dbus_verbose ("Iteration block=%d depth=%d timeout_count=%d watch_count=%d\n",
>                  block, loop->depth, loop->timeout_count, loop->watch_count);
> #endif
>-  
>-  if (loop->watches == NULL && loop->timeouts == NULL)
>+
>+  if (loop->watches == NULL && loop->oom_watches == NULL &&
>+      loop->timeouts == NULL)
>     goto next_iteration;
> 
>   if (loop->watch_count > N_STACK_DESCRIPTORS)
>@@ -577,6 +597,21 @@ _dbus_loop_iterate (DBusLoop     *loop,
>       watches_for_fds = stack_watches_for_fds;
>     }
> 
>+  if (loop->oom_watches != NULL)
>+    {
>+#if MAINLOOP_SPEW
>+      _dbus_verbose ("Skipping at least one watch: OOM last time");
>+#endif
>+      /* If some watches were OOM last time, we'll skip them this time,
>+       * but re-enable them for next time. Also, we have a timeout this time.
>+       * To do this, we steal the list of OOM watches into a local variable. */
>+      oom_last_time = loop->oom_watches;
>+      loop->oom_watches = NULL;
>+      /* Return TRUE here to keep the loop going, since we don't know the
>+       * watch is inactive */
>+      retval = TRUE;
>+    }
>+
>   /* fill our array of fds and watches */
>   n_fds = 0;
>   link = _dbus_list_get_first_link (&loop->watches);
>@@ -593,24 +628,7 @@ _dbus_loop_iterate (DBusLoop     *loop,
>       wcb = WATCH_CALLBACK (cb);
>       fd = dbus_watch_get_socket (wcb->watch);
> 
>-      if (wcb->last_iteration_oom)
>-        {
>-          /* we skip this one this time, but reenable it next time,
>-           * and have a timeout on this iteration
>-           */
>-          wcb->last_iteration_oom = FALSE;
>-          oom_watch_pending = TRUE;
>-
>-          retval = TRUE; /* return TRUE here to keep the loop going,
>-                          * since we don't know the watch is inactive
>-                          */
>-
>-#if MAINLOOP_SPEW
>-          _dbus_verbose ("  skipping watch on fd %d as it was out of memory last time\n",
>-                         fd);
>-#endif
>-        }
>-      else if (_DBUS_UNLIKELY (fd == -1))
>+      if (_DBUS_UNLIKELY (fd == -1))
>         {
>           _dbus_warn ("watch %p was invalidated but not removed; "
>               "removing it now\n", wcb->watch);
>@@ -705,10 +723,10 @@ _dbus_loop_iterate (DBusLoop     *loop,
> #endif
>     }
> 
>-  /* if a watch is OOM, don't wait longer than the OOM
>+  /* if a watch was OOM last time, don't wait longer than the OOM
>    * wait to re-enable it
>    */
>-  if (oom_watch_pending)
>+  if (oom_last_time != NULL)
>     timeout = MIN (timeout, _dbus_get_oom_wait ());
> 
> #if MAINLOOP_SPEW
>@@ -812,16 +830,23 @@ _dbus_loop_iterate (DBusLoop     *loop,
>               if (condition != 0 &&
>                   dbus_watch_get_enabled (wcb->watch))
>                 {
>-                  dbus_bool_t oom;
>-
>-                  if (!dbus_watch_handle (wcb->watch, condition))
>-                    wcb->last_iteration_oom = TRUE;
>-
>+                  if (dbus_watch_handle (wcb->watch, condition))
>+                    {
> #if MAINLOOP_SPEW
>-                  _dbus_verbose ("  Invoked watch, oom = %d\n",
>-                                 wcb->last_iteration_oom);
>+                      _dbus_verbose ("  Invoked watch, had enough memory\n");
> #endif
>-                  
>+                    }
>+                  else
>+                    {
>+#if MAINLOOP_SPEW
>+                      _dbus_verbose ("  OOM, will skip watch next time\n");
>+#endif
>+                      link = _dbus_list_find_last (&loop->watches, wcb);
>+                      _dbus_assert (link != NULL);
>+                      _dbus_list_unlink (&loop->watches, link);
>+                      _dbus_list_append_link (&loop->oom_watches, link);
>+                    }
>+
>                   retval = TRUE;
>                 }
> 
>@@ -848,6 +873,17 @@ _dbus_loop_iterate (DBusLoop     *loop,
>   
>   if (fds && fds != stack_fds)
>     dbus_free (fds);
>+
>+  /* All the watches that were OOM last time can go back into the list
>+   * for another try. The watches that were OOM *this* time are the new
>+   * value of loop->oom_watches. */
>+  while (oom_last_time != NULL)
>+    {
>+      link = oom_last_time;
>+      _dbus_list_unlink (&oom_last_time, link);
>+      _dbus_list_append_link (&loop->watches, link);
>+    }
>+
>   if (watches_for_fds)
>     {
>       i = 0;
>-- 
>1.7.2.3

Actions: View | Splinter Review

Attachments on bug 33342: 42374 | 42375 | 42376 | 42377 | 42378 | 42379 | 42380 | 42381 | 42382 | 42383 | 42384 | 42450 | 42451 | 42452 | 42453

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.