From: Mathieu Desnoyers Date: Tue, 30 Apr 2013 03:03:44 +0000 (-0400) Subject: Fix RCU-related hangs: incorrect lttng_ht_destroy use X-Git-Tag: v2.2.0-rc2~25 X-Git-Url: https://git.lttng.org/?p=lttng-tools.git;a=commitdiff_plain;h=36b588eddce05ef840bd247f6a58316925b9a0a2;hp=3a1aff7a271016fa0104e45492ca94e7e06b2492 Fix RCU-related hangs: incorrect lttng_ht_destroy use lttng_ht_destroy() should *NOT* be called with rcu_read_lock() held. It can cause hangs. Signed-off-by: Mathieu Desnoyers --- diff --git a/src/bin/lttng-sessiond/buffer-registry.c b/src/bin/lttng-sessiond/buffer-registry.c index 5751b8d58..2df98d736 100644 --- a/src/bin/lttng-sessiond/buffer-registry.c +++ b/src/bin/lttng-sessiond/buffer-registry.c @@ -527,8 +527,10 @@ void buffer_reg_channel_destroy(struct buffer_reg_channel *regp, /* * Destroy a buffer registry session with the given domain. + * + * Should *NOT* be called with RCU read-side lock held. */ -void buffer_reg_session_destroy(struct buffer_reg_session *regp, +static void buffer_reg_session_destroy(struct buffer_reg_session *regp, enum lttng_domain_type domain) { int ret; @@ -545,9 +547,10 @@ void buffer_reg_session_destroy(struct buffer_reg_session *regp, assert(!ret); buffer_reg_channel_destroy(reg_chan, domain); } - lttng_ht_destroy(regp->channels); rcu_read_unlock(); + lttng_ht_destroy(regp->channels); + switch (domain) { case LTTNG_DOMAIN_UST: ust_registry_session_destroy(regp->reg.ust); @@ -562,8 +565,7 @@ void buffer_reg_session_destroy(struct buffer_reg_session *regp, } /* - * Remove buffer registry UID object from the global hash table. RCU read side - * lock MUST be acquired before calling this. + * Remove buffer registry UID object from the global hash table. */ void buffer_reg_uid_remove(struct buffer_reg_uid *regp) { @@ -572,9 +574,11 @@ void buffer_reg_uid_remove(struct buffer_reg_uid *regp) assert(regp); + rcu_read_lock(); iter.iter.node = ®p->node.node; ret = lttng_ht_del(buffer_registry_uid, &iter); assert(!ret); + rcu_read_unlock(); } static void rcu_free_buffer_reg_uid(struct rcu_head *head) @@ -620,11 +624,12 @@ void buffer_reg_uid_destroy(struct buffer_reg_uid *regp, goto destroy; } + rcu_read_lock(); /* Get the right socket from the consumer object. */ socket = consumer_find_socket_by_bitness(regp->bits_per_long, consumer); if (!socket) { - goto destroy; + goto unlock; } switch (regp->domain) { @@ -637,9 +642,12 @@ void buffer_reg_uid_destroy(struct buffer_reg_uid *regp, break; default: assert(0); + rcu_read_unlock(); return; } +unlock: + rcu_read_unlock(); destroy: call_rcu(®p->node.head, rcu_free_buffer_reg_uid); } @@ -679,6 +687,8 @@ void buffer_reg_pid_destroy(struct buffer_reg_pid *regp) /* * Destroy per PID and UID registry hash table. + * + * Should *NOT* be called with RCU read-side lock held. */ void buffer_reg_destroy_registries(void) { diff --git a/src/bin/lttng-sessiond/buffer-registry.h b/src/bin/lttng-sessiond/buffer-registry.h index 486b675f4..d77a07187 100644 --- a/src/bin/lttng-sessiond/buffer-registry.h +++ b/src/bin/lttng-sessiond/buffer-registry.h @@ -133,10 +133,6 @@ void buffer_reg_stream_add(struct buffer_reg_stream *stream, void buffer_reg_stream_destroy(struct buffer_reg_stream *regp, enum lttng_domain_type domain); -/* Session */ -void buffer_reg_session_destroy(struct buffer_reg_session *regp, - enum lttng_domain_type domain); - /* Global registry. */ void buffer_reg_destroy_registries(void); diff --git a/src/bin/lttng-sessiond/consumer.c b/src/bin/lttng-sessiond/consumer.c index 7ad749e6d..245fda562 100644 --- a/src/bin/lttng-sessiond/consumer.c +++ b/src/bin/lttng-sessiond/consumer.c @@ -407,6 +407,8 @@ error: /* * Delete the consumer_output object from the list and free the ptr. + * + * Should *NOT* be called with RCU read-side lock held. */ void consumer_destroy_output(struct consumer_output *obj) { @@ -434,6 +436,8 @@ void consumer_destroy_output(struct consumer_output *obj) /* * Copy consumer output and returned the newly allocated copy. + * + * Should *NOT* be called with RCU read-side lock held. */ struct consumer_output *consumer_copy_output(struct consumer_output *obj) { diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c index f6a051aa1..445485b41 100644 --- a/src/bin/lttng-sessiond/main.c +++ b/src/bin/lttng-sessiond/main.c @@ -2159,6 +2159,8 @@ error: * Copy consumer output from the tracing session to the domain session. The * function also applies the right modification on a per domain basis for the * trace files destination directory. + * + * Should *NOT* be called with RCU read-side lock held. */ static int copy_session_consumer(int domain, struct ltt_session *session) { @@ -2216,6 +2218,8 @@ error: /* * Create an UST session and add it to the session ust list. + * + * Should *NOT* be called with RCU read-side lock held. */ static int create_ust_session(struct ltt_session *session, struct lttng_domain *domain) @@ -2341,6 +2345,8 @@ static unsigned int lttng_sessions_count(uid_t uid, gid_t gid) * Return any error encountered or 0 for success. * * "sock" is only used for special-case var. len data. + * + * Should *NOT* be called with RCU read-side lock held. */ static int process_client_msg(struct command_ctx *cmd_ctx, int sock, int *sock_error) diff --git a/src/bin/lttng-sessiond/session.c b/src/bin/lttng-sessiond/session.c index 17dc3544d..b6b24b76c 100644 --- a/src/bin/lttng-sessiond/session.c +++ b/src/bin/lttng-sessiond/session.c @@ -147,6 +147,7 @@ found: * Delete session from the session list and free the memory. * * Return -1 if no session is found. On success, return 1; + * Should *NOT* be called with RCU read-side lock held. */ int session_destroy(struct ltt_session *session) { @@ -157,9 +158,7 @@ int session_destroy(struct ltt_session *session) del_session_list(session); pthread_mutex_destroy(&session->lock); - rcu_read_lock(); consumer_destroy_output(session->consumer); - rcu_read_unlock(); free(session); return LTTNG_OK; diff --git a/src/bin/lttng-sessiond/trace-kernel.c b/src/bin/lttng-sessiond/trace-kernel.c index afb578cfd..d889b6ab6 100644 --- a/src/bin/lttng-sessiond/trace-kernel.c +++ b/src/bin/lttng-sessiond/trace-kernel.c @@ -421,6 +421,8 @@ void trace_kernel_destroy_metadata(struct ltt_kernel_metadata *metadata) /* * Cleanup kernel session structure + * + * Should *NOT* be called with RCU read-side lock held. */ void trace_kernel_destroy_session(struct ltt_kernel_session *session) { diff --git a/src/bin/lttng-sessiond/trace-ust.c b/src/bin/lttng-sessiond/trace-ust.c index 0c97d37f1..7592d7741 100644 --- a/src/bin/lttng-sessiond/trace-ust.c +++ b/src/bin/lttng-sessiond/trace-ust.c @@ -474,12 +474,14 @@ static void destroy_contexts(struct lttng_ht *ht) assert(ht); + rcu_read_lock(); cds_lfht_for_each_entry(ht->ht, &iter.iter, node, node) { ret = lttng_ht_del(ht, &iter); if (!ret) { call_rcu(&node->head, destroy_context_rcu); } } + rcu_read_unlock(); lttng_ht_destroy(ht); } @@ -520,34 +522,34 @@ static void destroy_events(struct lttng_ht *events) assert(events); + rcu_read_lock(); cds_lfht_for_each_entry(events->ht, &iter.iter, node, node) { ret = lttng_ht_del(events, &iter); assert(!ret); call_rcu(&node->head, destroy_event_rcu); } + rcu_read_unlock(); lttng_ht_destroy(events); } /* * Cleanup ust channel structure. + * + * Should _NOT_ be called with RCU read lock held. */ -void trace_ust_destroy_channel(struct ltt_ust_channel *channel) +static void _trace_ust_destroy_channel(struct ltt_ust_channel *channel) { assert(channel); DBG2("Trace destroy UST channel %s", channel->name); - rcu_read_lock(); - /* Destroying all events of the channel */ destroy_events(channel->events); /* Destroying all context of the channel */ destroy_contexts(channel->ctx); free(channel); - - rcu_read_unlock(); } /* @@ -560,7 +562,12 @@ static void destroy_channel_rcu(struct rcu_head *head) struct ltt_ust_channel *channel = caa_container_of(node, struct ltt_ust_channel, node); - trace_ust_destroy_channel(channel); + _trace_ust_destroy_channel(channel); +} + +void trace_ust_destroy_channel(struct ltt_ust_channel *channel) +{ + call_rcu(&channel->node.head, destroy_channel_rcu); } /* @@ -595,10 +602,9 @@ static void destroy_channels(struct lttng_ht *channels) assert(!ret); call_rcu(&node->head, destroy_channel_rcu); } + rcu_read_unlock(); lttng_ht_destroy(channels); - - rcu_read_unlock(); } /* @@ -613,6 +619,8 @@ static void destroy_domain_global(struct ltt_ust_domain_global *dom) /* * Cleanup ust session structure + * + * Should *NOT* be called with RCU read-side lock held. */ void trace_ust_destroy_session(struct ltt_ust_session *session) { @@ -620,8 +628,6 @@ void trace_ust_destroy_session(struct ltt_ust_session *session) assert(session); - rcu_read_lock(); - DBG2("Trace UST destroy session %u", session->id); /* Cleaning up UST domain */ @@ -639,6 +645,4 @@ void trace_ust_destroy_session(struct ltt_ust_session *session) consumer_destroy_output(session->tmp_consumer); free(session); - - rcu_read_unlock(); } diff --git a/src/bin/lttng-sessiond/ust-app.c b/src/bin/lttng-sessiond/ust-app.c index 9bd4e69a2..d8c8017d8 100644 --- a/src/bin/lttng-sessiond/ust-app.c +++ b/src/bin/lttng-sessiond/ust-app.c @@ -305,6 +305,23 @@ void delete_ust_app_stream(int sock, struct ust_app_stream *stream) free(stream); } +/* + * We need to execute ht_destroy outside of RCU read-side critical + * section, so we postpone its execution using call_rcu. It is simpler + * than to change the semantic of the many callers of + * delete_ust_app_channel(). + */ +static +void delete_ust_app_channel_rcu(struct rcu_head *head) +{ + struct ust_app_channel *ua_chan = + caa_container_of(head, struct ust_app_channel, rcu_head); + + lttng_ht_destroy(ua_chan->ctx); + lttng_ht_destroy(ua_chan->events); + free(ua_chan); +} + /* * Delete ust app channel safely. RCU read lock must be held before calling * this function. @@ -336,7 +353,6 @@ void delete_ust_app_channel(int sock, struct ust_app_channel *ua_chan, assert(!ret); delete_ust_app_ctx(sock, ua_ctx); } - lttng_ht_destroy(ua_chan->ctx); /* Wipe events */ cds_lfht_for_each_entry(ua_chan->events->ht, &iter.iter, ua_event, @@ -345,7 +361,6 @@ void delete_ust_app_channel(int sock, struct ust_app_channel *ua_chan, assert(!ret); delete_ust_app_event(sock, ua_event); } - lttng_ht_destroy(ua_chan->events); /* Wipe and free registry from session registry. */ registry = get_session_registry(ua_chan->session); @@ -365,7 +380,7 @@ void delete_ust_app_channel(int sock, struct ust_app_channel *ua_chan, lttng_fd_put(LTTNG_FD_APPS, 1); free(ua_chan->obj); } - free(ua_chan); + call_rcu(&ua_chan->rcu_head, delete_ust_app_channel_rcu); } /* @@ -541,6 +556,22 @@ error: return ret; } +/* + * We need to execute ht_destroy outside of RCU read-side critical + * section, so we postpone its execution using call_rcu. It is simpler + * than to change the semantic of the many callers of + * delete_ust_app_session(). + */ +static +void delete_ust_app_session_rcu(struct rcu_head *head) +{ + struct ust_app_session *ua_sess = + caa_container_of(head, struct ust_app_session, rcu_head); + + lttng_ht_destroy(ua_sess->channels); + free(ua_sess); +} + /* * Delete ust app session safely. RCU read lock must be held before calling * this function. @@ -577,7 +608,6 @@ void delete_ust_app_session(int sock, struct ust_app_session *ua_sess, assert(!ret); delete_ust_app_channel(sock, ua_chan, app); } - lttng_ht_destroy(ua_sess->channels); /* In case of per PID, the registry is kept in the session. */ if (ua_sess->buffer_type == LTTNG_BUFFER_PER_PID) { @@ -595,12 +625,14 @@ void delete_ust_app_session(int sock, struct ust_app_session *ua_sess, sock, ret); } } - free(ua_sess); + call_rcu(&ua_sess->rcu_head, delete_ust_app_session_rcu); } /* * Delete a traceable application structure from the global list. Never call * this function outside of a call_rcu call. + * + * RCU read side lock should _NOT_ be held when calling this function. */ static void delete_ust_app(struct ust_app *app) @@ -608,20 +640,20 @@ void delete_ust_app(struct ust_app *app) int ret, sock; struct ust_app_session *ua_sess, *tmp_ua_sess; - rcu_read_lock(); - /* Delete ust app sessions info */ sock = app->sock; app->sock = -1; - lttng_ht_destroy(app->sessions); - /* Wipe sessions */ cds_list_for_each_entry_safe(ua_sess, tmp_ua_sess, &app->teardown_head, teardown_node) { /* Free every object in the session and the session. */ + rcu_read_lock(); delete_ust_app_session(sock, ua_sess, app); + rcu_read_unlock(); } + + lttng_ht_destroy(app->sessions); lttng_ht_destroy(app->ust_objd); /* @@ -645,8 +677,6 @@ void delete_ust_app(struct ust_app *app) DBG2("UST app pid %d deleted", app->pid); free(app); - - rcu_read_unlock(); } /* @@ -2347,7 +2377,7 @@ error: * Create UST app channel and create it on the tracer. Set ua_chanp of the * newly created channel if not NULL. * - * Called with UST app session lock held. + * Called with UST app session lock and RCU read-side lock held. * * Return 0 on success or else a negative value. */ @@ -3046,6 +3076,8 @@ error: /* * Free and clean all traceable apps of the global list. + * + * Should _NOT_ be called with RCU read-side lock held. */ void ust_app_clean_list(void) { @@ -3076,13 +3108,12 @@ void ust_app_clean_list(void) ret = lttng_ht_del(ust_app_ht_by_notify_sock, &iter); assert(!ret); } + rcu_read_unlock(); /* Destroy is done only when the ht is empty */ lttng_ht_destroy(ust_app_ht); lttng_ht_destroy(ust_app_ht_by_sock); lttng_ht_destroy(ust_app_ht_by_notify_sock); - - rcu_read_unlock(); } /* diff --git a/src/bin/lttng-sessiond/ust-app.h b/src/bin/lttng-sessiond/ust-app.h index 255a75cf7..6e6ff0203 100644 --- a/src/bin/lttng-sessiond/ust-app.h +++ b/src/bin/lttng-sessiond/ust-app.h @@ -154,6 +154,8 @@ struct ust_app_channel { * ust_objd hash table in the ust_app object. */ struct lttng_ht_node_ulong ust_objd_node; + /* For delayed reclaim */ + struct rcu_head rcu_head; }; struct ust_app_session { @@ -193,6 +195,8 @@ struct ust_app_session { enum lttng_buffer_type buffer_type; /* ABI of the session. Same value as the application. */ uint32_t bits_per_long; + /* For delayed reclaim */ + struct rcu_head rcu_head; }; /* diff --git a/src/bin/lttng-sessiond/ust-registry.c b/src/bin/lttng-sessiond/ust-registry.c index b538a8fa9..7250dd12d 100644 --- a/src/bin/lttng-sessiond/ust-registry.c +++ b/src/bin/lttng-sessiond/ust-registry.c @@ -308,12 +308,26 @@ void ust_registry_destroy_event(struct ust_registry_channel *chan, return; } +/* + * We need to execute ht_destroy outside of RCU read-side critical + * section, so we postpone its execution using call_rcu. It is simpler + * than to change the semantic of the many callers of + * destroy_channel(). + */ +static +void destroy_channel_rcu(struct rcu_head *head) +{ + struct ust_registry_channel *chan = + caa_container_of(head, struct ust_registry_channel, rcu_head); + + lttng_ht_destroy(chan->ht); + free(chan); +} + /* * Destroy every element of the registry and free the memory. This does NOT * free the registry pointer since it might not have been allocated before so * it's the caller responsability. - * - * This *MUST NOT* be called within a RCU read side lock section. */ static void destroy_channel(struct ust_registry_channel *chan) { @@ -329,9 +343,7 @@ static void destroy_channel(struct ust_registry_channel *chan) ust_registry_destroy_event(chan, event); } rcu_read_unlock(); - - lttng_ht_destroy(chan->ht); - free(chan); + call_rcu(&chan->rcu_head, destroy_channel_rcu); } /* @@ -435,12 +447,6 @@ void ust_registry_channel_del_free(struct ust_registry_session *session, ret = lttng_ht_del(session->channels, &iter); assert(!ret); rcu_read_unlock(); - - /* - * Destroying the hash table should be done without RCU - * read-side lock held. Since we own "chan" now, it is OK to use - * it outside of RCU read-side critical section. - */ destroy_channel(chan); end: @@ -535,8 +541,8 @@ void ust_registry_session_destroy(struct ust_registry_session *reg) assert(!ret); destroy_channel(chan); } - lttng_ht_destroy(reg->channels); rcu_read_unlock(); + lttng_ht_destroy(reg->channels); free(reg->metadata); } diff --git a/src/bin/lttng-sessiond/ust-registry.h b/src/bin/lttng-sessiond/ust-registry.h index 3cd474b51..3c46984a3 100644 --- a/src/bin/lttng-sessiond/ust-registry.h +++ b/src/bin/lttng-sessiond/ust-registry.h @@ -107,6 +107,8 @@ struct ust_registry_channel { size_t nr_ctx_fields; struct ustctl_field *ctx_fields; struct lttng_ht_node_u64 node; + /* For delayed reclaim */ + struct rcu_head rcu_head; }; /* diff --git a/src/common/consumer.c b/src/common/consumer.c index 343855797..c47c0ff08 100644 --- a/src/common/consumer.c +++ b/src/common/consumer.c @@ -319,9 +319,9 @@ static void cleanup_relayd_ht(void) destroy_relayd(relayd); } - lttng_ht_destroy(consumer_data.relayd_ht); - rcu_read_unlock(); + + lttng_ht_destroy(consumer_data.relayd_ht); } /*