Reference count ltt_session objects

The lifetime of ltt_session objects is mishandled in a number
of situations. For instance, if a rotation is launched on a
session and the session is destroyed during the rotation, it
is not possible for the session daemon to identify the completion
of the rotation. This then prevents the session daemon from
renaming the last chunk of a now-destroyed session and from
notifying clients that a new trace archive chunk is available.

This change only introduces reference counting of the ltt_session
objects; it does not change any behavior with regards to the
lifetime of the sessions themselved. Follow-up commits introduce
those changes.

Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>

diff --git a/src/bin/lttng-sessiond/agent-thread.c b/src/bin/lttng-sessiond/agent-thread.c
index b238656812c69390b2d7e16ee841d7830cd3ba03..51221c4c623fc34a08f1f9c9cb8e90d4177cd8f6 100644 (file)
--- a/src/bin/lttng-sessiond/agent-thread.c
+++ b/src/bin/lttng-sessiond/agent-thread.c
@@ -59,6 +59,10 @@ static void update_agent_app(struct agent_app *app)
 
        session_lock_list();
        cds_list_for_each_entry_safe(session, stmp, &list->head, list) {
+               if (!session_get(session)) {
+                       continue;
+               }
+
                session_lock(session);
                if (session->ust_session) {
                        struct agent *agt;
@@ -71,6 +75,7 @@ static void update_agent_app(struct agent_app *app)
                        rcu_read_unlock();
                }
                session_unlock(session);
+               session_put(session);
        }
        session_unlock_list();
 }

