Attachment 47660 Details for Bug 38005 – Consistently use atomic operations for all access to DBusConnection refcount

[patch] Consistently use atomic operations for all access to DBusConnection refcount

0001-Consistently-use-atomic-operations-for-all-access-to.patch (text/plain), 4.88 KB, created by Simon McVittie on 2011-06-07 06:08:30 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2011-06-07 06:08:30 UTC

Size: 4.88 KB

patch

obsolete

>From 424db582c4c2d9e4f86b7c1c68b1d7c3c95f3bf3 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Tue, 7 Jun 2011 13:48:05 +0100
>Subject: [PATCH] Consistently use atomic operations for all access to
> DBusConnection refcount
>
>Trying to mix atomic operations with locked non-atomic operations is
>broken: the atomic ops aren't necessarily atomic with respect to the
>locked non-atomic ops, and the non-atomic ops aren't protected by the
>lock because the atomic ops can change the refcount behind their back.
>
>In theory we could use the connection lock if atomic ops aren't supported
>(making a per-connection lock cheaper than the global lock used to
>implement atomic ops) *and* our mutexes are recursive (making it safe
>against deadlocks)... but life's too short.
>
>Bug: https://bugs.freedesktop.org/show_bug.cgi?id=38005
>---
> dbus/dbus-connection.c |   82 +++++++++--------------------------------------
> 1 files changed, 16 insertions(+), 66 deletions(-)
>
>diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
>index d0aca25..e81ed76 100644
>--- a/dbus/dbus-connection.c
>+++ b/dbus/dbus-connection.c
>@@ -1399,13 +1399,8 @@ _dbus_connection_ref_unlocked (DBusConnection *connection)
>   _dbus_assert (connection->generation == _dbus_current_generation);
> 
>   HAVE_LOCK_CHECK (connection);
>-  
>-#ifdef DBUS_HAVE_ATOMIC_INT
>+
>   _dbus_atomic_inc (&connection->refcount);
>-#else
>-  _dbus_assert (connection->refcount.value > 0);
>-  connection->refcount.value += 1;
>-#endif
> 
>   return connection;
> }
>@@ -1425,22 +1420,8 @@ _dbus_connection_unref_unlocked (DBusConnection *connection)
>   
>   _dbus_assert (connection != NULL);
> 
>-  /* The connection lock is better than the global
>-   * lock in the atomic increment fallback
>-   */
>-  
>-#ifdef DBUS_HAVE_ATOMIC_INT
>   last_unref = (_dbus_atomic_dec (&connection->refcount) == 1);
>-#else
>-  _dbus_assert (connection->refcount.value > 0);
> 
>-  connection->refcount.value -= 1;
>-  last_unref = (connection->refcount.value == 0);  
>-#if 0
>-  printf ("unref_unlocked() connection %p count = %d\n", connection, connection->refcount.value);
>-#endif
>-#endif
>-  
>   if (last_unref)
>     _dbus_connection_last_unref (connection);
> }
>@@ -2126,11 +2107,21 @@ _dbus_connection_send_and_unlock (DBusConnection *connection,
> void
> _dbus_connection_close_if_only_one_ref (DBusConnection *connection)
> {
>+  dbus_int32_t tmp_refcount;
>+
>   CONNECTION_LOCK (connection);
>-  
>-  _dbus_assert (connection->refcount.value > 0);
> 
>-  if (connection->refcount.value == 1)
>+  _dbus_atomic_inc (&connection->refcount);
>+
>+  tmp_refcount = _dbus_atomic_dec (&connection->refcount);
>+
>+  /* The caller should have one ref, and this function temporarily took
>+   * one more, which is reflected in this count even though we already
>+   * released it (relying on the caller's ref) due to _dbus_atomic_dec
>+   * semantics */
>+  _dbus_assert (tmp_refcount >= 2);
>+
>+  if (tmp_refcount == 2)
>     _dbus_connection_close_possibly_shared_and_unlock (connection);
>   else
>     CONNECTION_UNLOCK (connection);
>@@ -2632,25 +2623,8 @@ dbus_connection_ref (DBusConnection *connection)
> {
>   _dbus_return_val_if_fail (connection != NULL, NULL);
>   _dbus_return_val_if_fail (connection->generation == _dbus_current_generation, NULL);
>-  
>-  /* The connection lock is better than the global
>-   * lock in the atomic increment fallback
>-   *
>-   * (FIXME but for now we always use the atomic version,
>-   * to avoid taking the connection lock, due to
>-   * the mess with set_timeout_functions()/set_watch_functions()
>-   * calling out to the app without dropping locks)
>-   */
>-  
>-#if 1
>-  _dbus_atomic_inc (&connection->refcount);
>-#else
>-  CONNECTION_LOCK (connection);
>-  _dbus_assert (connection->refcount.value > 0);
> 
>-  connection->refcount.value += 1;
>-  CONNECTION_UNLOCK (connection);
>-#endif
>+  _dbus_atomic_inc (&connection->refcount);
> 
>   return connection;
> }
>@@ -2787,33 +2761,9 @@ dbus_connection_unref (DBusConnection *connection)
> 
>   _dbus_return_if_fail (connection != NULL);
>   _dbus_return_if_fail (connection->generation == _dbus_current_generation);
>-  
>-  /* The connection lock is better than the global
>-   * lock in the atomic increment fallback
>-   *
>-   * (FIXME but for now we always use the atomic version,
>-   * to avoid taking the connection lock, due to
>-   * the mess with set_timeout_functions()/set_watch_functions()
>-   * calling out to the app without dropping locks)
>-   */
>-  
>-#if 1
>-  last_unref = (_dbus_atomic_dec (&connection->refcount) == 1);
>-#else
>-  CONNECTION_LOCK (connection);
>-  
>-  _dbus_assert (connection->refcount.value > 0);
> 
>-  connection->refcount.value -= 1;
>-  last_unref = (connection->refcount.value == 0);
>+  last_unref = (_dbus_atomic_dec (&connection->refcount) == 1);
> 
>-#if 0
>-  printf ("unref() connection %p count = %d\n", connection, connection->refcount.value);
>-#endif
>-  
>-  CONNECTION_UNLOCK (connection);
>-#endif
>-  
>   if (last_unref)
>     {
> #ifndef DBUS_DISABLE_CHECKS
>-- 
>1.7.5.3
>

Actions: View | Splinter Review

Attachments on bug 38005: 47660 | 48335 | 48336 | 48337 | 48338 | 48339 | 48340 | 48341 | 48342 | 49096 | 49097 | 49098 | 49100 | 49102 | 49103 | 49349 | 49675

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.