From a8f75f21b4b2264b385022496c597573ecb709da Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 13 Sep 2011 17:37:33 +0100 Subject: [PATCH] GDBusWorker: combine num_writes_pending with flush_pending num_writes_pending was a counter, but it only took values 0 or 1, so make it a boolean: it would never make sense to be trying to write out two messages at the same time (they'd get interleaved). Similarly, we can never be writing and flushing at the same time (that'd mean we were flushing halfway through a message, which would be pointless) so combine it with flush_pending too, calling the result output_pending. Also assert that it takes the expected value whenever we change it, and document the locking discipline used for it, including a subtle case in write_message_in_idle_cb where it's not obvious at first glance why we don't need the lock. (Having the combined boolean at the top of the block of write-related struct members improves struct packing on 64-bit platforms, by packing read_num_ancillary_messages and output_pending into one word.) Bug: https://bugzilla.gnome.org/show_bug.cgi?id=651268 Bug-NB: NB#271520 Signed-off-by: Simon McVittie Signed-off-by: David Zeuthen --- gio/gdbusprivate.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c index aa2a16e..86af72d 100644 --- a/gio/gdbusprivate.c +++ b/gio/gdbusprivate.c @@ -370,13 +370,17 @@ struct GDBusWorker GSocketControlMessage **read_ancillary_messages; gint read_num_ancillary_messages; + /* TRUE if an async write or flush is pending. + * Only the worker thread may change its value, and only with the write_lock. + * Other threads may read its value when holding the write_lock. + * The worker thread may read its value at any time. + */ + gboolean output_pending; /* used for writing */ - gint num_writes_pending; GMutex *write_lock; GQueue *write_queue; guint64 write_num_messages_written; GList *write_pending_flushes; - gboolean flush_pending; }; /* ---------------------------------------------------------------------------------------------------- */ @@ -1107,7 +1111,8 @@ ostream_flush_cb (GObject *source_object, /* Make sure we tell folks that we don't have additional flushes pending */ g_mutex_lock (data->worker->write_lock); - data->worker->flush_pending = FALSE; + g_assert (data->worker->output_pending); + data->worker->output_pending = FALSE; g_mutex_unlock (data->worker->write_lock); /* OK, cool, finally kick off the next write */ @@ -1164,7 +1169,8 @@ message_written (GDBusWorker *worker, } if (flushers != NULL) { - worker->flush_pending = TRUE; + g_assert (!worker->output_pending); + worker->output_pending = TRUE; } g_mutex_unlock (worker->write_lock); @@ -1198,7 +1204,8 @@ write_message_cb (GObject *source_object, GError *error; g_mutex_lock (data->worker->write_lock); - data->worker->num_writes_pending -= 1; + g_assert (data->worker->output_pending); + data->worker->output_pending = FALSE; g_mutex_unlock (data->worker->write_lock); error = NULL; @@ -1225,15 +1232,17 @@ maybe_write_next_message (GDBusWorker *worker) MessageToWriteData *data; write_next: + /* we mustn't try to write two things at once */ + g_assert (!worker->output_pending); g_mutex_lock (worker->write_lock); data = g_queue_pop_head (worker->write_queue); if (data != NULL) - worker->num_writes_pending += 1; + worker->output_pending = TRUE; g_mutex_unlock (worker->write_lock); /* Note that write_lock is only used for protecting the @write_queue - * and @num_writes_pending fields of the GDBusWorker struct ... which we + * and @output_pending fields of the GDBusWorker struct ... which we * need to modify from arbitrary threads in _g_dbus_worker_send_message(). * * Therefore, it's fine to drop it here when calling back into user @@ -1257,7 +1266,7 @@ maybe_write_next_message (GDBusWorker *worker) { /* filters dropped message */ g_mutex_lock (worker->write_lock); - worker->num_writes_pending -= 1; + worker->output_pending = FALSE; g_mutex_unlock (worker->write_lock); message_to_write_data_free (data); goto write_next; @@ -1300,8 +1309,13 @@ static gboolean write_message_in_idle_cb (gpointer user_data) { GDBusWorker *worker = user_data; - if (worker->num_writes_pending == 0 && !worker->flush_pending) + + /* Because this is the worker thread, we can read this struct member + * without holding the lock: no other thread ever modifies it. + */ + if (!worker->output_pending) maybe_write_next_message (worker); + return FALSE; } @@ -1328,7 +1342,7 @@ _g_dbus_worker_send_message (GDBusWorker *worker, g_mutex_lock (worker->write_lock); g_queue_push_tail (worker->write_queue, data); - if (worker->num_writes_pending == 0) + if (!worker->output_pending) { GSource *idle_source; idle_source = g_idle_source_new (); @@ -1373,7 +1387,7 @@ _g_dbus_worker_new (GIOStream *stream, worker->stream = g_object_ref (stream); worker->capabilities = capabilities; worker->cancellable = g_cancellable_new (); - worker->flush_pending = FALSE; + worker->output_pending = FALSE; worker->frozen = initially_frozen; worker->received_messages_while_frozen = g_queue_new (); -- 2.7.4