Re: thread-safety of counters

Adrian Szyndela <[email protected]> Thu, 26 Mar 2015 16:24:52 +0100

Newsgroups	gmane.comp.freedesktop.dbus
Message-ID	<[email protected]>

W dniu 25.02.2015 o 09:54, Adrian Szyndela pisze:
> I just need a few answers, and I'll prepare a patch:
> an existing mutex or a new one? If an existing, then which one? If a new
> one, then to what objects should it be added? Counters? Messages?

I'll try to answer myself.

We can't use existing mutexes from DBusConnection, because there might
be already no connection object while freeing a message.

Using one of global locks seems to make no sense for local objects.

I found only one more mutex, in DBusServer, but that is not relevant.

So, to use a mutex, we need to introduce a new one. As it is needed only
for DBusCounter objects locally, it seems natural to introduce such
mutex in there.

I attach a patch that does exactly this.

> some rationale for "mutex vs. atomic"?

For future reference: an example. In _dbus_counter_notify()
notify_function is called depending on notify_pending field value. On
the other hand _dbus_counter_set_notify() can be called from
_dbus_transport_finalize_base() at the same time, interfering with
_dbus_counter_notify(). This one would be hard to fix with atomics.

_______________________________________________
dbus mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dbus

mutex-counters.patch (text/x-patch, 3.8 KB)

diff --git a/dbus/dbus-resources.c b/dbus/dbus-resources.c
index 80fb55b..f76fd9f 100644
--- a/dbus/dbus-resources.c
+++ b/dbus/dbus-resources.c
@@ -69,6 +69,7 @@ struct DBusCounter
   DBusCounterNotifyFunction notify_function; /**< notify function */
   void *notify_data; /**< data for notify function */
   dbus_bool_t notify_pending : 1; /**< TRUE if the guard value has been crossed */
+  DBusRMutex *mutex;    /**< Lock on the entire DBusCounter */
 };
 
 /** @} */  /* end of resource limits internals docs */
@@ -95,6 +96,13 @@ _dbus_counter_new (void)
 
   counter->refcount = 1;
 
+  _dbus_rmutex_new_at_location (&counter->mutex);
+  if (counter->mutex == NULL)
+  {
+    dbus_free (counter);
+	counter = NULL;
+  }
+
   return counter;
 }
 
@@ -107,10 +115,14 @@ _dbus_counter_new (void)
 DBusCounter *
 _dbus_counter_ref (DBusCounter *counter)
 {
+  _dbus_rmutex_lock (counter->mutex);
+
   _dbus_assert (counter->refcount > 0);
   
   counter->refcount += 1;
 
+  _dbus_rmutex_unlock (counter->mutex);
+
   return counter;
 }
 
@@ -123,13 +135,20 @@ _dbus_counter_ref (DBusCounter *counter)
 void
 _dbus_counter_unref (DBusCounter *counter)
 {
+  dbus_bool_t last_ref = FALSE;
+
+  _dbus_rmutex_lock (counter->mutex);
+
   _dbus_assert (counter->refcount > 0);
 
   counter->refcount -= 1;
+  last_ref = counter->refcount == 0;
+
+  _dbus_rmutex_unlock (counter->mutex);
 
-  if (counter->refcount == 0)
+  if (last_ref)
     {
-      
+      _dbus_rmutex_free_at_location (&counter->mutex);
       dbus_free (counter);
     }
 }
@@ -148,7 +167,11 @@ void
 _dbus_counter_adjust_size (DBusCounter *counter,
                            long         delta)
 {
-  long old = counter->size_value;
+  long old = 0;
+
+  _dbus_rmutex_lock (counter->mutex);
+  
+  old = counter->size_value;
 
   counter->size_value += delta;
 
@@ -168,6 +191,8 @@ _dbus_counter_adjust_size (DBusCounter *counter,
        (old >= counter->notify_size_guard_value &&
         counter->size_value < counter->notify_size_guard_value)))
     counter->notify_pending = TRUE;
+
+  _dbus_rmutex_unlock (counter->mutex);
 }
 
 /**
@@ -181,11 +206,20 @@ _dbus_counter_adjust_size (DBusCounter *counter,
 void
 _dbus_counter_notify (DBusCounter *counter)
 {
+  DBusCounterNotifyFunction notify_function = NULL;
+  void *notify_data = NULL;
+
+  _dbus_rmutex_lock (counter->mutex);
   if (counter->notify_pending)
     {
       counter->notify_pending = FALSE;
-      (* counter->notify_function) (counter, counter->notify_data);
+	  notify_function = counter->notify_function;
+	  notify_data = counter->notify_data;
     }
+  _dbus_rmutex_unlock (counter->mutex);
+
+  if (notify_function != NULL)
+    (* notify_function) (counter, notify_data);
 }
 
 /**
@@ -202,7 +236,11 @@ void
 _dbus_counter_adjust_unix_fd (DBusCounter *counter,
                               long         delta)
 {
-  long old = counter->unix_fd_value;
+  long old = 0;
+
+  _dbus_rmutex_lock (counter->mutex);
+
+  old = counter->unix_fd_value;
   
   counter->unix_fd_value += delta;
 
@@ -222,6 +260,8 @@ _dbus_counter_adjust_unix_fd (DBusCounter *counter,
        (old >= counter->notify_unix_fd_guard_value &&
         counter->unix_fd_value < counter->notify_unix_fd_guard_value)))
     counter->notify_pending = TRUE;
+
+  _dbus_rmutex_unlock (counter->mutex);
 }
 
 /**
@@ -266,11 +306,13 @@ _dbus_counter_set_notify (DBusCounter               *counter,
                           DBusCounterNotifyFunction  function,
                           void                      *user_data)
 {
+  _dbus_rmutex_lock (counter->mutex);
   counter->notify_size_guard_value = size_guard_value;
   counter->notify_unix_fd_guard_value = unix_fd_guard_value;
   counter->notify_function = function;
   counter->notify_data = user_data;
   counter->notify_pending = FALSE;
+  _dbus_rmutex_unlock (counter->mutex);
 }
 
 #ifdef DBUS_ENABLE_STATS