From 57ab23491c8c80b4a1606ea3b72e179c1b742bb9 Mon Sep 17 00:00:00 2001 From: "John (J5) Palmieri" Date: Thu, 14 Sep 2006 04:26:00 +0000 Subject: [PATCH] * dbus/dbus-threads.c: Allow recursive mutex's to be passed into dbus_threads_init and be used by the dbus mutex functions to avoid deadlocks. * doc/TODO: Remove recursive mutex dbus_connection_dispatch TODO item --- ChangeLog | 8 ++++ dbus/dbus-dataslot.c | 9 ++-- dbus/dbus-list.c | 3 +- dbus/dbus-threads-internal.h | 4 +- dbus/dbus-threads.c | 105 ++++++++++++++++++++++++++++++++----------- dbus/dbus-threads.h | 23 +++++++--- doc/TODO | 7 --- 7 files changed, 109 insertions(+), 50 deletions(-) diff --git a/ChangeLog b/ChangeLog index ae2e5c4..beb6d26 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2006-09-14 John (J5) Palmieri + + * dbus/dbus-threads.c: Allow recursive mutex's to be passed into + dbus_threads_init and be used by the dbus mutex functions to + avoid deadlocks. + + * doc/TODO: Remove recursive mutex dbus_connection_dispatch TODO item + 2006-09-13 John (J5) Palmieri * dbus/dbus-sysdeps-util-unix.c (_dbus_directory_get_next_file): diff --git a/dbus/dbus-dataslot.c b/dbus/dbus-dataslot.c index d53f3bd..f9c12f8 100644 --- a/dbus/dbus-dataslot.c +++ b/dbus/dbus-dataslot.c @@ -70,8 +70,7 @@ _dbus_data_slot_allocator_alloc (DBusDataSlotAllocator *allocator, { dbus_int32_t slot; - if (!_dbus_mutex_lock (*mutex_loc)) - return FALSE; + _dbus_mutex_lock (*mutex_loc); if (allocator->n_allocated_slots == 0) { @@ -246,8 +245,7 @@ _dbus_data_slot_list_set (DBusDataSlotAllocator *allocator, * be e.g. realloc()ing allocated_slots. We avoid doing this if asserts * are disabled, since then the asserts are empty. */ - if (!_dbus_mutex_lock (*(allocator->lock_loc))) - return FALSE; + _dbus_mutex_lock (*(allocator->lock_loc)); _dbus_assert (slot < allocator->n_allocated_slots); _dbus_assert (allocator->allocated_slots[slot].slot_id == slot); _dbus_mutex_unlock (*(allocator->lock_loc)); @@ -304,8 +302,7 @@ _dbus_data_slot_list_get (DBusDataSlotAllocator *allocator, * be e.g. realloc()ing allocated_slots. We avoid doing this if asserts * are disabled, since then the asserts are empty. */ - if (!_dbus_mutex_lock (*(allocator->lock_loc))) - return NULL; + _dbus_mutex_lock (*(allocator->lock_loc)); _dbus_assert (slot >= 0); _dbus_assert (slot < allocator->n_allocated_slots); _dbus_assert (allocator->allocated_slots[slot].slot_id == slot); diff --git a/dbus/dbus-list.c b/dbus/dbus-list.c index 394db5f..09ece64 100644 --- a/dbus/dbus-list.c +++ b/dbus/dbus-list.c @@ -55,8 +55,7 @@ alloc_link (void *data) { DBusList *link; - if (!_DBUS_LOCK (list)) - return NULL; + _DBUS_LOCK (list); if (list_pool == NULL) { diff --git a/dbus/dbus-threads-internal.h b/dbus/dbus-threads-internal.h index 700c4f7..8f26bca 100644 --- a/dbus/dbus-threads-internal.h +++ b/dbus/dbus-threads-internal.h @@ -31,8 +31,8 @@ DBUS_BEGIN_DECLS DBusMutex* _dbus_mutex_new (void); void _dbus_mutex_free (DBusMutex *mutex); -dbus_bool_t _dbus_mutex_lock (DBusMutex *mutex); -dbus_bool_t _dbus_mutex_unlock (DBusMutex *mutex); +void _dbus_mutex_lock (DBusMutex *mutex); +void _dbus_mutex_unlock (DBusMutex *mutex); void _dbus_mutex_new_at_location (DBusMutex **location_p); void _dbus_mutex_free_at_location (DBusMutex **location_p); diff --git a/dbus/dbus-threads.c b/dbus/dbus-threads.c index 251dd45..8c7eb5e 100644 --- a/dbus/dbus-threads.c +++ b/dbus/dbus-threads.c @@ -39,10 +39,10 @@ static DBusThreadFunctions thread_functions = { 0, - NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, - + NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL }; @@ -87,7 +87,9 @@ static DBusList *uninitialized_condvar_list = NULL; DBusMutex* _dbus_mutex_new (void) { - if (thread_functions.mutex_new) + if (thread_functions.recursive_mutex_new) + return (* thread_functions.recursive_mutex_new) (); + else if (thread_functions.mutex_new) return (* thread_functions.mutex_new) (); else return _DBUS_DUMMY_MUTEX; @@ -126,8 +128,13 @@ _dbus_mutex_new_at_location (DBusMutex **location_p) void _dbus_mutex_free (DBusMutex *mutex) { - if (mutex && thread_functions.mutex_free) - (* thread_functions.mutex_free) (mutex); + if (mutex) + { + if (mutex && thread_functions.recursive_mutex_free) + (* thread_functions.recursive_mutex_free) (mutex); + else if (mutex && thread_functions.mutex_free) + (* thread_functions.mutex_free) (mutex); + } } /** @@ -149,17 +156,19 @@ _dbus_mutex_free_at_location (DBusMutex **location_p) /** * Locks a mutex. Does nothing if passed a #NULL pointer. - * Locks are not recursive. - * - * @returns #TRUE on success + * Locks may be recursive if threading implementation initialized + * recursive locks. */ -dbus_bool_t +void _dbus_mutex_lock (DBusMutex *mutex) { - if (mutex && thread_functions.mutex_lock) - return (* thread_functions.mutex_lock) (mutex); - else - return TRUE; + if (mutex) + { + if (thread_functions.recursive_mutex_lock) + (* thread_functions.recursive_mutex_lock) (mutex); + else if (thread_functions.mutex_lock) + (* thread_functions.mutex_lock) (mutex); + } } /** @@ -167,13 +176,16 @@ _dbus_mutex_lock (DBusMutex *mutex) * * @returns #TRUE on success */ -dbus_bool_t +void _dbus_mutex_unlock (DBusMutex *mutex) { - if (mutex && thread_functions.mutex_unlock) - return (* thread_functions.mutex_unlock) (mutex); - else - return TRUE; + if (mutex) + { + if (thread_functions.recursive_mutex_unlock) + (* thread_functions.recursive_mutex_unlock) (mutex); + else if (thread_functions.mutex_unlock) + (* thread_functions.mutex_unlock) (mutex); + } } /** @@ -515,25 +527,20 @@ init_locks (void) dbus_bool_t dbus_threads_init (const DBusThreadFunctions *functions) { + dbus_bool_t mutex_set; + dbus_bool_t recursive_mutex_set; + _dbus_assert (functions != NULL); /* these base functions are required. Future additions to * DBusThreadFunctions may be optional. */ - _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_MUTEX_NEW_MASK); - _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_MUTEX_FREE_MASK); - _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_MUTEX_LOCK_MASK); - _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_MUTEX_UNLOCK_MASK); _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_CONDVAR_NEW_MASK); _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_CONDVAR_FREE_MASK); _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_CONDVAR_WAIT_MASK); _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_CONDVAR_WAIT_TIMEOUT_MASK); _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_CONDVAR_WAKE_ONE_MASK); _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_CONDVAR_WAKE_ALL_MASK); - _dbus_assert (functions->mutex_new != NULL); - _dbus_assert (functions->mutex_free != NULL); - _dbus_assert (functions->mutex_lock != NULL); - _dbus_assert (functions->mutex_unlock != NULL); _dbus_assert (functions->condvar_new != NULL); _dbus_assert (functions->condvar_free != NULL); _dbus_assert (functions->condvar_wait != NULL); @@ -541,6 +548,40 @@ dbus_threads_init (const DBusThreadFunctions *functions) _dbus_assert (functions->condvar_wake_one != NULL); _dbus_assert (functions->condvar_wake_all != NULL); + /* Either the mutex function set or recursive mutex set needs + * to be available but not both + */ + mutex_set = (functions->mask & DBUS_THREAD_FUNCTIONS_MUTEX_NEW_MASK) && + (functions->mask & DBUS_THREAD_FUNCTIONS_MUTEX_FREE_MASK) && + (functions->mask & DBUS_THREAD_FUNCTIONS_MUTEX_LOCK_MASK) && + (functions->mask & DBUS_THREAD_FUNCTIONS_MUTEX_UNLOCK_MASK) && + functions->mutex_new && + functions->mutex_free && + functions->mutex_lock && + functions->mutex_unlock; + + recursive_mutex_set = + (functions->mask & DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_NEW_MASK) && + (functions->mask & DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_FREE_MASK) && + (functions->mask & DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_LOCK_MASK) && + (functions->mask & DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_UNLOCK_MASK) && + functions->recursive_mutex_new && + functions->recursive_mutex_free && + functions->recursive_mutex_lock && + functions->recursive_mutex_unlock; + + if (!(mutex_set || recursive_mutex_set)) + _dbus_assert_not_reached ("Either the nonrecusrive or recursive mutex " + "functions sets should be passed into " + "dbus_threads_init. Neither sets were passed."); + + if (mutex_set && recursive_mutex_set) + _dbus_assert_not_reached ("Either the nonrecusrive or recursive mutex " + "functions sets should be passed into " + "dbus_threads_init. Both sets were passed. " + "You most likely just want to set the recursive " + "mutex functions to avoid deadlocks in D-Bus."); + /* Check that all bits in the mask actually are valid mask bits. * ensures people won't write code that breaks when we add * new bits. @@ -567,7 +608,19 @@ dbus_threads_init (const DBusThreadFunctions *functions) thread_functions.condvar_wait_timeout = functions->condvar_wait_timeout; thread_functions.condvar_wake_one = functions->condvar_wake_one; thread_functions.condvar_wake_all = functions->condvar_wake_all; + + if (functions->mask & DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_NEW_MASK) + thread_functions.recursive_mutex_new = functions->recursive_mutex_new; + if (functions->mask & DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_FREE_MASK) + thread_functions.recursive_mutex_free = functions->recursive_mutex_free; + + if (functions->mask & DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_LOCK_MASK) + thread_functions.recursive_mutex_lock = functions->recursive_mutex_lock; + + if (functions->mask & DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_UNLOCK_MASK) + thread_functions.recursive_mutex_unlock = functions->recursive_mutex_unlock; + thread_functions.mask = functions->mask; if (!init_locks ()) diff --git a/dbus/dbus-threads.h b/dbus/dbus-threads.h index 8d8e072..5e5a67e 100644 --- a/dbus/dbus-threads.h +++ b/dbus/dbus-threads.h @@ -40,6 +40,11 @@ typedef void (* DBusMutexFreeFunction) (DBusMutex *mutex); typedef dbus_bool_t (* DBusMutexLockFunction) (DBusMutex *mutex); typedef dbus_bool_t (* DBusMutexUnlockFunction) (DBusMutex *mutex); +typedef DBusMutex* (* DBusRecursiveMutexNewFunction) (void); +typedef void (* DBusRecursiveMutexFreeFunction) (DBusMutex *mutex); +typedef void (* DBusRecursiveMutexLockFunction) (DBusMutex *mutex); +typedef void (* DBusRecursiveMutexUnlockFunction) (DBusMutex *mutex); + typedef DBusCondVar* (* DBusCondVarNewFunction) (void); typedef void (* DBusCondVarFreeFunction) (DBusCondVar *cond); typedef void (* DBusCondVarWaitFunction) (DBusCondVar *cond, @@ -62,8 +67,11 @@ typedef enum DBUS_THREAD_FUNCTIONS_CONDVAR_WAIT_TIMEOUT_MASK = 1 << 7, DBUS_THREAD_FUNCTIONS_CONDVAR_WAKE_ONE_MASK = 1 << 8, DBUS_THREAD_FUNCTIONS_CONDVAR_WAKE_ALL_MASK = 1 << 9, - - DBUS_THREAD_FUNCTIONS_ALL_MASK = (1 << 10) - 1 + DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_NEW_MASK = 1 << 10, + DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_FREE_MASK = 1 << 11, + DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_LOCK_MASK = 1 << 12, + DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_UNLOCK_MASK = 1 << 13, + DBUS_THREAD_FUNCTIONS_ALL_MASK = (1 << 13) - 1 } DBusThreadFunctionsMask; /** @@ -85,15 +93,16 @@ typedef struct DBusCondVarWaitTimeoutFunction condvar_wait_timeout; /**< Function to wait on a condition with a timeout */ DBusCondVarWakeOneFunction condvar_wake_one; /**< Function to wake one thread waiting on the condition */ DBusCondVarWakeAllFunction condvar_wake_all; /**< Function to wake all threads waiting on the condition */ - + + DBusRecursiveMutexNewFunction recursive_mutex_new; /**< Function to create a recursive mutex */ + DBusRecursiveMutexFreeFunction recursive_mutex_free; /**< Function to free a recursive mutex */ + DBusRecursiveMutexLockFunction recursive_mutex_lock; /**< Function to lock a recursive mutex */ + DBusRecursiveMutexUnlockFunction recursive_mutex_unlock; /**< Function to unlock a recursive mutex */ + void (* padding1) (void); /**< Reserved for future expansion */ void (* padding2) (void); /**< Reserved for future expansion */ void (* padding3) (void); /**< Reserved for future expansion */ void (* padding4) (void); /**< Reserved for future expansion */ - void (* padding5) (void); /**< Reserved for future expansion */ - void (* padding6) (void); /**< Reserved for future expansion */ - void (* padding7) (void); /**< Reserved for future expansion */ - void (* padding8) (void); /**< Reserved for future expansion */ } DBusThreadFunctions; diff --git a/doc/TODO b/doc/TODO index d3ece32..fc9ef2e 100644 --- a/doc/TODO +++ b/doc/TODO @@ -1,13 +1,6 @@ Important for 1.0 === - - add a new return code from dbus_connection_dispatch() called - IN_PROGRESS or RECURSED or something, indicating that DATA_REMAINS - but another dispatch is in progress, so we can't dispatch at - this time. OR maybe just switch to recursive locks for the dispatch - locks. Fixes the recursive deadlock. See the @todo for more - and this thread: http://lists.freedesktop.org/archives/dbus/2006-February/004128.html - - Take a look at the issues marked @todo 1.0 or FIXME 1.0. Ones with Question marks at the ends either need clarification or are not really needed for 1.0 but would be nice. -- 2.7.4