Fix RCU-related hangs: incorrect lttng_ht_destroy use
authorMathieu Desnoyers <mathieu.desnoyers@efficios.com>
Tue, 30 Apr 2013 03:03:44 +0000 (23:03 -0400)
committerMathieu Desnoyers <mathieu.desnoyers@efficios.com>
Tue, 30 Apr 2013 14:28:22 +0000 (10:28 -0400)
lttng_ht_destroy() should *NOT* be called with rcu_read_lock() held. It
can cause hangs.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
12 files changed:
src/bin/lttng-sessiond/buffer-registry.c
src/bin/lttng-sessiond/buffer-registry.h
src/bin/lttng-sessiond/consumer.c
src/bin/lttng-sessiond/main.c
src/bin/lttng-sessiond/session.c
src/bin/lttng-sessiond/trace-kernel.c
src/bin/lttng-sessiond/trace-ust.c
src/bin/lttng-sessiond/ust-app.c
src/bin/lttng-sessiond/ust-app.h
src/bin/lttng-sessiond/ust-registry.c
src/bin/lttng-sessiond/ust-registry.h
src/common/consumer.c

index 5751b8d587f8a1a12a64a1b2fc207aaba22b0534..2df98d7360445b42ae4aba1941cd976d16e6d723 100644 (file)
@@ -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 = &regp->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(&regp->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)
 {
index 486b675f4a4bbfdbe8a636ebab953dceb37f4bf1..d77a07187b8e417359eddbec99a49ea2af0ccf57 100644 (file)
@@ -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);
 
index 7ad749e6d248695d61970ef34f50455afb869b73..245fda5628d42c707ee4097e5757e143ffbfb32a 100644 (file)
@@ -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)
 {
index f6a051aa1cb52f8080cc9b2209a71044f052e860..445485b417d5d0b0e0cd6f1de4eaab41336baaf5 100644 (file)
@@ -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)
index 17dc3544dbe55a4dac884b5af9f178e2f1c44991..b6b24b76c3f9ae28cec31d3c9e3224351647a2ef 100644 (file)
@@ -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;
index afb578cfde886754321d7a1c16c9d9d2ce89ec6b..d889b6ab6582570587c9c4a66cdcf51fad30943a 100644 (file)
@@ -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)
 {
index 0c97d37f1c86883474dc731c7d329638a4d47e8b..7592d774177a43abb8782424084c711e371bcc5c 100644 (file)
@@ -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();
 }
index 9bd4e69a2e031be166da7cac0e81edba38d44147..d8c8017d83800511afa90e7223211a002e7d6481 100644 (file)
@@ -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();
 }
 
 /*
index 255a75cf75ee18f59e599a8d253087946b2bf3ac..6e6ff0203cdb9cc1ddf88a1d626f0cd96db5f033 100644 (file)
@@ -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;
 };
 
 /*
index b538a8fa9f37d6f5108c609a3021a27173cbe9ac..7250dd12d751b0edcce93ffd4b886f8bdd7c7395 100644 (file)
@@ -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);
 }
index 3cd474b51abfebad76d98b6255ff6f736810ed73..3c46984a30f024a27df1d8d04a60d69aa4b44a03 100644 (file)
@@ -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;
 };
 
 /*
index 34385579741f6f0273aee213576bd0a188a95b0e..c47c0ff08239f9c80ccb9a8fae286034def197fc 100644 (file)
@@ -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);
 }
 
 /*
This page took 0.050497 seconds and 4 git commands to generate.