From e309dffaa9af7f0ef11b0bbbf8c059b16a98e48c Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 15 Apr 2013 21:16:10 +0100 Subject: [PATCH 7/8] Always initialize threading before allocating a dynamic mutex Dynamic allocation of mutexes can fail anyway, so this is easy. Justification for not keeping the dummy mutex code-paths, even as an opt-in thing for processes known to be high-performance and single-threaded: real mutexes only cut the throughput of test/dbus-daemon.c by a couple of percent on my laptop (from around 6700 to around 6600 messages per second), and libdbus crashes caused by not calling dbus_threads_init_default() are sufficiently widespread that they're wasting a lot of everyone's time. --- dbus/dbus-threads-internal.h | 2 + dbus/dbus-threads.c | 297 +++++++++--------------------------- test/name-test/test-threads-init.c | 26 +++- 3 files changed, 99 insertions(+), 226 deletions(-) diff --git a/dbus/dbus-threads-internal.h b/dbus/dbus-threads-internal.h index 813dd9e..79a0be9 100644 --- a/dbus/dbus-threads-internal.h +++ b/dbus/dbus-threads-internal.h @@ -78,6 +78,8 @@ dbus_bool_t _dbus_global_lock (DBusGlobalMutex mutex) _DBUS_GNU void _dbus_global_lock_cannot_fail (DBusGlobalMutex mutex); void _dbus_global_unlock (DBusGlobalMutex mutex); +dbus_bool_t _dbus_threads_init (void); + void _dbus_rmutex_lock (DBusRMutex *mutex); void _dbus_rmutex_unlock (DBusRMutex *mutex); void _dbus_rmutex_new_at_location (DBusRMutex **location_p); diff --git a/dbus/dbus-threads.c b/dbus/dbus-threads.c index aa85680..f4c068e 100644 --- a/dbus/dbus-threads.c +++ b/dbus/dbus-threads.c @@ -27,18 +27,6 @@ #include "dbus-list.h" static int thread_init_generation = 0; - -static DBusList *uninitialized_rmutex_list = NULL; -static DBusList *uninitialized_cmutex_list = NULL; -static DBusList *uninitialized_condvar_list = NULL; - -/** This is used for the no-op default mutex pointer, just to be distinct from #NULL */ -#define _DBUS_DUMMY_MUTEX ((DBusMutex*)0xABCDEF) -#define _DBUS_DUMMY_RMUTEX ((DBusRMutex *) _DBUS_DUMMY_MUTEX) -#define _DBUS_DUMMY_CMUTEX ((DBusCMutex *) _DBUS_DUMMY_MUTEX) - -/** This is used for the no-op default mutex pointer, just to be distinct from #NULL */ -#define _DBUS_DUMMY_CONDVAR ((DBusCondVar*)0xABCDEF2) /** * @defgroup DBusThreadsInternals Thread functions @@ -71,17 +59,13 @@ _dbus_rmutex_new_at_location (DBusRMutex **location_p) { _dbus_assert (location_p != NULL); - if (thread_init_generation == _dbus_current_generation) + if (!_dbus_threads_init ()) { - *location_p = _dbus_platform_rmutex_new (); + *location_p = NULL; + return; } - else - { - *location_p = _DBUS_DUMMY_RMUTEX; - if (!_dbus_list_append (&uninitialized_rmutex_list, location_p)) - *location_p = NULL; - } + *location_p = _dbus_platform_rmutex_new (); } /** @@ -104,22 +88,17 @@ _dbus_cmutex_new_at_location (DBusCMutex **location_p) { _dbus_assert (location_p != NULL); - if (thread_init_generation == _dbus_current_generation) + if (!_dbus_threads_init ()) { - *location_p = _dbus_platform_cmutex_new (); + *location_p = NULL; + return; } - else - { - *location_p = _DBUS_DUMMY_CMUTEX; - if (!_dbus_list_append (&uninitialized_cmutex_list, location_p)) - *location_p = NULL; - } + *location_p = _dbus_platform_cmutex_new (); } /** - * Frees a DBusRMutex or removes it from the uninitialized mutex list; - * does nothing if passed a #NULL pointer. + * Frees a DBusRMutex; does nothing if passed a #NULL pointer. */ void _dbus_rmutex_free_at_location (DBusRMutex **location_p) @@ -127,23 +106,12 @@ _dbus_rmutex_free_at_location (DBusRMutex **location_p) if (location_p == NULL) return; - if (thread_init_generation == _dbus_current_generation) - { - if (*location_p != NULL) - _dbus_platform_rmutex_free (*location_p); - } - else - { - _dbus_assert (*location_p == NULL || *location_p == _DBUS_DUMMY_RMUTEX); - - _dbus_list_remove (&uninitialized_rmutex_list, location_p); - } + if (*location_p != NULL) + _dbus_platform_rmutex_free (*location_p); } /** - * Frees a DBusCMutex and removes it from the - * uninitialized mutex list; - * does nothing if passed a #NULL pointer. + * Frees a DBusCMutex; does nothing if passed a #NULL pointer. */ void _dbus_cmutex_free_at_location (DBusCMutex **location_p) @@ -151,17 +119,8 @@ _dbus_cmutex_free_at_location (DBusCMutex **location_p) if (location_p == NULL) return; - if (thread_init_generation == _dbus_current_generation) - { - if (*location_p != NULL) - _dbus_platform_cmutex_free (*location_p); - } - else - { - _dbus_assert (*location_p == NULL || *location_p == _DBUS_DUMMY_CMUTEX); - - _dbus_list_remove (&uninitialized_cmutex_list, location_p); - } + if (*location_p != NULL) + _dbus_platform_cmutex_free (*location_p); } /** @@ -172,8 +131,10 @@ _dbus_cmutex_free_at_location (DBusCMutex **location_p) void _dbus_rmutex_lock (DBusRMutex *mutex) { - if (mutex && thread_init_generation == _dbus_current_generation) - _dbus_platform_rmutex_lock (mutex); + if (mutex == NULL) + return; + + _dbus_platform_rmutex_lock (mutex); } /** @@ -184,8 +145,10 @@ _dbus_rmutex_lock (DBusRMutex *mutex) void _dbus_cmutex_lock (DBusCMutex *mutex) { - if (mutex && thread_init_generation == _dbus_current_generation) - _dbus_platform_cmutex_lock (mutex); + if (mutex == NULL) + return; + + _dbus_platform_cmutex_lock (mutex); } /** @@ -196,8 +159,10 @@ _dbus_cmutex_lock (DBusCMutex *mutex) void _dbus_rmutex_unlock (DBusRMutex *mutex) { - if (mutex && thread_init_generation == _dbus_current_generation) - _dbus_platform_rmutex_unlock (mutex); + if (mutex == NULL) + return; + + _dbus_platform_rmutex_unlock (mutex); } /** @@ -208,8 +173,10 @@ _dbus_rmutex_unlock (DBusRMutex *mutex) void _dbus_cmutex_unlock (DBusCMutex *mutex) { - if (mutex && thread_init_generation == _dbus_current_generation) - _dbus_platform_cmutex_unlock (mutex); + if (mutex == NULL) + return; + + _dbus_platform_cmutex_unlock (mutex); } /** @@ -223,10 +190,10 @@ _dbus_cmutex_unlock (DBusCMutex *mutex) DBusCondVar * _dbus_condvar_new (void) { - if (thread_init_generation == _dbus_current_generation) - return _dbus_platform_condvar_new (); - else - return _DBUS_DUMMY_CONDVAR; + if (!_dbus_threads_init ()) + return NULL; + + return _dbus_platform_condvar_new (); } @@ -245,17 +212,13 @@ _dbus_condvar_new_at_location (DBusCondVar **location_p) { _dbus_assert (location_p != NULL); - if (thread_init_generation == _dbus_current_generation) + if (!_dbus_threads_init ()) { - *location_p = _dbus_condvar_new(); + *location_p = NULL; + return; } - else - { - *location_p = _DBUS_DUMMY_CONDVAR; - if (!_dbus_list_append (&uninitialized_condvar_list, location_p)) - *location_p = NULL; - } + *location_p = _dbus_condvar_new(); } @@ -266,14 +229,14 @@ _dbus_condvar_new_at_location (DBusCondVar **location_p) void _dbus_condvar_free (DBusCondVar *cond) { - if (cond && thread_init_generation == _dbus_current_generation) - _dbus_platform_condvar_free (cond); + if (cond == NULL) + return; + + _dbus_platform_condvar_free (cond); } /** - * Frees a conditional variable and removes it from the - * uninitialized_condvar_list; - * does nothing if passed a #NULL pointer. + * Frees a condition variable; does nothing if passed a #NULL pointer. */ void _dbus_condvar_free_at_location (DBusCondVar **location_p) @@ -281,17 +244,8 @@ _dbus_condvar_free_at_location (DBusCondVar **location_p) if (location_p == NULL) return; - if (thread_init_generation == _dbus_current_generation) - { - if (*location_p != NULL) - _dbus_platform_condvar_free (*location_p); - } - else - { - _dbus_assert (*location_p == NULL || *location_p == _DBUS_DUMMY_CONDVAR); - - _dbus_list_remove (&uninitialized_condvar_list, location_p); - } + if (*location_p != NULL) + _dbus_platform_condvar_free (*location_p); } /** @@ -304,8 +258,10 @@ void _dbus_condvar_wait (DBusCondVar *cond, DBusCMutex *mutex) { - if (cond && mutex && thread_init_generation == _dbus_current_generation) - _dbus_platform_condvar_wait (cond, mutex); + if (cond == NULL || mutex == NULL) + return; + + _dbus_platform_condvar_wait (cond, mutex); } /** @@ -324,11 +280,11 @@ _dbus_condvar_wait_timeout (DBusCondVar *cond, DBusCMutex *mutex, int timeout_milliseconds) { - if (cond && mutex && thread_init_generation == _dbus_current_generation) - return _dbus_platform_condvar_wait_timeout (cond, mutex, - timeout_milliseconds); - else + if (cond == NULL || mutex == NULL) return TRUE; + + return _dbus_platform_condvar_wait_timeout (cond, mutex, + timeout_milliseconds); } /** @@ -339,128 +295,10 @@ _dbus_condvar_wait_timeout (DBusCondVar *cond, void _dbus_condvar_wake_one (DBusCondVar *cond) { - if (cond && thread_init_generation == _dbus_current_generation) - _dbus_platform_condvar_wake_one (cond); -} - -static void -shutdown_uninitialized_locks (void *data) -{ - _dbus_list_clear (&uninitialized_rmutex_list); - _dbus_list_clear (&uninitialized_cmutex_list); - _dbus_list_clear (&uninitialized_condvar_list); -} - -static dbus_bool_t -init_uninitialized_locks (void) -{ - DBusList *link; - - _dbus_assert (thread_init_generation != _dbus_current_generation); - - link = uninitialized_rmutex_list; - while (link != NULL) - { - DBusRMutex **mp; - - mp = link->data; - _dbus_assert (*mp == _DBUS_DUMMY_RMUTEX); - - *mp = _dbus_platform_rmutex_new (); - if (*mp == NULL) - goto fail_mutex; - - link = _dbus_list_get_next_link (&uninitialized_rmutex_list, link); - } - - link = uninitialized_cmutex_list; - while (link != NULL) - { - DBusCMutex **mp; - - mp = link->data; - _dbus_assert (*mp == _DBUS_DUMMY_CMUTEX); - - *mp = _dbus_platform_cmutex_new (); - if (*mp == NULL) - goto fail_mutex; - - link = _dbus_list_get_next_link (&uninitialized_cmutex_list, link); - } - - link = uninitialized_condvar_list; - while (link != NULL) - { - DBusCondVar **cp; - - cp = (DBusCondVar **)link->data; - _dbus_assert (*cp == _DBUS_DUMMY_CONDVAR); - - *cp = _dbus_platform_condvar_new (); - if (*cp == NULL) - goto fail_condvar; - - link = _dbus_list_get_next_link (&uninitialized_condvar_list, link); - } - - _dbus_list_clear (&uninitialized_rmutex_list); - _dbus_list_clear (&uninitialized_cmutex_list); - _dbus_list_clear (&uninitialized_condvar_list); - - if (!_dbus_register_shutdown_func (shutdown_uninitialized_locks, - NULL)) - goto fail_condvar; - - return TRUE; - - fail_condvar: - link = uninitialized_condvar_list; - while (link != NULL) - { - DBusCondVar **cp; - - cp = link->data; - - if (*cp != _DBUS_DUMMY_CONDVAR && *cp != NULL) - _dbus_platform_condvar_free (*cp); - - *cp = _DBUS_DUMMY_CONDVAR; - - link = _dbus_list_get_next_link (&uninitialized_condvar_list, link); - } - - fail_mutex: - link = uninitialized_rmutex_list; - while (link != NULL) - { - DBusRMutex **mp; - - mp = link->data; - - if (*mp != _DBUS_DUMMY_RMUTEX && *mp != NULL) - _dbus_platform_rmutex_free (*mp); - - *mp = _DBUS_DUMMY_RMUTEX; - - link = _dbus_list_get_next_link (&uninitialized_rmutex_list, link); - } - - link = uninitialized_cmutex_list; - while (link != NULL) - { - DBusCMutex **mp; - - mp = link->data; - - if (*mp != _DBUS_DUMMY_CMUTEX && *mp != NULL) - _dbus_platform_cmutex_free (*mp); - - *mp = _DBUS_DUMMY_CMUTEX; - - link = _dbus_list_get_next_link (&uninitialized_cmutex_list, link); - } + if (cond == NULL) + return; - return FALSE; + _dbus_platform_condvar_wake_one (cond); } /* All NULL-initialized (ISO C guarantees that partial initializers @@ -532,7 +370,7 @@ _dbus_global_lock (DBusGlobalMutex mutex) * or we're already multi-threaded, in which case the application is * already broken and we're unlikely to make it any worse. */ - if (global_locks[0] == NULL && !init_global_locks ()) + if (global_locks[0] == NULL && !_dbus_threads_init ()) return FALSE; _dbus_assert (global_locks[mutex] != NULL); @@ -571,6 +409,24 @@ _dbus_global_unlock (DBusGlobalMutex mutex) _dbus_platform_rmutex_unlock (global_locks[mutex]); } +/** + * Initialize threading in a platform-specific way. + * + * On Unix, it is safe to call this function from any thread. It will + * do the equivalent of dbus_threads_init_default() whenever necessary. + * + * On Windows, if dbus_threads_init_default() has not been called, it + * is not safe to call this function, but it's no worse than calling + * any other libdbus API from the same place. + * + * Returns: #FALSE if no memory + */ +dbus_bool_t +_dbus_threads_init (void) +{ + return dbus_threads_init_default (); +} + /** @} */ /* end of internals */ /** @@ -614,8 +470,7 @@ dbus_threads_init (const DBusThreadFunctions *functions) } if (!_dbus_threads_init_platform_specific() || - !init_global_locks () || - !init_uninitialized_locks ()) + !init_global_locks ()) { _dbus_threads_unlock_platform_specific (); return FALSE; diff --git a/test/name-test/test-threads-init.c b/test/name-test/test-threads-init.c index 5e22852..ab0e9e3 100644 --- a/test/name-test/test-threads-init.c +++ b/test/name-test/test-threads-init.c @@ -128,6 +128,20 @@ main (int argc, char *argv[]) &io_path_mutex1, &dispatch_cond1, &io_path_cond1); + + /* Now that we automatically allocate locks, we can assert that + * these are non-NULL and non-dummy. */ + _dbus_assert (mutex1 != NULL); + _dbus_assert (dispatch_mutex1 != NULL); + _dbus_assert (io_path_mutex1 != NULL); + _dbus_assert (dispatch_cond1 != NULL); + _dbus_assert (io_path_cond1 != NULL); + _dbus_assert (mutex1 != (DBusMutex *) 0xABCDEF); + _dbus_assert (dispatch_mutex1 != (DBusMutex *) 0xABCDEF); + _dbus_assert (io_path_mutex1 != (DBusMutex *) 0xABCDEF); + _dbus_assert (dispatch_cond1 != (DBusCondVar *) 0xABCDEF2); + _dbus_assert (io_path_cond1 != (DBusCondVar *) 0xABCDEF2); + _run_iteration (conn); _dbus_connection_test_get_locks (conn, &mutex2, &dispatch_mutex2, @@ -149,11 +163,13 @@ main (int argc, char *argv[]) &dispatch_cond1, &io_path_cond1); - check_mutex_lock (mutex1, mutex2, FALSE); - check_mutex_lock (dispatch_mutex1, dispatch_mutex2, FALSE); - check_mutex_lock (io_path_mutex1, io_path_mutex2, FALSE); - check_condvar_lock (dispatch_cond1, dispatch_cond2, FALSE); - check_condvar_lock (io_path_cond1, io_path_cond2, FALSE); + /* At this point we previously asserted that they had changed, + * but now we can assert that they have *not* changed. */ + check_mutex_lock (mutex1, mutex2, TRUE); + check_mutex_lock (dispatch_mutex1, dispatch_mutex2, TRUE); + check_mutex_lock (io_path_mutex1, io_path_mutex2, TRUE); + check_condvar_lock (dispatch_cond1, dispatch_cond2, TRUE); + check_condvar_lock (io_path_cond1, io_path_cond2, TRUE); _run_iteration (conn); _dbus_connection_test_get_locks (conn, &mutex2, -- 1.7.10.4