From 310940bdcfa4f53c83711184bfe1c29fc1dc8833 Mon Sep 17 00:00:00 2001 From: Simon McVittie 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