diff --git a/src/bin/lttng-sessiond/cmd.c b/src/bin/lttng-sessiond/cmd.c
index 2aea2ea747d3639dc507144fed844ddf870ed453..def3e64e59a2d27899d1b70dba0f74e74a7b7dea 100644 (file)
--- a/src/bin/lttng-sessiond/cmd.c
+++ b/src/bin/lttng-sessiond/cmd.c
@@ -2910,39 +2910,31 @@ int cmd_create_session_uri(char *name, struct lttng_uri *uris,
                size_t nb_uri, lttng_sock_cred *creds, unsigned int live_timer)
 {
        int ret;
-       struct ltt_session *session;
+       struct ltt_session *session = NULL;
 
        assert(name);
        assert(creds);
 
-       /*
-        * Verify if the session already exist
-        *
-        * XXX: There is no need for the session lock list here since the caller
-        * (process_client_msg) is holding it. We might want to change that so a
-        * single command does not lock the entire session list.
-        */
+       /* Check if the session already exists. */
+       session_lock_list();
        session = session_find_by_name(name);
+       session_unlock_list();
        if (session != NULL) {
                ret = LTTNG_ERR_EXIST_SESS;
-               goto find_error;
+               goto end;
        }
 
        /* Create tracing session in the registry */
        ret = session_create(name, LTTNG_SOCK_GET_UID_CRED(creds),
                        LTTNG_SOCK_GET_GID_CRED(creds));
        if (ret != LTTNG_OK) {
-               goto session_error;
+               goto end;
        }
 
-       /*
-        * Get the newly created session pointer back
-        *
-        * XXX: There is no need for the session lock list here since the caller
-        * (process_client_msg) is holding it. We might want to change that so a
-        * single command does not lock the entire session list.
-        */
+       /* Get the newly created session pointer back. */
+       session_lock_list();
        session = session_find_by_name(name);
+       session_unlock_list();
        assert(session);
 
        session->live_timer = live_timer;
@@ -2950,13 +2942,13 @@ int cmd_create_session_uri(char *name, struct lttng_uri *uris,
        session->consumer = consumer_create_output(CONSUMER_DST_LOCAL);
        if (session->consumer == NULL) {
                ret = LTTNG_ERR_FATAL;
-               goto consumer_error;
+               goto end;
        }
 
        if (uris) {
                ret = cmd_set_consumer_uri(session, nb_uri, uris);
                if (ret != LTTNG_OK) {
-                       goto consumer_error;
+                       goto end;
                }
                session->output_traces = 1;
        } else {
@@ -2966,12 +2958,13 @@ int cmd_create_session_uri(char *name, struct lttng_uri *uris,
 
        session->consumer->enabled = 1;
 
-       return LTTNG_OK;
-
-consumer_error:
-       session_destroy(session);
-session_error:
-find_error:
+       ret = LTTNG_OK;
+end:
+       if (session) {
+               session_lock_list();
+               session_put(session);
+               session_unlock_list();
+       }
        return ret;
 }
 
@@ -2982,7 +2975,7 @@ int cmd_create_session_snapshot(char *name, struct lttng_uri *uris,
                size_t nb_uri, lttng_sock_cred *creds)
 {
        int ret;
-       struct ltt_session *session;
+       struct ltt_session *session = NULL;
        struct snapshot_output *new_output = NULL;
 
        assert(name);
@@ -2994,11 +2987,13 @@ int cmd_create_session_snapshot(char *name, struct lttng_uri *uris,
         */
        ret = cmd_create_session_uri(name, NULL, 0, creds, 0);
        if (ret != LTTNG_OK) {
-               goto error;
+               goto end;
        }
 
        /* Get the newly created session pointer back. This should NEVER fail. */
+       session_lock_list();
        session = session_find_by_name(name);
+       session_unlock_list();
        assert(session);
 
        /* Flag session for snapshot mode. */
@@ -3006,6 +3001,7 @@ int cmd_create_session_snapshot(char *name, struct lttng_uri *uris,
 
        /* Skip snapshot output creation if no URI is given. */
        if (nb_uri == 0) {
+               /* Not an error. */
                goto end;
        }
 
@@ -3030,14 +3026,18 @@ int cmd_create_session_snapshot(char *name, struct lttng_uri *uris,
        snapshot_add_output(&session->snapshot, new_output);
        rcu_read_unlock();
 
-end:
-       return LTTNG_OK;
+       ret = LTTNG_OK;
+       goto end;
 
 error_snapshot:
        snapshot_output_destroy(new_output);
 error_snapshot_alloc:
-       session_destroy(session);
-error:
+end:
+       if (session) {
+               session_lock_list();
+               session_put(session);
+               session_unlock_list();
+       }
        return ret;
 }
 
@@ -3046,19 +3046,14 @@ error:
  *
  * Called with session lock held.
  */
-int cmd_destroy_session(struct ltt_session *session, int wpipe,
+int cmd_destroy_session(struct ltt_session *session,
                struct notification_thread_handle *notification_thread_handle)
 {
        int ret;
-       struct ltt_ust_session *usess;
-       struct ltt_kernel_session *ksess;
 
        /* Safety net */
        assert(session);
 
-       usess = session->ust_session;
-       ksess = session->kernel_session;
-
        DBG("Begin destroy session %s (id %" PRIu64 ")", session->name, session->id);
 
        if (session->rotation_pending_check_timer_enabled) {
@@ -3090,33 +3085,6 @@ int cmd_destroy_session(struct ltt_session *session, int wpipe,
                rename_active_chunk(session);
        }
 
-       /* Clean kernel session teardown */
-       kernel_destroy_session(ksess);
-
-       /* UST session teardown */
-       if (usess) {
-               /* Close any relayd session */
-               consumer_output_send_destroy_relayd(usess->consumer);
-
-               /* Destroy every UST application related to this session. */
-               ret = ust_app_destroy_trace_all(usess);
-               if (ret) {
-                       ERR("Error in ust_app_destroy_trace_all");
-               }
-
-               /* Clean up the rest. */
-               trace_ust_destroy_session(usess);
-       }
-
-       /*
-        * Must notify the kernel thread here to update it's poll set in order to
-        * remove the channel(s)' fd just destroyed.
-        */
-       ret = notify_thread_pipe(wpipe);
-       if (ret < 0) {
-               PERROR("write kernel poll pipe");
-       }
-
        if (session->shm_path[0]) {
                /*
                 * When a session is created with an explicit shm_path,
@@ -3170,7 +3138,14 @@ int cmd_destroy_session(struct ltt_session *session, int wpipe,
                                sizeof(destroy_completion_handler.shm_path));
                assert(!ret);
        }
-       ret = session_destroy(session);
+
+       /*
+        * The session is destroyed. However, note that the command context
+        * still holds a reference to the session, thus delaying its destruction
+        * _at least_ up to the point when that reference is released.
+        */
+       session_destroy(session);
+       ret = LTTNG_OK;
 
        return ret;
 }
@@ -3475,10 +3450,15 @@ void cmd_list_lttng_sessions(struct lttng_session *sessions, uid_t uid,
         * the buffer.
         */
        cds_list_for_each_entry(session, &list->head, list) {
+               if (!session_get(session)) {
+                       continue;
+               }
                /*
                 * Only list the sessions the user can control.
                 */
-               if (!session_access_ok(session, uid, gid)) {
+               if (!session_access_ok(session, uid, gid) ||
+                               session->destroyed) {
+                       session_put(session);
                        continue;
                }
 
@@ -3496,6 +3476,7 @@ void cmd_list_lttng_sessions(struct lttng_session *sessions, uid_t uid,
                }
                if (ret < 0) {
                        PERROR("snprintf session path");
+                       session_put(session);
                        continue;
                }
 
@@ -3505,6 +3486,7 @@ void cmd_list_lttng_sessions(struct lttng_session *sessions, uid_t uid,
                sessions[i].snapshot_mode = session->snapshot_mode;
                sessions[i].live_timer_interval = session->live_timer;
                i++;
+               session_put(session);
        }
 }
 

diff --git a/src/bin/lttng-sessiond/cmd.h b/src/bin/lttng-sessiond/cmd.h
index c0fbee8f0ee2e220f0fb713309297aa7ef7665a5..a207e52b35a8a45e0ac6d7aa1428ce0f22647447 100644 (file)
--- a/src/bin/lttng-sessiond/cmd.h
+++ b/src/bin/lttng-sessiond/cmd.h
@@ -48,7 +48,7 @@ int cmd_create_session_uri(char *name, struct lttng_uri *uris,
                size_t nb_uri, lttng_sock_cred *creds, unsigned int live_timer);
 int cmd_create_session_snapshot(char *name, struct lttng_uri *uris,
                size_t nb_uri, lttng_sock_cred *creds);
-int cmd_destroy_session(struct ltt_session *session, int wpipe,
+int cmd_destroy_session(struct ltt_session *session,
                struct notification_thread_handle *notification_thread_handle);
 
 /* Channel commands */

diff --git a/src/bin/lttng-sessiond/globals.c b/src/bin/lttng-sessiond/globals.c
index fe924747ac66843cf313197fa6ec3559d53c44c8..63fd799386581059d80f4235593f489a1f76f4fc 100644 (file)
--- a/src/bin/lttng-sessiond/globals.c
+++ b/src/bin/lttng-sessiond/globals.c
@@ -33,6 +33,7 @@ struct lttng_ht *agent_apps_ht_by_sock = NULL;
 int kernel_tracer_fd = -1;
 
 int apps_cmd_notify_pipe[2] = { -1, -1 };
+int kernel_poll_pipe[2] = { -1, -1 };
 
 pid_t ppid;
 pid_t child_ppid;

diff --git a/src/bin/lttng-sessiond/kernel-consumer.c b/src/bin/lttng-sessiond/kernel-consumer.c
index 05e447754bb2156a77b6ce3d8dc1555f84973864..9ccad8be4e87dd19cd4e0212acbbd64c4330a4af 100644 (file)
--- a/src/bin/lttng-sessiond/kernel-consumer.c
+++ b/src/bin/lttng-sessiond/kernel-consumer.c
@@ -117,7 +117,7 @@ int kernel_consumer_add_channel(struct consumer_socket *sock,
        struct lttcomm_consumer_msg lkm;
        struct consumer_output *consumer;
        enum lttng_error_code status;
-       struct ltt_session *session;
+       struct ltt_session *session = NULL;
        struct lttng_channel_extended *channel_attr_extended;
 
        /* Safety net */
@@ -191,6 +191,9 @@ int kernel_consumer_add_channel(struct consumer_socket *sock,
        channel->published_to_notification_thread = true;
 
 error:
+       if (session) {
+               session_put(session);
+       }
        free(pathname);
        return ret;
 }
@@ -207,7 +210,7 @@ int kernel_consumer_add_metadata(struct consumer_socket *sock,
        char *pathname;
        struct lttcomm_consumer_msg lkm;
        struct consumer_output *consumer;
-       struct ltt_session *session;
+       struct ltt_session *session = NULL;
 
        rcu_read_lock();
 
@@ -284,6 +287,9 @@ int kernel_consumer_add_metadata(struct consumer_socket *sock,
 error:
        rcu_read_unlock();
        free(pathname);
+       if (session) {
+               session_put(session);
+       }
        return ret;
 }
 
@@ -379,7 +385,7 @@ int kernel_consumer_send_channel_streams(struct consumer_socket *sock,
 {
        int ret = LTTNG_OK;
        struct ltt_kernel_stream *stream;
-       struct ltt_session *session;
+       struct ltt_session *session = NULL;
 
        /* Safety net */
        assert(channel);
@@ -428,6 +434,9 @@ int kernel_consumer_send_channel_streams(struct consumer_socket *sock,
 
 error:
        rcu_read_unlock();
+       if (session) {
+               session_put(session);
+       }
        return ret;
 }
 

diff --git a/src/bin/lttng-sessiond/kernel.c b/src/bin/lttng-sessiond/kernel.c
index edb7f7b2f2f3f83efd53ac077f005b26388d52d9..8e972b0693cc28362fe36e9a80b040d128bed62c 100644 (file)
--- a/src/bin/lttng-sessiond/kernel.c
+++ b/src/bin/lttng-sessiond/kernel.c
@@ -1244,7 +1244,7 @@ enum lttng_error_code kernel_snapshot_record(struct ltt_kernel_session *ksess,
        struct consumer_socket *socket;
        struct lttng_ht_iter iter;
        struct ltt_kernel_metadata *saved_metadata;
-       struct ltt_session *session;
+       struct ltt_session *session = NULL;
        uint64_t trace_archive_id;
 
        assert(ksess);
@@ -1344,7 +1344,9 @@ error:
        /* Restore metadata state.*/
        ksess->metadata = saved_metadata;
        ksess->metadata_stream_fd = saved_metadata_fd;
-
+       if (session) {
+               session_put(session);
+       }
        rcu_read_unlock();
        return status;
 }

diff --git a/src/bin/lttng-sessiond/lttng-sessiond.h b/src/bin/lttng-sessiond/lttng-sessiond.h
index f0bd98781987b96f12b185b4a5cc78037183f09d..25020d87e4e4d6c263d24c4cd1a8ca15525c18ac 100644 (file)
--- a/src/bin/lttng-sessiond/lttng-sessiond.h
+++ b/src/bin/lttng-sessiond/lttng-sessiond.h
@@ -104,6 +104,8 @@ extern int apps_cmd_notify_pipe[2];
  */
 extern int ht_cleanup_pipe[2];
 
+extern int kernel_poll_pipe[2];
+
 /*
  * Populated when the daemon starts with the current page size of the system.
  * Set in main() with the current page size.

diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c
index 7a0cdb81c83ac0c451341aa41ed29c57d4d3cb77..98f58630712ee720cb71225795c09696a9c9d5f9 100644 (file)
--- a/src/bin/lttng-sessiond/main.c
+++ b/src/bin/lttng-sessiond/main.c
@@ -177,7 +177,6 @@ static int dispatch_thread_exit;
 /* Sockets and FDs */
 static int client_sock = -1;
 static int apps_sock = -1;
-static int kernel_poll_pipe[2] = { -1, -1 };
 
 /*
  * This pipe is used to inform the thread managing application communication
@@ -211,18 +210,6 @@ static pthread_t timer_thread;
  */
 static struct ust_cmd_queue ust_cmd_queue;
 
-/*
- * Pointer initialized before thread creation.
- *
- * This points to the tracing session list containing the session count and a
- * mutex lock. The lock MUST be taken if you iterate over the list. The lock
- * MUST NOT be taken if you call a public function in session.c.
- *
- * The lock is nested inside the structure: session_list_ptr->lock. Please use
- * session_lock_list and session_unlock_list for lock acquisition.
- */
-static struct ltt_session_list *session_list_ptr;
-
 static const char *module_proc_lttng = "/proc/lttng";
 
 /*
@@ -389,6 +376,7 @@ static void sessiond_cleanup(void)
 {
        int ret;
        struct ltt_session *sess, *stmp;
+       struct ltt_session_list *session_list = session_get_list();
 
        DBG("Cleanup sessiond");
 
@@ -437,15 +425,19 @@ static void sessiond_cleanup(void)
        DBG("Cleaning up all sessions");
 
        /* Destroy session list mutex */
-       if (session_list_ptr != NULL) {
-               pthread_mutex_destroy(&session_list_ptr->lock);
-
+       if (session_list) {
+               session_lock_list();
                /* Cleanup ALL session */
                cds_list_for_each_entry_safe(sess, stmp,
-                               &session_list_ptr->head, list) {
-                       cmd_destroy_session(sess, kernel_poll_pipe[1],
+                               &session_list->head, list) {
+                       if (sess->destroyed) {
+                               continue;
+                       }
+                       cmd_destroy_session(sess,
                                        notification_thread_handle);
                }
+               session_unlock_list();
+               pthread_mutex_destroy(&session_list->lock);
        }
 
        wait_consumer(&kconsumer_data);
@@ -619,16 +611,21 @@ static int setup_lttng_msg_no_cmd_header(struct command_ctx *cmd_ctx,
 static int update_kernel_poll(struct lttng_poll_event *events)
 {
        int ret;
-       struct ltt_session *session;
        struct ltt_kernel_channel *channel;
+       struct ltt_session *session;
+       const struct ltt_session_list *session_list = session_get_list();
 
        DBG("Updating kernel poll set");
 
        session_lock_list();
-       cds_list_for_each_entry(session, &session_list_ptr->head, list) {
+       cds_list_for_each_entry(session, &session_list->head, list) {
+               if (!session_get(session)) {
+                       continue;
+               }
                session_lock(session);
                if (session->kernel_session == NULL) {
                        session_unlock(session);
+                       session_put(session);
                        continue;
                }
 
@@ -638,6 +635,7 @@ static int update_kernel_poll(struct lttng_poll_event *events)
                        ret = lttng_poll_add(events, channel->fd, LPOLLIN | LPOLLRDNORM);
                        if (ret < 0) {
                                session_unlock(session);
+                               session_put(session);
                                goto error;
                        }
                        DBG("Channel fd %d added to kernel set", channel->fd);
@@ -665,14 +663,19 @@ static int update_kernel_stream(int fd)
        struct ltt_session *session;
        struct ltt_kernel_session *ksess;
        struct ltt_kernel_channel *channel;
+       const struct ltt_session_list *session_list = session_get_list();
 
        DBG("Updating kernel streams for channel fd %d", fd);
 
        session_lock_list();
-       cds_list_for_each_entry(session, &session_list_ptr->head, list) {
+       cds_list_for_each_entry(session, &session_list->head, list) {
+               if (!session_get(session)) {
+                       continue;
+               }
                session_lock(session);
                if (session->kernel_session == NULL) {
                        session_unlock(session);
+                       session_put(session);
                        continue;
                }
                ksess = session->kernel_session;
@@ -720,12 +723,14 @@ static int update_kernel_stream(int fd)
                        rcu_read_unlock();
                }
                session_unlock(session);
+               session_put(session);
        }
        session_unlock_list();
        return ret;
 
 error:
        session_unlock(session);
+       session_put(session);
        session_unlock_list();
        return ret;
 }
@@ -737,6 +742,7 @@ error:
 static void update_ust_app(int app_sock)
 {
        struct ltt_session *sess, *stmp;
+       const struct ltt_session_list *session_list = session_get_list();
 
        /* Consumer is in an ERROR state. Stop any application update. */
        if (uatomic_read(&ust_consumerd_state) == CONSUMER_ERROR) {
@@ -745,9 +751,12 @@ static void update_ust_app(int app_sock)
        }
 
        /* For all tracing session(s) */
-       cds_list_for_each_entry_safe(sess, stmp, &session_list_ptr->head, list) {
+       cds_list_for_each_entry_safe(sess, stmp, &session_list->head, list) {
                struct ust_app *app;
 
+               if (!session_get(sess)) {
+                       continue;
+               }
                session_lock(sess);
                if (!sess->ust_session) {
                        goto unlock_session;
@@ -771,6 +780,7 @@ static void update_ust_app(int app_sock)
                rcu_read_unlock();
        unlock_session:
                session_unlock(sess);
+               session_put(sess);
        }
 }
 
@@ -2716,17 +2726,22 @@ static unsigned int lttng_sessions_count(uid_t uid, gid_t gid)
 {
        unsigned int i = 0;
        struct ltt_session *session;
+       const struct ltt_session_list *session_list = session_get_list();
 
        DBG("Counting number of available session for UID %d GID %d",
                        uid, gid);
-       cds_list_for_each_entry(session, &session_list_ptr->head, list) {
-               /*
-                * Only list the sessions the user can control.
-                */
-               if (!session_access_ok(session, uid, gid)) {
+       cds_list_for_each_entry(session, &session_list->head, list) {
+               if (!session_get(session)) {
                        continue;
                }
-               i++;
+               session_lock(session);
+               /* Only count the sessions the user can control. */
+               if (session_access_ok(session, uid, gid) &&
+                               !session->destroyed) {
+                       i++;
+               }
+               session_unlock(session);
+               session_put(session);
        }
        return i;
 }
@@ -3213,7 +3228,8 @@ skip_domain:
        if (need_tracing_session) {
                if (!session_access_ok(cmd_ctx->session,
                                LTTNG_SOCK_GET_UID_CRED(&cmd_ctx->creds),
-                               LTTNG_SOCK_GET_GID_CRED(&cmd_ctx->creds))) {
+                               LTTNG_SOCK_GET_GID_CRED(&cmd_ctx->creds)) ||
+                               cmd_ctx->session->destroyed) {
                        ret = LTTNG_ERR_EPERM;
                        goto error;
                }
@@ -3728,11 +3744,8 @@ error_add_context:
        }
        case LTTNG_DESTROY_SESSION:
        {
-               ret = cmd_destroy_session(cmd_ctx->session, kernel_poll_pipe[1],
+               ret = cmd_destroy_session(cmd_ctx->session,
                                notification_thread_handle);
-
-               /* Set session to NULL so we do not unlock it after free. */
-               cmd_ctx->session = NULL;
                break;
        }
        case LTTNG_LIST_DOMAINS:
@@ -4189,6 +4202,7 @@ error:
 setup_error:
        if (cmd_ctx->session) {
                session_unlock(cmd_ctx->session);
+               session_put(cmd_ctx->session);
        }
        if (need_tracing_session) {
                session_unlock_list();
@@ -5961,12 +5975,6 @@ int main(int argc, char **argv)
        /* Init UST command queue. */
        cds_wfcq_init(&ust_cmd_queue.head, &ust_cmd_queue.tail);
 
-       /*
-        * Get session list pointer. This pointer MUST NOT be free'd. This list
-        * is statically declared in session.c
-        */
-       session_list_ptr = session_get_list();
-
        cmd_init();
 
        /* Check for the application socket timeout env variable. */

diff --git a/src/bin/lttng-sessiond/rotation-thread.c b/src/bin/lttng-sessiond/rotation-thread.c
index f78e8ea25adf53bed9ad917e87338deaac328c95..a9f9967a3b49ee04d3fab5beba7b3d27c268c627 100644 (file)
--- a/src/bin/lttng-sessiond/rotation-thread.c
+++ b/src/bin/lttng-sessiond/rotation-thread.c
@@ -706,7 +706,6 @@ int handle_job_queue(struct rotation_thread_handle *handle,
 {
        int ret = 0;
        int fd = lttng_pipe_get_readfd(queue->event_pipe);
-       struct ltt_session *session;
        char buf;
 
        ret = lttng_read(fd, &buf, 1);
@@ -717,6 +716,7 @@ int handle_job_queue(struct rotation_thread_handle *handle,
        }
 
        for (;;) {
+               struct ltt_session *session;
                struct rotation_thread_job *job;
 
                /* Take the queue lock only to pop an element from the list. */
@@ -747,12 +747,14 @@ int handle_job_queue(struct rotation_thread_handle *handle,
                         */
                        session_unlock_list();
                        free(job);
+                       session_put(session);
                        continue;
                }
 
                session_lock(session);
                ret = run_job(job, session, handle->notification_thread_handle);
                session_unlock(session);
+               session_put(session);
                session_unlock_list();
                free(job);
                if (ret) {
@@ -841,6 +843,7 @@ int handle_condition(const struct lttng_condition *condition,
 
 end_unlock:
        session_unlock(session);
+       session_put(session);
 end:
        return ret;
 }

diff --git a/src/bin/lttng-sessiond/save.c b/src/bin/lttng-sessiond/save.c
index 8daf870c270395ab3675b6eb35a15db81c8f3168..ce9f2f9ff1772910ac5ce1a1b3b8023e566214c9 100644 (file)
--- a/src/bin/lttng-sessiond/save.c
+++ b/src/bin/lttng-sessiond/save.c
@@ -2301,7 +2301,7 @@ int save_session(struct ltt_session *session,
 
        if (!session_access_ok(session,
                LTTNG_SOCK_GET_UID_CRED(creds),
-               LTTNG_SOCK_GET_GID_CRED(creds))) {
+               LTTNG_SOCK_GET_GID_CRED(creds)) || session->destroyed) {
                ret = LTTNG_ERR_EPERM;
                goto end;
        }
@@ -2527,6 +2527,7 @@ int cmd_save_sessions(struct lttng_save_session_attr *attr,
                session_lock(session);
                ret = save_session(session, attr, creds);
                session_unlock(session);
+               session_put(session);
                if (ret) {
                        goto end;
                }
@@ -2534,10 +2535,13 @@ int cmd_save_sessions(struct lttng_save_session_attr *attr,
                struct ltt_session_list *list = session_get_list();
 
                cds_list_for_each_entry(session, &list->head, list) {
+                       if (!session_get(session)) {
+                               continue;
+                       }
                        session_lock(session);
                        ret = save_session(session, attr, creds);
                        session_unlock(session);
-
+                       session_put(session);
                        /* Don't abort if we don't have the required permissions. */
                        if (ret && ret != LTTNG_ERR_EPERM) {
                                goto end;

diff --git a/src/bin/lttng-sessiond/session.c b/src/bin/lttng-sessiond/session.c
index abf61e898ceb8e6ef037a53e6729020397e5194b..173afa8784f47b22e7e24fbc237dbbaae70809d2 100644 (file)
--- a/src/bin/lttng-sessiond/session.c
+++ b/src/bin/lttng-sessiond/session.c
@@ -29,6 +29,8 @@
 #include <common/common.h>
 #include <common/sessiond-comm/sessiond-comm.h>
 #include <lttng/location-internal.h>
+#include "lttng-sessiond.h"
+#include "kernel.h"
 
 #include "session.h"
 #include "utils.h"
@@ -383,29 +385,120 @@ void session_unlock(struct ltt_session *session)
        pthread_mutex_unlock(&session->lock);
 }
 
+static
+void session_release(struct urcu_ref *ref)
+{
+       int ret;
+       struct ltt_ust_session *usess;
+       struct ltt_kernel_session *ksess;
+       struct ltt_session *session = container_of(ref, typeof(*session), ref);
+
+       usess = session->ust_session;
+       ksess = session->kernel_session;
+
+       /* Clean kernel session teardown */
+       kernel_destroy_session(ksess);
+
+       /* UST session teardown */
+       if (usess) {
+               /* Close any relayd session */
+               consumer_output_send_destroy_relayd(usess->consumer);
+
+               /* Destroy every UST application related to this session. */
+               ret = ust_app_destroy_trace_all(usess);
+               if (ret) {
+                       ERR("Error in ust_app_destroy_trace_all");
+               }
+
+               /* Clean up the rest. */
+               trace_ust_destroy_session(usess);
+       }
+
+       /*
+        * Must notify the kernel thread here to update it's poll set in order to
+        * remove the channel(s)' fd just destroyed.
+        */
+       ret = notify_thread_pipe(kernel_poll_pipe[1]);
+       if (ret < 0) {
+               PERROR("write kernel poll pipe");
+       }
+
+       DBG("Destroying session %s (id %" PRIu64 ")", session->name, session->id);
+       pthread_mutex_destroy(&session->lock);
+
+       consumer_output_put(session->consumer);
+       snapshot_destroy(&session->snapshot);
+       del_session_list(session);
+       del_session_ht(session);
+       free(session);
+}
+
+/*
+ * Acquire a reference to a session.
+ * This function may fail (return false); its return value must be checked.
+ */
+bool session_get(struct ltt_session *session)
+{
+       return urcu_ref_get_unless_zero(&session->ref);
+}
+
+/*
+ * Release a reference to a session.
+ */
+void session_put(struct ltt_session *session)
+{
+       /*
+        * The session list lock must be held as any session_put()
+        * may cause the removal of the session from the session_list.
+        */
+       ASSERT_LOCKED(ltt_session_list.lock);
+       assert(session->ref.refcount);
+       urcu_ref_put(&session->ref, session_release);
+}
+
+/*
+ * Destroy a session.
+ *
+ * This method does not immediately release/free the session as other
+ * components may still hold a reference to the session. However,
+ * the session should no longer be presented to the user.
+ *
+ * Releases the session list's reference to the session
+ * and marks it as destroyed. Iterations on the session list should be
+ * mindful of the "destroyed" flag.
+ */
+void session_destroy(struct ltt_session *session)
+{
+       assert(!session->destroyed);
+       session->destroyed = true;
+       session_put(session);
+}
+
 /*
  * Return a ltt_session structure ptr that matches name. If no session found,
  * NULL is returned. This must be called with the session list lock held using
  * session_lock_list and session_unlock_list.
+ * A reference to the session is implicitly acquired by this function.
  */
 struct ltt_session *session_find_by_name(const char *name)
 {
        struct ltt_session *iter;
 
        assert(name);
+       ASSERT_LOCKED(ltt_session_list.lock);
 
        DBG2("Trying to find session by name %s", name);
 
        cds_list_for_each_entry(iter, &ltt_session_list.head, list) {
-               if (strncmp(iter->name, name, NAME_MAX) == 0) {
+               if (!strncmp(iter->name, name, NAME_MAX) &&
+                               !iter->destroyed) {
                        goto found;
                }
        }
 
-       iter = NULL;
-
+       return NULL;
 found:
-       return iter;
+       return session_get(iter) ? iter : NULL;
 }
 
 /*
@@ -419,6 +512,8 @@ struct ltt_session *session_find_by_id(uint64_t id)
        struct lttng_ht_iter iter;
        struct ltt_session *ls;
 
+       ASSERT_LOCKED(ltt_session_list.lock);
+
        if (!ltt_sessions_ht_by_id) {
                goto end;
        }
@@ -431,36 +526,13 @@ struct ltt_session *session_find_by_id(uint64_t id)
        ls = caa_container_of(node, struct ltt_session, node);
 
        DBG3("Session %" PRIu64 " found by id.", id);
-       return ls;
+       return session_get(ls) ? ls : NULL;
 
 end:
        DBG3("Session %" PRIu64 " NOT found by id", id);
        return NULL;
 }
 
-/*
- * 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)
-{
-       /* Safety check */
-       assert(session);
-
-       DBG("Destroying session %s (id %" PRIu64 ")", session->name, session->id);
-       del_session_list(session);
-       pthread_mutex_destroy(&session->lock);
-       del_session_ht(session);
-
-       consumer_output_put(session->consumer);
-       snapshot_destroy(&session->snapshot);
-       free(session);
-
-       return LTTNG_OK;
-}
-
 /*
  * Create a brand new session and add it to the session list.
  */
@@ -477,6 +549,8 @@ int session_create(char *name, uid_t uid, gid_t gid)
                goto error_malloc;
        }
 
+       urcu_ref_init(&new_session->ref);
+
        /* Define session name */
        if (name != NULL) {
                if (snprintf(new_session->name, NAME_MAX, "%s", name) < 0) {

diff --git a/src/bin/lttng-sessiond/session.h b/src/bin/lttng-sessiond/session.h
index b1ca2504e6d86b722fef2bc0c4c3b7ae048e7872..6a059798d925afbd681f07a898f866e7dc8374f2 100644 (file)
--- a/src/bin/lttng-sessiond/session.h
+++ b/src/bin/lttng-sessiond/session.h
@@ -67,6 +67,7 @@ struct ltt_session {
        char hostname[HOST_NAME_MAX]; /* Local hostname. */
        struct ltt_kernel_session *kernel_session;
        struct ltt_ust_session *ust_session;
+       struct urcu_ref ref;
        /*
         * Protect any read/write on this session data structure. This lock must be
         * acquired *before* using any public functions declared below. Use
@@ -75,6 +76,8 @@ struct ltt_session {
        pthread_mutex_t lock;
        struct cds_list_head list;
        uint64_t id;            /* session unique identifier */
+       /* Indicates if a destroy command has been applied to this session. */
+       bool destroyed;
        /* UID/GID of the user owning the session */
        uid_t uid;
        gid_t gid;
@@ -205,7 +208,6 @@ struct ltt_session {
 
 /* Prototypes */
 int session_create(char *name, uid_t uid, gid_t gid);
-int session_destroy(struct ltt_session *session);
 
 void session_lock(struct ltt_session *session);
 void session_lock_list(void);
@@ -213,6 +215,11 @@ int session_trylock_list(void);
 void session_unlock(struct ltt_session *session);
 void session_unlock_list(void);
 
+void session_destroy(struct ltt_session *session);
+
+bool session_get(struct ltt_session *session);
+void session_put(struct ltt_session *session);
+
 enum consumer_dst_type session_get_consumer_destination_type(
                const struct ltt_session *session);
 const char *session_get_net_consumer_hostname(

diff --git a/src/bin/lttng-sessiond/ust-app.c b/src/bin/lttng-sessiond/ust-app.c
index 7b44fb4b48a050927fe2c897650dd2fda156dfac..e2b7a083c0ecb33eb95db9c8789abe34c154f2f8 100644 (file)
--- a/src/bin/lttng-sessiond/ust-app.c
+++ b/src/bin/lttng-sessiond/ust-app.c
@@ -437,6 +437,9 @@ void save_per_pid_lost_discarded_counters(struct ust_app_channel *ua_chan)
 
 end:
        rcu_read_unlock();
+       if (session) {
+               session_put(session);
+       }
 }
 
 /*
@@ -2865,7 +2868,7 @@ static int create_channel_per_uid(struct ust_app *app,
        int ret;
        struct buffer_reg_uid *reg_uid;
        struct buffer_reg_channel *reg_chan;
-       struct ltt_session *session;
+       struct ltt_session *session = NULL;
        enum lttng_error_code notification_ret;
        struct ust_registry_channel *chan_reg;
 
@@ -2968,6 +2971,9 @@ send_channel:
        }
 
 error:
+       if (session) {
+               session_put(session);
+       }
        return ret;
 }
 
@@ -2986,7 +2992,7 @@ static int create_channel_per_pid(struct ust_app *app,
        int ret;
        struct ust_registry_session *registry;
        enum lttng_error_code cmd_ret;
-       struct ltt_session *session;
+       struct ltt_session *session = NULL;
        uint64_t chan_reg_key;
        struct ust_registry_channel *chan_reg;
 
@@ -3061,6 +3067,9 @@ error_remove_from_registry:
        }
 error:
        rcu_read_unlock();
+       if (session) {
+               session_put(session);
+       }
        return ret;
 }
 
@@ -3251,7 +3260,7 @@ static int create_ust_app_metadata(struct ust_app_session *ua_sess,
        struct ust_app_channel *metadata;
        struct consumer_socket *socket;
        struct ust_registry_session *registry;
-       struct ltt_session *session;
+       struct ltt_session *session = NULL;
 
        assert(ua_sess);
        assert(app);
@@ -3342,6 +3351,9 @@ error_consumer:
        delete_ust_app_channel(-1, metadata, app);
 error:
        pthread_mutex_unlock(&registry->lock);
+       if (session) {
+               session_put(session);
+       }
        return ret;
 }
 
@@ -5963,7 +5975,7 @@ enum lttng_error_code ust_app_snapshot_record(struct ltt_ust_session *usess,
        struct lttng_ht_iter iter;
        struct ust_app *app;
        char pathname[PATH_MAX];
-       struct ltt_session *session;
+       struct ltt_session *session = NULL;
        uint64_t trace_archive_id;
 
        assert(usess);
@@ -6111,6 +6123,9 @@ enum lttng_error_code ust_app_snapshot_record(struct ltt_ust_session *usess,
 
 error:
        rcu_read_unlock();
+       if (session) {
+               session_put(session);
+       }
        return status;
 }
 

diff --git a/tests/unit/test_session.c b/tests/unit/test_session.c
index cfbb52d048dd00820e6817a24dbe221a5a165716..74643221c6aa4b93e7d41a63398a9e5b6946bf4b 100644 (file)
--- a/tests/unit/test_session.c
+++ b/tests/unit/test_session.c
@@ -112,9 +112,11 @@ static void empty_session_list(void)
 {
        struct ltt_session *iter, *tmp;
 
+       session_lock_list();
        cds_list_for_each_entry_safe(iter, tmp, &session_list->head, list) {
                session_destroy(iter);
        }
+       session_unlock_list();
 
        /* Session list must be 0 */
        assert(!session_list_count());
@@ -160,19 +162,18 @@ static int destroy_one_session(struct ltt_session *session)
        strncpy(session_name, session->name, sizeof(session_name));
        session_name[sizeof(session_name) - 1] = '\0';
 
-       ret = session_destroy(session);
-       if (ret == LTTNG_OK) {
-               ret = find_session_name(session_name);
-               if (ret < 0) {
-                       /* Success, -1 means that the sesion is NOT found */
-                       return 0;
-               } else {
-                       /* Fail */
-                       return -1;
-               }
-       }
+       session_destroy(session);
+       session_put(session);
 
-       return 0;
+       ret = find_session_name(session_name);
+       if (ret < 0) {
+               /* Success, -1 means that the sesion is NOT found */
+               ret = 0;
+       } else {
+               /* Fail */
+               ret = -1;
+       }
+       return ret;
 }
 
 /*
@@ -187,17 +188,27 @@ static int two_session_same_name(void)
        ret = create_one_session(SESSION1);
        if (ret < 0) {
                /* Fail */
-               return -1;
+               ret = -1;
+               goto end;
        }
 
+       session_lock_list();
        sess = session_find_by_name(SESSION1);
        if (sess) {
                /* Success */
-               return 0;
+               session_put(sess);
+               session_unlock_list();
+               ret = 0;
+               goto end_unlock;
+       } else {
+               /* Fail */
+               ret = -1;
+               goto end_unlock;
        }
-
-       /* Fail */
-       return -1;
+end_unlock:
+       session_unlock_list();
+end:
+       return ret;
 }
 
 void test_session_list(void)
@@ -217,6 +228,7 @@ void test_validate_session(void)
 {
        struct ltt_session *tmp;
 
+       session_lock_list();
        tmp = session_find_by_name(SESSION1);
 
        ok(tmp != NULL,
@@ -228,12 +240,15 @@ void test_validate_session(void)
 
        session_lock(tmp);
        session_unlock(tmp);
+       session_put(tmp);
+       session_unlock_list();
 }
 
 void test_destroy_session(void)
 {
        struct ltt_session *tmp;
 
+       session_lock_list();
        tmp = session_find_by_name(SESSION1);
 
        ok(tmp != NULL,
@@ -242,6 +257,7 @@ void test_destroy_session(void)
        ok(destroy_one_session(tmp) == 0,
           "Destroying session: %s destroyed",
           SESSION1);
+       session_unlock_list();
 }
 
 void test_duplicate_session(void)
@@ -279,8 +295,10 @@ void test_large_session_number(void)
 
        failed = 0;
 
+       session_lock_list();
        for (i = 0; i < MAX_SESSIONS; i++) {
                cds_list_for_each_entry_safe(iter, tmp, &session_list->head, list) {
+                       assert(session_get(iter));
                        ret = destroy_one_session(iter);
                        if (ret < 0) {
                                diag("session %d destroy failed", i);
@@ -288,6 +306,7 @@ void test_large_session_number(void)
                        }
                }
        }
+       session_unlock_list();
 
        ok(failed == 0 && session_list_count() == 0,
           "Large sessions number: destroyed %u sessions",