From 17dfb34b2e6613f3a6d2f084e2d78fe3c301ad98 Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Fri, 26 Aug 2011 15:46:41 -0400 Subject: [PATCH] Centralize locking with ust_lock, fork handling A single lock is held across forks (ust_lock), which synchronizes _all_ operations performed on UST structures. - cleanup/reinit UST after fork in child. - ensure to unlock URCU-bp mutex before calling synchronize_rcu again. Signed-off-by: Mathieu Desnoyers --- libust/ltt-events.c | 30 ++++------------ libust/ltt-probes.c | 10 +++--- libust/ltt-tracer-core.h | 4 +-- libust/lttng-ust-abi.c | 1 + libust/lttng-ust-comm.c | 78 ++++++++++++++++++++++++++-------------- libust/tracepoint.c | 73 ++++++++++++------------------------- 6 files changed, 89 insertions(+), 107 deletions(-) diff --git a/libust/ltt-events.c b/libust/ltt-events.c index 0c6ef37..325fe1d 100644 --- a/libust/ltt-events.c +++ b/libust/ltt-events.c @@ -34,16 +34,17 @@ typedef u32 uint32_t; /* * The sessions mutex is the centralized mutex across UST tracing - * control and probe registration. + * control and probe registration. All operations within this file are + * called by the communication thread, under ust_lock protection. */ static DEFINE_MUTEX(sessions_mutex); -void lock_ust(void) +void ust_lock(void) { pthread_mutex_lock(&sessions_mutex); } -void unlock_ust(void) +void ust_unlock(void) { pthread_mutex_unlock(&sessions_mutex); } @@ -162,12 +163,10 @@ struct ltt_session *ltt_session_create(void) session = zmalloc(sizeof(struct ltt_session)); if (!session) return NULL; - pthread_mutex_lock(&sessions_mutex); CDS_INIT_LIST_HEAD(&session->chan); CDS_INIT_LIST_HEAD(&session->events); uuid_generate(session->uuid); cds_list_add(&session->list, &sessions); - pthread_mutex_unlock(&sessions_mutex); return session; } @@ -177,7 +176,6 @@ void ltt_session_destroy(struct ltt_session *session) struct ltt_event *event, *tmpevent; int ret; - pthread_mutex_lock(&sessions_mutex); CMM_ACCESS_ONCE(session->active) = 0; cds_list_for_each_entry(event, &session->events, list) { ret = _ltt_event_unregister(event); @@ -189,7 +187,6 @@ void ltt_session_destroy(struct ltt_session *session) cds_list_for_each_entry_safe(chan, tmpchan, &session->chan, list) _ltt_channel_destroy(chan); cds_list_del(&session->list); - pthread_mutex_unlock(&sessions_mutex); free(session); } @@ -198,7 +195,6 @@ int ltt_session_enable(struct ltt_session *session) int ret = 0; struct ltt_channel *chan; - pthread_mutex_lock(&sessions_mutex); if (session->active) { ret = -EBUSY; goto end; @@ -223,7 +219,6 @@ int ltt_session_enable(struct ltt_session *session) if (ret) CMM_ACCESS_ONCE(session->active) = 0; end: - pthread_mutex_unlock(&sessions_mutex); return ret; } @@ -231,14 +226,12 @@ int ltt_session_disable(struct ltt_session *session) { int ret = 0; - pthread_mutex_lock(&sessions_mutex); if (!session->active) { ret = -EBUSY; goto end; } CMM_ACCESS_ONCE(session->active) = 0; end: - pthread_mutex_unlock(&sessions_mutex); return ret; } @@ -311,7 +304,6 @@ struct ltt_channel *ltt_channel_create(struct ltt_session *session, struct ltt_channel *chan; struct ltt_transport *transport; - pthread_mutex_lock(&sessions_mutex); if (session->been_active) goto active; /* Refuse to add channel to active session */ transport = ltt_transport_find(transport_name); @@ -338,7 +330,6 @@ struct ltt_channel *ltt_channel_create(struct ltt_session *session, chan->enabled = 1; chan->ops = &transport->ops; cds_list_add(&chan->list, &session->chan); - pthread_mutex_unlock(&sessions_mutex); return chan; create_error: @@ -346,7 +337,6 @@ create_error: nomem: notransport: active: - pthread_mutex_unlock(&sessions_mutex); return NULL; } @@ -372,7 +362,6 @@ struct ltt_event *ltt_event_create(struct ltt_channel *chan, struct ltt_event *event; int ret; - pthread_mutex_lock(&sessions_mutex); if (chan->used_event_id == -1UL) goto full; /* @@ -426,7 +415,6 @@ struct ltt_event *ltt_event_create(struct ltt_channel *chan, goto statedump_error; } cds_list_add(&event->list, &chan->session->events); - pthread_mutex_unlock(&sessions_mutex); return event; statedump_error: @@ -442,7 +430,6 @@ register_error: cache_error: exist: full: - pthread_mutex_unlock(&sessions_mutex); return NULL; } @@ -494,7 +481,7 @@ void _ltt_event_destroy(struct ltt_event *event) /* * We have exclusive access to our metadata buffer (protected by the - * sessions_mutex), so we can do racy operations such as looking for + * ust_lock), so we can do racy operations such as looking for * remaining space left in packet and write, since mutual exclusion * protects us from concurrent writes. */ @@ -990,24 +977,21 @@ end: * @transport: transport structure * * Registers a transport which can be used as output to extract the data out of - * LTTng. + * LTTng. Called with ust_lock held. */ void ltt_transport_register(struct ltt_transport *transport) { - pthread_mutex_lock(&sessions_mutex); cds_list_add_tail(&transport->node, <t_transport_list); - pthread_mutex_unlock(&sessions_mutex); } /** * ltt_transport_unregister - LTT transport unregistration * @transport: transport structure + * Called with ust_lock held. */ void ltt_transport_unregister(struct ltt_transport *transport) { - pthread_mutex_lock(&sessions_mutex); cds_list_del(&transport->node); - pthread_mutex_unlock(&sessions_mutex); } void ltt_events_exit(void) diff --git a/libust/ltt-probes.c b/libust/ltt-probes.c index 76156f0..04392d8 100644 --- a/libust/ltt-probes.c +++ b/libust/ltt-probes.c @@ -17,7 +17,7 @@ #include "ltt-tracer-core.h" /* - * probe list is protected by lock_ust()/unlock_ust(). + * probe list is protected by ust_lock()/ust_unlock(). */ static CDS_LIST_HEAD(probe_list); @@ -41,7 +41,7 @@ int ltt_probe_register(struct lttng_probe_desc *desc) int ret = 0; int i; - lock_ust(); + ust_lock(); /* * TODO: This is O(N^2). Turn into a hash table when probe registration * overhead becomes an issue. @@ -62,15 +62,15 @@ int ltt_probe_register(struct lttng_probe_desc *desc) assert(!ret); } end: - unlock_ust(); + ust_unlock(); return ret; } void ltt_probe_unregister(struct lttng_probe_desc *desc) { - lock_ust(); + ust_lock(); cds_list_del(&desc->head); - unlock_ust(); + ust_unlock(); } /* diff --git a/libust/ltt-tracer-core.h b/libust/ltt-tracer-core.h index 1a32978..fe2ecb9 100644 --- a/libust/ltt-tracer-core.h +++ b/libust/ltt-tracer-core.h @@ -34,7 +34,7 @@ struct ltt_session; struct ltt_channel; struct ltt_event; -void lock_ust(void); -void unlock_ust(void); +void ust_lock(void); +void ust_unlock(void); #endif /* _LTT_TRACER_CORE_H */ diff --git a/libust/lttng-ust-abi.c b/libust/lttng-ust-abi.c index 8afac10..53dd9a2 100644 --- a/libust/lttng-ust-abi.c +++ b/libust/lttng-ust-abi.c @@ -188,6 +188,7 @@ void objd_table_destroy(void) ops->release(i); } free(objd_table.array); + objd_table.freelist_head = -1; } /* diff --git a/libust/lttng-ust-comm.c b/libust/lttng-ust-comm.c index 1e4031b..b6c5de5 100644 --- a/libust/lttng-ust-comm.c +++ b/libust/lttng-ust-comm.c @@ -44,10 +44,10 @@ static int initialized; /* - * 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/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. */ -static pthread_mutex_t lttng_ust_comm_mutex = PTHREAD_MUTEX_INITIALIZER; /* Should the ust comm thread quit ? */ static int lttng_ust_comm_should_quit; @@ -207,7 +207,7 @@ int handle_message(struct sock_info *sock_info, const struct objd_ops *ops; struct lttcomm_ust_reply lur; - pthread_mutex_lock(<tng_ust_comm_mutex); + ust_lock(); memset(&lur, 0, sizeof(lur)); @@ -255,7 +255,7 @@ end: } ret = send_reply(sock, &lur); - pthread_mutex_unlock(<tng_ust_comm_mutex); + ust_unlock(); return ret; } @@ -295,10 +295,10 @@ void *ust_listener_thread(void *arg) /* Restart trying to connect to the session daemon */ restart: - pthread_mutex_lock(<tng_ust_comm_mutex); + ust_lock(); if (lttng_ust_comm_should_quit) { - pthread_mutex_unlock(<tng_ust_comm_mutex); + ust_unlock(); goto quit; } @@ -322,7 +322,7 @@ restart: */ ret = handle_register_done(sock_info); assert(!ret); - pthread_mutex_unlock(<tng_ust_comm_mutex); + ust_unlock(); sleep(5); goto restart; } @@ -337,7 +337,7 @@ restart: ret = lttng_abi_create_root_handle(); if (ret) { ERR("Error creating root handle"); - pthread_mutex_unlock(<tng_ust_comm_mutex); + ust_unlock(); goto quit; } sock_info->root_handle = ret; @@ -352,11 +352,11 @@ restart: */ ret = handle_register_done(sock_info); assert(!ret); - pthread_mutex_unlock(<tng_ust_comm_mutex); + ust_unlock(); sleep(5); goto restart; } - pthread_mutex_unlock(<tng_ust_comm_mutex); + ust_unlock(); for (;;) { ssize_t len; @@ -501,6 +501,27 @@ void __attribute__((constructor)) lttng_ust_init(void) } } +static +void lttng_ust_cleanup(int exiting) +{ + cleanup_sock_info(&global_apps); + if (local_apps.allowed) { + cleanup_sock_info(&local_apps); + } + lttng_ust_abi_exit(); + ltt_events_exit(); + ltt_ring_buffer_client_discard_exit(); + ltt_ring_buffer_client_overwrite_exit(); + ltt_ring_buffer_metadata_client_exit(); + exit_tracepoint(); + if (!exiting) { + /* Reinitialize values for fork */ + sem_count = 2; + lttng_ust_comm_should_quit = 0; + initialized = 0; + } +} + void __attribute__((destructor)) lttng_ust_exit(void) { int ret; @@ -516,32 +537,21 @@ void __attribute__((destructor)) lttng_ust_exit(void) * mutexes to ensure it is not in a mutex critical section when * pthread_cancel is later called. */ - pthread_mutex_lock(<tng_ust_comm_mutex); + ust_lock(); lttng_ust_comm_should_quit = 1; - pthread_mutex_unlock(<tng_ust_comm_mutex); + ust_unlock(); ret = pthread_cancel(global_apps.ust_listener); if (ret) { ERR("Error cancelling global ust listener thread"); } - - cleanup_sock_info(&global_apps); - if (local_apps.allowed) { ret = pthread_cancel(local_apps.ust_listener); if (ret) { ERR("Error cancelling local ust listener thread"); } - - cleanup_sock_info(&local_apps); } - - lttng_ust_abi_exit(); - ltt_events_exit(); - ltt_ring_buffer_client_discard_exit(); - ltt_ring_buffer_client_overwrite_exit(); - ltt_ring_buffer_metadata_client_exit(); - exit_tracepoint(); + lttng_ust_cleanup(1); } /* @@ -568,7 +578,7 @@ void ust_before_fork(ust_fork_info_t *fork_info) if (ret == -1) { PERROR("sigprocmask"); } - pthread_mutex_lock(<tng_ust_comm_mutex); + ust_lock(); rcu_bp_before_fork(); } @@ -576,7 +586,8 @@ static void ust_after_fork_common(ust_fork_info_t *fork_info) { int ret; - pthread_mutex_unlock(<tng_ust_comm_mutex); + DBG("process %d", getpid()); + ust_unlock(); /* Restore signals */ ret = sigprocmask(SIG_SETMASK, &fork_info->orig_sigs, NULL); if (ret == -1) { @@ -586,15 +597,28 @@ static void ust_after_fork_common(ust_fork_info_t *fork_info) void ust_after_fork_parent(ust_fork_info_t *fork_info) { + DBG("process %d", getpid()); rcu_bp_after_fork_parent(); /* Release mutexes and reenable signals */ ust_after_fork_common(fork_info); } +/* + * After fork, in the child, we need to cleanup all the leftover state, + * except the worker thread which already magically disappeared thanks + * to the weird Linux fork semantics. After tyding up, we call + * lttng_ust_init() again to start over as a new PID. + * + * This is meant for forks() that have tracing in the child between the + * fork and following exec call (if there is any). + */ void ust_after_fork_child(ust_fork_info_t *fork_info) { + DBG("process %d", getpid()); /* Release urcu mutexes */ rcu_bp_after_fork_child(); + lttng_ust_cleanup(0); + lttng_ust_init(); /* Release mutexes and reenable signals */ ust_after_fork_common(fork_info); } diff --git a/libust/tracepoint.c b/libust/tracepoint.c index a006f70..c512199 100644 --- a/libust/tracepoint.c +++ b/libust/tracepoint.c @@ -40,29 +40,12 @@ static void (*new_tracepoint_cb)(struct tracepoint *); static CDS_LIST_HEAD(libs); /* - * Allow nested mutex for mutex listing and nested enable. + * The UST lock protects the library tracepoints, the hash table, and + * the library list. + * All calls to the tracepoint API must be protected by the UST lock, + * excepts calls to tracepoint_register_lib and + * tracepoint_unregister_lib, which take the UST lock themselves. */ -static __thread int nested_mutex; - -/* - * Tracepoints mutex protects the library tracepoints, the hash table, - * and the library list. - */ -static pthread_mutex_t tracepoints_mutex; - -static -void lock_tracepoints(void) -{ - if (!(nested_mutex++)) - pthread_mutex_lock(&tracepoints_mutex); -} - -static -void unlock_tracepoints(void) -{ - if (!(--nested_mutex)) - pthread_mutex_unlock(&tracepoints_mutex); -} /* * Tracepoint hash table, containing the active tracepoints. @@ -257,7 +240,7 @@ static struct tracepoint_entry *add_tracepoint(const char *name) /* * Remove the tracepoint from the tracepoint hash table. Must be called with - * mutex_lock held. + * ust_lock held. */ static inline void remove_tracepoint(struct tracepoint_entry *e) { @@ -331,12 +314,10 @@ static void lib_update_tracepoints(void) { struct tracepoint_lib *lib; - lock_tracepoints(); cds_list_for_each_entry(lib, &libs, list) { tracepoint_update_probe_range(lib->tracepoints_start, lib->tracepoints_start + lib->tracepoints_count); } - unlock_tracepoints(); } /* @@ -373,14 +354,13 @@ tracepoint_add_probe(const char *name, void *probe, void *data) * * Returns 0 if ok, error value on error. * The probe address must at least be aligned on the architecture pointer size. + * Called with the UST lock held. */ int __tracepoint_probe_register(const char *name, void *probe, void *data) { void *old; - lock_tracepoints(); old = tracepoint_add_probe(name, probe, data); - unlock_tracepoints(); if (IS_ERR(old)) return PTR_ERR(old); @@ -411,18 +391,13 @@ static void *tracepoint_remove_probe(const char *name, void *probe, void *data) * @probe: probe function pointer * @probe: probe data pointer * - * We do not need to call a synchronize_sched to make sure the probes have - * finished running before doing a module unload, because the module unload - * itself uses stop_machine(), which insures that every preempt disabled section - * have finished. + * Called with the UST lock held. */ int __tracepoint_probe_unregister(const char *name, void *probe, void *data) { void *old; - lock_tracepoints(); old = tracepoint_remove_probe(name, probe, data); - unlock_tracepoints(); if (IS_ERR(old)) return PTR_ERR(old); @@ -447,20 +422,18 @@ static void tracepoint_add_old_probes(void *old) * @probe: probe handler * * caller must call tracepoint_probe_update_all() + * Called with the UST lock held. */ int tracepoint_probe_register_noupdate(const char *name, void *probe, void *data) { void *old; - lock_tracepoints(); old = tracepoint_add_probe(name, probe, data); if (IS_ERR(old)) { - unlock_tracepoints(); return PTR_ERR(old); } tracepoint_add_old_probes(old); - unlock_tracepoints(); return 0; } @@ -470,40 +443,36 @@ int tracepoint_probe_register_noupdate(const char *name, void *probe, * @probe: probe function pointer * * caller must call tracepoint_probe_update_all() + * Called with the UST lock held. */ int tracepoint_probe_unregister_noupdate(const char *name, void *probe, void *data) { void *old; - lock_tracepoints(); old = tracepoint_remove_probe(name, probe, data); if (IS_ERR(old)) { - unlock_tracepoints(); return PTR_ERR(old); } tracepoint_add_old_probes(old); - unlock_tracepoints(); return 0; } /** * tracepoint_probe_update_all - update tracepoints + * Called with the UST lock held. */ void tracepoint_probe_update_all(void) { CDS_LIST_HEAD(release_probes); struct tp_probes *pos, *next; - lock_tracepoints(); if (!need_update) { - unlock_tracepoints(); return; } if (!cds_list_empty(&old_probes)) cds_list_replace_init(&old_probes, &release_probes); need_update = 0; - unlock_tracepoints(); tracepoint_update_probes(); cds_list_for_each_entry_safe(pos, next, &release_probes, u.list) { @@ -578,14 +547,16 @@ static void tracepoint_get_iter(struct tracepoint_iter *iter) tracepoint_iter_reset(iter); } +/* + * Called with UST lock held. + */ void tracepoint_iter_start(struct tracepoint_iter *iter) { - lock_tracepoints(); tracepoint_get_iter(iter); } /* - * Called with tracepoint mutex held. + * Called with UST lock held. */ void tracepoint_iter_next(struct tracepoint_iter *iter) { @@ -598,9 +569,11 @@ void tracepoint_iter_next(struct tracepoint_iter *iter) tracepoint_get_iter(iter); } +/* + * Called with UST lock held. + */ void tracepoint_iter_stop(struct tracepoint_iter *iter) { - unlock_tracepoints(); } void tracepoint_iter_reset(struct tracepoint_iter *iter) @@ -637,7 +610,7 @@ int tracepoint_register_lib(struct tracepoint * const *tracepoints_start, pl->tracepoints_start = tracepoints_start; pl->tracepoints_count = tracepoints_count; - lock_tracepoints(); + ust_lock(); /* * We sort the libs by struct lib pointer address. */ @@ -652,12 +625,11 @@ int tracepoint_register_lib(struct tracepoint * const *tracepoints_start, /* We should be added at the head of the list */ cds_list_add(&pl->list, &libs); lib_added: - unlock_tracepoints(); - new_tracepoints(tracepoints_start, tracepoints_start + tracepoints_count); /* TODO: update just the loaded lib */ lib_update_tracepoints(); + ust_unlock(); DBG("just registered a tracepoints section from %p and having %d tracepoints", tracepoints_start, tracepoints_count); @@ -669,7 +641,7 @@ int tracepoint_unregister_lib(struct tracepoint * const *tracepoints_start) { struct tracepoint_lib *lib; - lock_tracepoints(); + ust_lock(); cds_list_for_each_entry(lib, &libs, list) { if (lib->tracepoints_start == tracepoints_start) { struct tracepoint_lib *lib2free = lib; @@ -678,7 +650,7 @@ int tracepoint_unregister_lib(struct tracepoint * const *tracepoints_start) break; } } - unlock_tracepoints(); + ust_unlock(); return 0; } @@ -692,4 +664,5 @@ void init_tracepoint(void) void exit_tracepoint(void) { + initialized = 0; } -- 2.34.1