From 37dddb65504eff070a64fb4a8f1c56ee81c3173c Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Sat, 30 Nov 2013 15:42:50 +0100 Subject: [PATCH] Fix: take the ust lock around session iteration in statedump This is crafted _very_ carefully: we need to always take the ust lock _within_ the dynamic loader lock. Signed-off-by: Mathieu Desnoyers --- include/lttng/ust-events.h | 3 +- liblttng-ust/lttng-events.c | 27 ++++++++++++++---- liblttng-ust/lttng-ust-baddr.c | 36 ++++++++++++++++++------ liblttng-ust/lttng-ust-baddr.h | 2 +- liblttng-ust/lttng-ust-comm.c | 50 +++++++++++++++++++++++----------- 5 files changed, 85 insertions(+), 33 deletions(-) diff --git a/include/lttng/ust-events.h b/include/lttng/ust-events.h index 28f7391b..d93c9121 100644 --- a/include/lttng/ust-events.h +++ b/include/lttng/ust-events.h @@ -610,6 +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_statedumps(t_statedump_func_ptr statedump_func_ptr); +int 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 9380e9ce..6e16cf25 100644 --- a/liblttng-ust/lttng-events.c +++ b/liblttng-ust/lttng-events.c @@ -54,6 +54,7 @@ #include "tracepoint-internal.h" #include "lttng-tracer.h" #include "lttng-tracer-core.h" +#include "lttng-ust-baddr.h" #include "wait.h" #include "../libringbuffer/shm.h" #include "jhash.h" @@ -77,6 +78,11 @@ void ust_unlock(void) static CDS_LIST_HEAD(sessions); +struct cds_list_head *_lttng_get_sessions(void) +{ + return &sessions; +} + static void _lttng_event_destroy(struct lttng_event *event); static @@ -676,18 +682,27 @@ int lttng_fix_pending_events(void) } /* - * Called after session enable: For each session, execute pending statedumps. + * For each session of the owner thread, execute pending statedump. + * 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_statedumps(t_statedump_func_ptr statedump_func_ptr) +int lttng_handle_pending_statedump(void *owner) { struct lttng_session *session; + /* Execute state dump */ + lttng_ust_baddr_statedump(owner); + + /* Clear pending state dump */ + ust_lock(); cds_list_for_each_entry(session, &sessions, node) { - if (session->statedump_pending) { - session->statedump_pending = 0; - statedump_func_ptr(session); - } + if (session->owner != owner) + continue; + if (!session->statedump_pending) + continue; + session->statedump_pending = 0; } + ust_unlock(); return 0; } diff --git a/liblttng-ust/lttng-ust-baddr.c b/liblttng-ust/lttng-ust-baddr.c index 1e6e08dc..001103a2 100644 --- a/liblttng-ust/lttng-ust-baddr.c +++ b/liblttng-ust/lttng-ust-baddr.c @@ -36,16 +36,21 @@ #define TRACEPOINT_DEFINE #include "ust_baddr_statedump.h" -static int -extract_soinfo_events(struct dl_phdr_info *info, size_t size, void *data) +static +int extract_soinfo_events(struct dl_phdr_info *info, size_t size, void *data) { int j; int num_loadable_segment = 0; + void *owner = data; + struct cds_list_head *sessionsp; + + sessionsp = _lttng_get_sessions(); for (j = 0; j < info->dlpi_phnum; j++) { char resolved_path[PATH_MAX]; struct stat sostat; void *base_addr_ptr; + struct lttng_session *session; if (info->dlpi_phdr[j].p_type != PT_LOAD) continue; @@ -83,9 +88,22 @@ extract_soinfo_events(struct dl_phdr_info *info, size_t size, void *data) sostat.st_mtime = -1; } - tracepoint(ust_baddr_statedump, soinfo, - (struct lttng_session *) data, base_addr_ptr, - resolved_path, sostat.st_size, sostat.st_mtime); + /* + * UST lock needs to be nested within dynamic loader + * lock. + */ + ust_lock(); + cds_list_for_each_entry(session, sessionsp, node) { + if (session->owner != owner) + continue; + if (!session->statedump_pending) + continue; + tracepoint(ust_baddr_statedump, soinfo, + session, base_addr_ptr, + resolved_path, sostat.st_size, + sostat.st_mtime); + } + ust_unlock(); /* * We are only interested in the base address (lowest virtual @@ -96,15 +114,15 @@ extract_soinfo_events(struct dl_phdr_info *info, size_t size, void *data) return 0; } -int -lttng_ust_baddr_statedump(struct lttng_session *session) +int lttng_ust_baddr_statedump(void *owner) { if (getenv("LTTNG_UST_WITHOUT_BADDR_STATEDUMP")) return 0; /* * Iterate through the list of currently loaded shared objects and - * generate events for loadable segments using extract_soinfo_events + * generate events for loadable segments using + * extract_soinfo_events. */ - dl_iterate_phdr(extract_soinfo_events, session); + dl_iterate_phdr(extract_soinfo_events, owner); return 0; } diff --git a/liblttng-ust/lttng-ust-baddr.h b/liblttng-ust/lttng-ust-baddr.h index d338541a..08f7db7d 100644 --- a/liblttng-ust/lttng-ust-baddr.h +++ b/liblttng-ust/lttng-ust-baddr.h @@ -21,6 +21,6 @@ #include -int lttng_ust_baddr_statedump(struct lttng_session *session); +int lttng_ust_baddr_statedump(void *owner); #endif /* LTTNG_UST_BADDR_H */ diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c index 4724fabc..256cbdc6 100644 --- a/liblttng-ust/lttng-ust-comm.c +++ b/liblttng-ust/lttng-ust-comm.c @@ -108,7 +108,8 @@ struct sock_info { char wait_shm_path[PATH_MAX]; char *wait_shm_mmap; - int session_enabled; + /* Keep track of lazy state dump not performed yet. */ + int statedump_pending; }; /* Socket from app (connect) to session daemon (listen) for communication */ @@ -126,7 +127,7 @@ struct sock_info global_apps = { .wait_shm_path = "/" LTTNG_UST_WAIT_FILENAME, - .session_enabled = 0, + .statedump_pending = 0, }; /* TODO: allow global_apps_sock_path override */ @@ -141,7 +142,7 @@ struct sock_info local_apps = { .socket = -1, .notify_socket = -1, - .session_enabled = 0, + .statedump_pending = 0, }; static int wait_poll_fallback; @@ -387,6 +388,28 @@ int handle_register_done(struct sock_info *sock_info) return 0; } +/* + * Only execute pending statedump after the constructor semaphore has + * been posted by each listener thread. This means statedump will only + * be performed after the "registration done" command is received from + * each session daemon the application is connected to. + * + * This ensures we don't run into deadlock issues with the dynamic + * loader mutex, which is held while the constructor is called and + * waiting on the constructor semaphore. All operations requiring this + * dynamic loader lock need to be postponed using this mechanism. + */ +static +void handle_pending_statedump(struct sock_info *sock_info) +{ + int ctor_passed = sock_info->constructor_sem_posted; + + if (ctor_passed && sock_info->statedump_pending) { + sock_info->statedump_pending = 0; + lttng_handle_pending_statedump(sock_info); + } +} + static int handle_message(struct sock_info *sock_info, int sock, struct ustcomm_ust_msg *lum) @@ -705,18 +728,15 @@ end: error: ust_unlock(); - return ret; -} -static -void handle_pending_statedumps(struct sock_info *sock_info) -{ - int ctor_passed = sock_info->constructor_sem_posted; + /* + * Performed delayed statedump operations outside of the UST + * lock. We need to take the dynamic loader lock before we take + * the UST lock internally within handle_pending_statedump(). + */ + handle_pending_statedump(sock_info); - if (ctor_passed && sock_info->session_enabled) { - sock_info->session_enabled = 0; - lttng_handle_pending_statedumps(<tng_ust_baddr_statedump); - } + return ret; } static @@ -1224,8 +1244,6 @@ restart: ret = handle_message(sock_info, sock, &lum); if (ret) { ERR("Error handling message for %s socket", sock_info->name); - } else { - handle_pending_statedumps(sock_info); } continue; default: @@ -1543,5 +1561,5 @@ void ust_after_fork_child(sigset_t *restore_sigset) void lttng_ust_sockinfo_session_enabled(void *owner) { struct sock_info *sock_info = owner; - sock_info->session_enabled = 1; + sock_info->statedump_pending = 1; } -- 2.34.1