From: Jérémie Galarneau Date: Wed, 21 Nov 2018 00:24:58 +0000 (-0500) Subject: Reference count ltt_session objects X-Git-Tag: v2.12.0-rc1~737 X-Git-Url: https://git.lttng.org/?p=lttng-tools.git;a=commitdiff_plain;h=e32d7f274604b77bcd83c24994e88df3761ed658 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 --- diff --git a/src/bin/lttng-sessiond/agent-thread.c b/src/bin/lttng-sessiond/agent-thread.c index b23865681..51221c4c6 100644 --- 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 2aea2ea74..def3e64e5 100644 --- 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 c0fbee8f0..a207e52b3 100644 --- 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 fe924747a..63fd79938 100644 --- 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 05e447754..9ccad8be4 100644 --- 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 edb7f7b2f..8e972b069 100644 --- 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 f0bd98781..25020d87e 100644 --- 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 7a0cdb81c..98f586307 100644 --- 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 f78e8ea25..a9f9967a3 100644 --- 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 8daf870c2..ce9f2f9ff 100644 --- 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 abf61e898..173afa878 100644 --- a/src/bin/lttng-sessiond/session.c +++ b/src/bin/lttng-sessiond/session.c @@ -29,6 +29,8 @@ #include #include #include +#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, <t_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 b1ca2504e..6a059798d 100644 --- 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 7b44fb4b4..e2b7a083c 100644 --- 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(®istry->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 cfbb52d04..74643221c 100644 --- 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",