From: David Goulet Date: Fri, 15 Jul 2011 23:11:07 +0000 (-0400) Subject: Add locking for all session data structure X-Git-Tag: v2.0-pre1~35 X-Git-Url: https://git.lttng.org/?p=lttng-tools.git;a=commitdiff_plain;h=b5541356f517dba006af9f676df8131dcb68f132 Add locking for all session data structure The session daemon is single threaded when processing client command however the CPU hotplug thread was introduced and uses a lot of session data structure that are now protected by pthread mutex with this commit. Signed-off-by: David Goulet --- diff --git a/ltt-sessiond/main.c b/ltt-sessiond/main.c index 3426b5cde..c3e162341 100644 --- a/ltt-sessiond/main.c +++ b/ltt-sessiond/main.c @@ -99,11 +99,20 @@ static sem_t kconsumerd_sem; static pthread_mutex_t kconsumerd_pid_mutex; /* Mutex to control kconsumerd pid assignation */ +/* + * 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. + */ +static struct ltt_session_list *session_list_ptr; + /* * teardown_kernel_session * - * Complete teardown of a kernel session. This free all data structure - * related to a kernel session and update counter. + * Complete teardown of a kernel session. This free all data structure related + * to a kernel session and update counter. */ static void teardown_kernel_session(struct ltt_session *session) { @@ -161,11 +170,14 @@ static void cleanup() DBG("Cleaning up all session"); /* Cleanup ALL session */ - cds_list_for_each_entry(sess, <t_session_list.head, list) { + cds_list_for_each_entry(sess, &session_list_ptr->head, list) { teardown_kernel_session(sess); // TODO complete session cleanup (including UST) } + /* Destroy session list mutex */ + pthread_mutex_destroy(&session_list_ptr->lock); + DBG("Closing kernel fd"); close(kernel_tracer_fd); close(kernel_poll_pipe[0]); @@ -432,11 +444,15 @@ static int update_kernel_pollfd(void) DBG("Updating kernel_pollfd"); /* Get the number of channel of all kernel session */ - cds_list_for_each_entry(session, <t_session_list.head, list) { + pthread_mutex_lock(&session_list_ptr->lock); + cds_list_for_each_entry(session, &session_list_ptr->head, list) { + lock_session(session); if (session->kernel_session == NULL) { + unlock_session(session); continue; } nb_fd += session->kernel_session->channel_count; + unlock_session(session); } DBG("Resizing kernel_pollfd to size %d", nb_fd); @@ -447,12 +463,15 @@ static int update_kernel_pollfd(void) goto error; } - cds_list_for_each_entry(session, <t_session_list.head, list) { + cds_list_for_each_entry(session, &session_list_ptr->head, list) { + lock_session(session); if (session->kernel_session == NULL) { + unlock_session(session); continue; } if (i >= nb_fd) { ERR("To much channel for kernel_pollfd size"); + unlock_session(session); break; } cds_list_for_each_entry(channel, &session->kernel_session->channel_list.head, list) { @@ -460,7 +479,9 @@ static int update_kernel_pollfd(void) kernel_pollfd[i].events = POLLIN | POLLRDNORM; i++; } + unlock_session(session); } + pthread_mutex_unlock(&session_list_ptr->lock); /* Adding wake up pipe */ kernel_pollfd[nb_fd - 1].fd = kernel_poll_pipe[0]; @@ -469,6 +490,7 @@ static int update_kernel_pollfd(void) return nb_fd; error: + pthread_mutex_unlock(&session_list_ptr->lock); return -1; } @@ -488,8 +510,11 @@ static int update_kernel_stream(int fd) DBG("Updating kernel streams for channel fd %d", fd); - cds_list_for_each_entry(session, <t_session_list.head, list) { + pthread_mutex_lock(&session_list_ptr->lock); + cds_list_for_each_entry(session, &session_list_ptr->head, list) { + lock_session(session); if (session->kernel_session == NULL) { + unlock_session(session); continue; } cds_list_for_each_entry(channel, &session->kernel_session->channel_list.head, list) { @@ -512,9 +537,14 @@ static int update_kernel_stream(int fd) goto end; } } + unlock_session(session); } end: + if (session) { + unlock_session(session); + } + pthread_mutex_unlock(&session_list_ptr->lock); return ret; } @@ -561,10 +591,15 @@ static void *thread_manage_kernel(void *data) * Check if the wake up pipe was triggered. If so, the kernel_pollfd * must be updated. */ - if (kernel_pollfd[nb_fd - 1].revents == POLLIN) { + switch (kernel_pollfd[nb_fd - 1].revents) { + case POLLIN: ret = read(kernel_poll_pipe[0], &tmp, 1); update_poll_flag = 1; continue; + case POLLERR: + goto error; + default: + break; } for (i = 0; i < nb_fd; i++) { @@ -1123,6 +1158,9 @@ static int process_client_msg(struct command_ctx *cmd_ctx) ret = LTTCOMM_SELECT_SESS; } goto error; + } else { + /* Acquire lock for the session */ + lock_session(cmd_ctx->session); } break; } @@ -1829,6 +1867,10 @@ static int process_client_msg(struct command_ctx *cmd_ctx) /* Set return code */ cmd_ctx->llm->ret_code = ret; + if (cmd_ctx->session) { + unlock_session(cmd_ctx->session); + } + return ret; error: @@ -1842,6 +1884,9 @@ error: cmd_ctx->llm->ret_code = ret; setup_error: + if (cmd_ctx->session) { + unlock_session(cmd_ctx->session); + } return ret; } @@ -2433,6 +2478,9 @@ int main(int argc, char **argv) goto error; } + /* Get session list pointer */ + session_list_ptr = get_session_list(); + while (1) { /* Create thread to manage the client socket */ ret = pthread_create(&client_thread, NULL, thread_manage_clients, (void *) NULL); diff --git a/ltt-sessiond/session.c b/ltt-sessiond/session.c index 92111ffed..97ab098dc 100644 --- a/ltt-sessiond/session.c +++ b/ltt-sessiond/session.c @@ -17,6 +17,7 @@ */ #define _GNU_SOURCE +#include #include #include #include @@ -26,63 +27,97 @@ #include "lttngerr.h" #include "session.h" -/* Variables */ -static unsigned int session_count; - -/* Static internal function */ -static void add_session_list(struct ltt_session *ls); -static void del_session_list(struct ltt_session *ls); - -/* Init global session list */ -struct ltt_session_list ltt_session_list = { - .head = CDS_LIST_HEAD_INIT(ltt_session_list.head), -}; - /* - * get_session_list + * NOTES: * - * Return a pointer to the session list. + * No ltt_session.lock is taken here because those data structure are widely + * spread across the lttng-tools code base so before caling functions below + * that can read/write a session, the caller MUST acquire the session lock + * using lock_session() and unlock_session(). */ -struct ltt_session_list *get_session_list(void) -{ - return <t_session_list; -} /* - * get_session_count + * Init tracing session list. * - * Return session_count + * Please see session.h for more explanation and correct usage of the list. */ -unsigned int get_session_count(void) -{ - return session_count; -} +static struct ltt_session_list ltt_session_list = { + .head = CDS_LIST_HEAD_INIT(ltt_session_list.head), + .lock = PTHREAD_MUTEX_INITIALIZER, + .count = 0, +}; /* * add_session_list * * Add a ltt_session structure to the global list. + * + * The caller MUST acquire the session list lock before. */ static void add_session_list(struct ltt_session *ls) { cds_list_add(&ls->list, <t_session_list.head); - session_count++; + ltt_session_list.count++; } /* * del_session_list * * Delete a ltt_session structure to the global list. + * + * The caller MUST acquire the session list lock before. */ static void del_session_list(struct ltt_session *ls) { cds_list_del(&ls->list); /* Sanity check */ - if (session_count != 0) { - session_count--; + if (ltt_session_list.count > 0) { + ltt_session_list.count--; } } +/* + * get_session_list + * + * Return a pointer to the session list. + */ +struct ltt_session_list *get_session_list(void) +{ + return <t_session_list; +} + +/* + * Acquire session lock + */ +void lock_session(struct ltt_session *session) +{ + pthread_mutex_lock(&session->lock); +} + +/* + * Release session lock + */ +void unlock_session(struct ltt_session *session) +{ + pthread_mutex_unlock(&session->lock); +} + +/* + * get_session_count + * + * Return session_count + */ +unsigned int get_session_count(void) +{ + unsigned int count; + + pthread_mutex_lock(<t_session_list.lock); + count = ltt_session_list.count; + pthread_mutex_unlock(<t_session_list.lock); + + return count; +} + /* * find_session_by_name * @@ -94,12 +129,14 @@ struct ltt_session *find_session_by_name(char *name) int found = 0; struct ltt_session *iter; + pthread_mutex_lock(<t_session_list.lock); cds_list_for_each_entry(iter, <t_session_list.head, list) { if (strncmp(iter->name, name, strlen(name)) == 0) { found = 1; break; } } + pthread_mutex_unlock(<t_session_list.lock); if (!found) { iter = NULL; @@ -111,26 +148,29 @@ struct ltt_session *find_session_by_name(char *name) /* * destroy_session * - * Delete session from the global session list - * and free the memory. + * Delete session from the session list and free the memory. * - * Return -1 if no session is found. - * On success, return 1; + * Return -1 if no session is found. On success, return 1; */ int destroy_session(char *name) { int found = -1; struct ltt_session *iter; + pthread_mutex_lock(<t_session_list.lock); cds_list_for_each_entry(iter, <t_session_list.head, list) { if (strcmp(iter->name, name) == 0) { DBG("Destroying session %s", iter->name); del_session_list(iter); + free(iter->name); + free(iter->path); + pthread_mutex_destroy(&iter->lock); free(iter); found = 1; break; } } + pthread_mutex_unlock(<t_session_list.lock); return found; } @@ -138,7 +178,7 @@ int destroy_session(char *name) /* * create_session * - * Create a brand new session and add it to the global session list. + * Create a brand new session and add it to the session list. */ int create_session(char *name, char *path) { @@ -194,12 +234,6 @@ int create_session(char *name, char *path) goto error; } - /* - * Set consumer (identifier) to 0. This means that there is - * NO consumer attach to that session yet. - */ - new_session->ust_consumer = 0; - /* Init kernel session */ new_session->kernel_session = NULL; @@ -209,10 +243,15 @@ int create_session(char *name, char *path) /* Set trace list counter */ new_session->ust_trace_count = 0; - /* Add new session to the global session list */ + /* Add new session to the session list */ + pthread_mutex_lock(<t_session_list.lock); add_session_list(new_session); + pthread_mutex_unlock(<t_session_list.lock); + + /* Init lock */ + pthread_mutex_init(&new_session->lock, NULL); - DBG("Tracing session %s created in %s", name, new_session->path); + DBG("Tracing session %s created in %s", new_session->name, new_session->path); return 0; @@ -240,9 +279,11 @@ void get_lttng_session(struct lttng_session *sessions) DBG("Getting all available session"); - /* Iterate over session list and append data after - * the control struct in the buffer. + /* + * Iterate over session list and append data after the control struct in + * the buffer. */ + pthread_mutex_lock(<t_session_list.lock); cds_list_for_each_entry(iter, <t_session_list.head, list) { strncpy(lsess.path, iter->path, sizeof(lsess.path)); lsess.path[sizeof(lsess.path) - 1] = '\0'; @@ -253,5 +294,6 @@ void get_lttng_session(struct lttng_session *sessions) /* Reset struct for next pass */ memset(&lsess, 0, sizeof(lsess)); } + pthread_mutex_unlock(<t_session_list.lock); } diff --git a/ltt-sessiond/session.h b/ltt-sessiond/session.h index 4227f9244..9eaa1c535 100644 --- a/ltt-sessiond/session.h +++ b/ltt-sessiond/session.h @@ -22,30 +22,53 @@ #include #include -/* Global session list */ +/* + * Tracing session list + * + * Statically declared in session.c and can be accessed by using + * get_session_list() function that returns the pointer to the list. + */ struct ltt_session_list { + /* + * This lock protects any read/write access to the list and count (which is + * basically the list size). All public functions in session.c acquire this + * lock and release it before returning. If none of those functions are + * used, the lock MUST be acquired in order to iterate or/and do any + * actions on that list. + */ + pthread_mutex_t lock; + /* + * Number of element in the list. + */ + unsigned int count; struct cds_list_head head; }; -extern struct ltt_session_list ltt_session_list; - -/* ltt-session - This data structure contains information needed - * to identify a tracing session for both LTTng and UST. +/* + * This data structure contains information needed to identify a tracing + * session for both LTTng and UST. */ struct ltt_session { - struct cds_list_head list; char *name; char *path; - struct cds_list_head ust_traces; struct ltt_kernel_session *kernel_session; + struct cds_list_head ust_traces; unsigned int ust_trace_count; - pid_t ust_consumer; + /* + * Protect any read/write on this session data structure. This lock must be + * acquired *before* using any public functions declared below. Use + * lock_session() and unlock_session() for that. + */ + pthread_mutex_t lock; + struct cds_list_head list; }; /* Prototypes */ int create_session(char *name, char *path); int destroy_session(char *name); void get_lttng_session(struct lttng_session *sessions); +void lock_session(struct ltt_session *session); +void unlock_session(struct ltt_session *session); struct ltt_session *find_session_by_name(char *name); unsigned int get_session_count(void); struct ltt_session_list *get_session_list(void);