From: Mathieu Desnoyers Date: Tue, 3 Dec 2013 21:47:52 +0000 (+0100) Subject: Fix: baddr deadlock with lttng-ust destructor X-Git-Tag: v2.4.0-rc2~4 X-Git-Url: https://git.lttng.org/?p=lttng-ust.git;a=commitdiff_plain;h=3327ac33b865cf2ee76934ded1c4b3b177edc3b9 Fix: baddr deadlock with lttng-ust destructor Was hanging lttng-tools tests/regression/ust/overlap/test_overlap Signed-off-by: Mathieu Desnoyers --- diff --git a/include/lttng/ust-events.h b/include/lttng/ust-events.h index d93c9121..d541f81f 100644 --- a/include/lttng/ust-events.h +++ b/include/lttng/ust-events.h @@ -610,7 +610,7 @@ struct cds_list_head *lttng_get_probe_list_head(void); int lttng_session_active(void); typedef int (*t_statedump_func_ptr)(struct lttng_session *session); -int lttng_handle_pending_statedump(void *owner); +void lttng_handle_pending_statedump(void *owner); struct cds_list_head *_lttng_get_sessions(void); #endif /* _LTTNG_UST_EVENTS_H */ diff --git a/liblttng-ust/lttng-events.c b/liblttng-ust/lttng-events.c index 6e16cf25..55d84f05 100644 --- a/liblttng-ust/lttng-events.c +++ b/liblttng-ust/lttng-events.c @@ -60,21 +60,9 @@ #include "jhash.h" /* - * The sessions mutex is the centralized mutex across UST tracing - * control and probe registration. All operations within this file are - * called by the communication thread, under ust_lock protection. + * All operations within this file are called by the communication + * thread, under ust_lock protection. */ -static pthread_mutex_t sessions_mutex = PTHREAD_MUTEX_INITIALIZER; - -void ust_lock(void) -{ - pthread_mutex_lock(&sessions_mutex); -} - -void ust_unlock(void) -{ - pthread_mutex_unlock(&sessions_mutex); -} static CDS_LIST_HEAD(sessions); @@ -686,7 +674,7 @@ int lttng_fix_pending_events(void) * Only dump state for the sessions owned by the caller thread, because * we don't keep ust_lock across the entire iteration. */ -int lttng_handle_pending_statedump(void *owner) +void lttng_handle_pending_statedump(void *owner) { struct lttng_session *session; @@ -694,7 +682,9 @@ int lttng_handle_pending_statedump(void *owner) lttng_ust_baddr_statedump(owner); /* Clear pending state dump */ - ust_lock(); + if (ust_lock()) { + goto end; + } cds_list_for_each_entry(session, &sessions, node) { if (session->owner != owner) continue; @@ -702,8 +692,9 @@ int lttng_handle_pending_statedump(void *owner) continue; session->statedump_pending = 0; } +end: ust_unlock(); - return 0; + return; } /* diff --git a/liblttng-ust/lttng-probes.c b/liblttng-ust/lttng-probes.c index 433171cf..bba5cd34 100644 --- a/liblttng-ust/lttng-probes.c +++ b/liblttng-ust/lttng-probes.c @@ -201,7 +201,7 @@ int lttng_probe_register(struct lttng_probe_desc *desc) if (!check_provider_version(desc)) return 0; - ust_lock(); + ust_lock_nocheck(); /* * Check if the provider has already been registered. @@ -237,7 +237,7 @@ void lttng_probe_unregister(struct lttng_probe_desc *desc) if (!check_provider_version(desc)) return; - ust_lock(); + ust_lock_nocheck(); if (!desc->lazy) cds_list_del(&desc->head); else diff --git a/liblttng-ust/lttng-tracer-core.h b/liblttng-ust/lttng-tracer-core.h index 57df175f..f7e61f05 100644 --- a/liblttng-ust/lttng-tracer-core.h +++ b/liblttng-ust/lttng-tracer-core.h @@ -34,7 +34,8 @@ struct lttng_session; struct lttng_channel; struct lttng_event; -void ust_lock(void); +int ust_lock(void) __attribute__ ((warn_unused_result)); +void ust_lock_nocheck(void); void ust_unlock(void); void lttng_fixup_event_tls(void); diff --git a/liblttng-ust/lttng-ust-baddr.c b/liblttng-ust/lttng-ust-baddr.c index 567741ad..681ba543 100644 --- a/liblttng-ust/lttng-ust-baddr.c +++ b/liblttng-ust/lttng-ust-baddr.c @@ -91,7 +91,14 @@ int extract_soinfo_events(struct dl_phdr_info *info, size_t size, void *data) * UST lock needs to be nested within dynamic loader * lock. */ - ust_lock(); + if (ust_lock()) { + /* + * Stop iteration on headers if need to exit. + */ + ust_unlock(); + return 1; + } + sessionsp = _lttng_get_sessions(); cds_list_for_each_entry(session, sessionsp, node) { if (session->owner != owner) @@ -104,7 +111,6 @@ int extract_soinfo_events(struct dl_phdr_info *info, size_t size, void *data) sostat.st_mtime); } ust_unlock(); - /* * We are only interested in the base address (lowest virtual * address associated with the memory image), skip the rest diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c index 256cbdc6..c6640954 100644 --- a/liblttng-ust/lttng-ust-comm.c +++ b/liblttng-ust/lttng-ust-comm.c @@ -63,11 +63,55 @@ static int initialized; * The ust_lock/ust_unlock lock is used as a communication thread mutex. * Held when handling a command, also held by fork() to deal with * removal of threads, and by exit path. + * + * The UST lock is the centralized mutex across UST tracing control and + * probe registration. + * + * ust_exit_mutex must never nest in ust_mutex. + */ +static pthread_mutex_t ust_mutex = PTHREAD_MUTEX_INITIALIZER; + +/* + * ust_exit_mutex protects thread_active variable wrt thread exit. It + * cannot be done by ust_mutex because pthread_cancel(), which takes an + * internal libc lock, cannot nest within ust_mutex. + * + * It never nests within a ust_mutex. */ +static pthread_mutex_t ust_exit_mutex = PTHREAD_MUTEX_INITIALIZER; /* Should the ust comm thread quit ? */ static int lttng_ust_comm_should_quit; +/* + * Return 0 on success, -1 if should quilt. + * The lock is taken in both cases. + */ +int ust_lock(void) +{ + pthread_mutex_lock(&ust_mutex); + if (lttng_ust_comm_should_quit) { + return -1; + } else { + return 0; + } +} + +/* + * ust_lock_nocheck() can be used in constructors/destructors, because + * they are already nested within the dynamic loader lock, and therefore + * have exclusive access against execution of liblttng-ust destructor. + */ +void ust_lock_nocheck(void) +{ + pthread_mutex_lock(&ust_mutex); +} + +void ust_unlock(void) +{ + pthread_mutex_unlock(&ust_mutex); +} + /* * Wait for either of these before continuing to the main * program: @@ -420,11 +464,9 @@ int handle_message(struct sock_info *sock_info, union ust_args args; ssize_t len; - ust_lock(); - memset(&lur, 0, sizeof(lur)); - if (lttng_ust_comm_should_quit) { + if (ust_lock()) { ret = -LTTNG_UST_ERR_EXITING; goto end; } @@ -983,8 +1025,7 @@ void wait_for_sessiond(struct sock_info *sock_info) { int ret; - ust_lock(); - if (lttng_ust_comm_should_quit) { + if (ust_lock()) { goto quit; } if (wait_poll_fallback) { @@ -1089,9 +1130,7 @@ restart: DBG("Info: sessiond not accepting connections to %s apps socket", sock_info->name); prev_connect_failed = 1; - ust_lock(); - - if (lttng_ust_comm_should_quit) { + if (ust_lock()) { goto quit; } @@ -1106,9 +1145,7 @@ restart: } sock_info->socket = ret; - ust_lock(); - - if (lttng_ust_comm_should_quit) { + if (ust_lock()) { goto quit; } @@ -1149,9 +1186,7 @@ restart: DBG("Info: sessiond not accepting connections to %s apps socket", sock_info->name); prev_connect_failed = 1; - ust_lock(); - - if (lttng_ust_comm_should_quit) { + if (ust_lock()) { goto quit; } @@ -1188,9 +1223,7 @@ restart: WARN("Unsupported timeout value %ld", timeout); } - ust_lock(); - - if (lttng_ust_comm_should_quit) { + if (ust_lock()) { goto quit; } @@ -1221,8 +1254,7 @@ restart: switch (len) { case 0: /* orderly shutdown */ DBG("%s lttng-sessiond has performed an orderly shutdown", sock_info->name); - ust_lock(); - if (lttng_ust_comm_should_quit) { + if (ust_lock()) { goto quit; } /* @@ -1261,8 +1293,7 @@ restart: } end: - ust_lock(); - if (lttng_ust_comm_should_quit) { + if (ust_lock()) { goto quit; } /* Cleanup socket handles before trying to reconnect */ @@ -1271,8 +1302,11 @@ end: goto restart; /* try to reconnect */ quit: - sock_info->thread_active = 0; ust_unlock(); + + pthread_mutex_lock(&ust_exit_mutex); + sock_info->thread_active = 0; + pthread_mutex_unlock(&ust_exit_mutex); return NULL; } @@ -1346,7 +1380,7 @@ void __attribute__((constructor)) lttng_ust_init(void) ERR("pthread_attr_setdetachstate: %s", strerror(ret)); } - ust_lock(); + ust_lock_nocheck(); ret = pthread_create(&global_apps.ust_listener, &thread_attr, ust_listener_thread, &global_apps); if (ret) { @@ -1356,7 +1390,7 @@ void __attribute__((constructor)) lttng_ust_init(void) ust_unlock(); if (local_apps.allowed) { - ust_lock(); + ust_lock_nocheck(); ret = pthread_create(&local_apps.ust_listener, &thread_attr, ust_listener_thread, &local_apps); if (ret) { @@ -1447,9 +1481,11 @@ void __attribute__((destructor)) lttng_ust_exit(void) * mutexes to ensure it is not in a mutex critical section when * pthread_cancel is later called. */ - ust_lock(); + ust_lock_nocheck(); lttng_ust_comm_should_quit = 1; + ust_unlock(); + pthread_mutex_lock(&ust_exit_mutex); /* cancel threads */ if (global_apps.thread_active) { ret = pthread_cancel(global_apps.ust_listener); @@ -1469,7 +1505,7 @@ void __attribute__((destructor)) lttng_ust_exit(void) local_apps.thread_active = 0; } } - ust_unlock(); + pthread_mutex_unlock(&ust_exit_mutex); /* * Do NOT join threads: use of sys_futex makes it impossible to @@ -1508,7 +1544,7 @@ void ust_before_fork(sigset_t *save_sigset) if (ret == -1) { PERROR("sigprocmask"); } - ust_lock(); + ust_lock_nocheck(); rcu_bp_before_fork(); }