From 645328ae989e5f50a3a49c1ac34b2fee287a3d7b Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 25 Jun 2014 15:42:15 -0400 Subject: [PATCH] Fix: add a kernel context list to the channel The internal state of the session daemon was recording only one context per channel thus overwriting the previous one if multiple context were added causing a memory leak. This commit adds a list inside a kernel channel which keeps track of all context added. It also fixes the save command that now saves all of them. Fixes #205 Signed-off-by: David Goulet --- src/bin/lttng-sessiond/context.c | 46 +++++++++++++++------------ src/bin/lttng-sessiond/kernel.c | 12 ++----- src/bin/lttng-sessiond/kernel.h | 2 +- src/bin/lttng-sessiond/save.c | 32 +++++++++++++++---- src/bin/lttng-sessiond/trace-kernel.c | 44 +++++++++++++++++++++++-- src/bin/lttng-sessiond/trace-kernel.h | 14 +++++--- tests/unit/test_kernel_data.c | 1 - 7 files changed, 106 insertions(+), 45 deletions(-) diff --git a/src/bin/lttng-sessiond/context.c b/src/bin/lttng-sessiond/context.c index 1b87df2e9..10352d026 100644 --- a/src/bin/lttng-sessiond/context.c +++ b/src/bin/lttng-sessiond/context.c @@ -34,7 +34,7 @@ * Add kernel context to all channel. */ static int add_kctx_all_channels(struct ltt_kernel_session *ksession, - struct lttng_kernel_context *kctx) + struct ltt_kernel_context *kctx) { int ret; struct ltt_kernel_channel *kchan; @@ -62,7 +62,7 @@ error: /* * Add kernel context to a specific channel. */ -static int add_kctx_to_channel(struct lttng_kernel_context *kctx, +static int add_kctx_to_channel(struct ltt_kernel_context *kctx, struct ltt_kernel_channel *kchan) { int ret; @@ -149,60 +149,66 @@ int context_kernel_add(struct ltt_kernel_session *ksession, { int ret; struct ltt_kernel_channel *kchan; - struct lttng_kernel_context kctx; + struct ltt_kernel_context *kctx; assert(ksession); assert(ctx); assert(channel_name); + kctx = trace_kernel_create_context(NULL); + if (!kctx) { + ret = LTTNG_ERR_NOMEM; + goto error; + } + /* Setup kernel context structure */ switch (ctx->ctx) { case LTTNG_EVENT_CONTEXT_PID: - kctx.ctx = LTTNG_KERNEL_CONTEXT_PID; + kctx->ctx.ctx = LTTNG_KERNEL_CONTEXT_PID; break; case LTTNG_EVENT_CONTEXT_PROCNAME: - kctx.ctx = LTTNG_KERNEL_CONTEXT_PROCNAME; + kctx->ctx.ctx = LTTNG_KERNEL_CONTEXT_PROCNAME; break; case LTTNG_EVENT_CONTEXT_PRIO: - kctx.ctx = LTTNG_KERNEL_CONTEXT_PRIO; + kctx->ctx.ctx = LTTNG_KERNEL_CONTEXT_PRIO; break; case LTTNG_EVENT_CONTEXT_NICE: - kctx.ctx = LTTNG_KERNEL_CONTEXT_NICE; + kctx->ctx.ctx = LTTNG_KERNEL_CONTEXT_NICE; break; case LTTNG_EVENT_CONTEXT_VPID: - kctx.ctx = LTTNG_KERNEL_CONTEXT_VPID; + kctx->ctx.ctx = LTTNG_KERNEL_CONTEXT_VPID; break; case LTTNG_EVENT_CONTEXT_TID: - kctx.ctx = LTTNG_KERNEL_CONTEXT_TID; + kctx->ctx.ctx = LTTNG_KERNEL_CONTEXT_TID; break; case LTTNG_EVENT_CONTEXT_VTID: - kctx.ctx = LTTNG_KERNEL_CONTEXT_VTID; + kctx->ctx.ctx = LTTNG_KERNEL_CONTEXT_VTID; break; case LTTNG_EVENT_CONTEXT_PPID: - kctx.ctx = LTTNG_KERNEL_CONTEXT_PPID; + kctx->ctx.ctx = LTTNG_KERNEL_CONTEXT_PPID; break; case LTTNG_EVENT_CONTEXT_VPPID: - kctx.ctx = LTTNG_KERNEL_CONTEXT_VPPID; + kctx->ctx.ctx = LTTNG_KERNEL_CONTEXT_VPPID; break; case LTTNG_EVENT_CONTEXT_HOSTNAME: - kctx.ctx = LTTNG_KERNEL_CONTEXT_HOSTNAME; + kctx->ctx.ctx = LTTNG_KERNEL_CONTEXT_HOSTNAME; break; case LTTNG_EVENT_CONTEXT_PERF_CPU_COUNTER: case LTTNG_EVENT_CONTEXT_PERF_COUNTER: - kctx.ctx = LTTNG_KERNEL_CONTEXT_PERF_CPU_COUNTER; + kctx->ctx.ctx = LTTNG_KERNEL_CONTEXT_PERF_CPU_COUNTER; break; default: return LTTNG_ERR_KERN_CONTEXT_FAIL; } - kctx.u.perf_counter.type = ctx->u.perf_counter.type; - kctx.u.perf_counter.config = ctx->u.perf_counter.config; - strncpy(kctx.u.perf_counter.name, ctx->u.perf_counter.name, + kctx->ctx.u.perf_counter.type = ctx->u.perf_counter.type; + kctx->ctx.u.perf_counter.config = ctx->u.perf_counter.config; + strncpy(kctx->ctx.u.perf_counter.name, ctx->u.perf_counter.name, LTTNG_SYMBOL_NAME_LEN); - kctx.u.perf_counter.name[LTTNG_SYMBOL_NAME_LEN - 1] = '\0'; + kctx->ctx.u.perf_counter.name[LTTNG_SYMBOL_NAME_LEN - 1] = '\0'; if (*channel_name == '\0') { - ret = add_kctx_all_channels(ksession, &kctx); + ret = add_kctx_all_channels(ksession, kctx); if (ret != LTTNG_OK) { goto error; } @@ -214,7 +220,7 @@ int context_kernel_add(struct ltt_kernel_session *ksession, goto error; } - ret = add_kctx_to_channel(&kctx, kchan); + ret = add_kctx_to_channel(kctx, kchan); if (ret != LTTNG_OK) { goto error; } diff --git a/src/bin/lttng-sessiond/kernel.c b/src/bin/lttng-sessiond/kernel.c index 44a71fc59..b997a4aea 100644 --- a/src/bin/lttng-sessiond/kernel.c +++ b/src/bin/lttng-sessiond/kernel.c @@ -37,7 +37,7 @@ * Add context on a kernel channel. */ int kernel_add_channel_context(struct ltt_kernel_channel *chan, - struct lttng_kernel_context *ctx) + struct ltt_kernel_context *ctx) { int ret; @@ -45,7 +45,7 @@ int kernel_add_channel_context(struct ltt_kernel_channel *chan, assert(ctx); DBG("Adding context to channel %s", chan->channel->name); - ret = kernctl_add_context(chan->fd, ctx); + ret = kernctl_add_context(chan->fd, &ctx->ctx); if (ret < 0) { if (errno != EEXIST) { PERROR("add context ioctl"); @@ -56,13 +56,7 @@ int kernel_add_channel_context(struct ltt_kernel_channel *chan, goto error; } - chan->ctx = zmalloc(sizeof(struct lttng_kernel_context)); - if (chan->ctx == NULL) { - PERROR("zmalloc event context"); - goto error; - } - - memcpy(chan->ctx, ctx, sizeof(struct lttng_kernel_context)); + cds_list_add_tail(&ctx->list, &chan->ctx_list); return 0; diff --git a/src/bin/lttng-sessiond/kernel.h b/src/bin/lttng-sessiond/kernel.h index 78d12aba3..e79478012 100644 --- a/src/bin/lttng-sessiond/kernel.h +++ b/src/bin/lttng-sessiond/kernel.h @@ -32,7 +32,7 @@ #define KERNEL_EVENT_INIT_LIST_SIZE 64 int kernel_add_channel_context(struct ltt_kernel_channel *chan, - struct lttng_kernel_context *ctx); + struct ltt_kernel_context *ctx); int kernel_create_session(struct ltt_session *session, int tracer_fd); int kernel_create_channel(struct ltt_kernel_session *session, struct lttng_channel *chan); diff --git a/src/bin/lttng-sessiond/save.c b/src/bin/lttng-sessiond/save.c index 8afdbb500..684c4edbf 100644 --- a/src/bin/lttng-sessiond/save.c +++ b/src/bin/lttng-sessiond/save.c @@ -670,12 +670,6 @@ int save_kernel_context(struct config_writer *writer, goto end; } - ret = config_writer_open_element(writer, config_element_contexts); - if (ret) { - ret = LTTNG_ERR_SAVE_IO_FAIL; - goto end; - } - ret = config_writer_open_element(writer, config_element_context); if (ret) { ret = LTTNG_ERR_SAVE_IO_FAIL; @@ -741,6 +735,30 @@ int save_kernel_context(struct config_writer *writer, goto end; } +end: + return ret; +} + +static +int save_kernel_contexts(struct config_writer *writer, + struct ltt_kernel_channel *kchan) +{ + int ret; + struct ltt_kernel_context *ctx; + + ret = config_writer_open_element(writer, config_element_contexts); + if (ret) { + ret = LTTNG_ERR_SAVE_IO_FAIL; + goto end; + } + + cds_list_for_each_entry(ctx, &kchan->ctx_list, list) { + ret = save_kernel_context(writer, &ctx->ctx); + if (ret) { + goto end; + } + } + /* /contexts */ ret = config_writer_close_element(writer); if (ret) { @@ -848,7 +866,7 @@ int save_kernel_channel(struct config_writer *writer, goto end; } - ret = save_kernel_context(writer, kchan->ctx); + ret = save_kernel_contexts(writer, kchan); if (ret) { goto end; } diff --git a/src/bin/lttng-sessiond/trace-kernel.c b/src/bin/lttng-sessiond/trace-kernel.c index 363d010a1..8e074fd91 100644 --- a/src/bin/lttng-sessiond/trace-kernel.c +++ b/src/bin/lttng-sessiond/trace-kernel.c @@ -165,10 +165,10 @@ struct ltt_kernel_channel *trace_kernel_create_channel( lkc->stream_count = 0; lkc->event_count = 0; lkc->enabled = 1; - lkc->ctx = NULL; /* Init linked list */ CDS_INIT_LIST_HEAD(&lkc->events_list.head); CDS_INIT_LIST_HEAD(&lkc->stream_list.head); + CDS_INIT_LIST_HEAD(&lkc->ctx_list); return lkc; @@ -176,6 +176,30 @@ error: return NULL; } +/* + * Allocate and init a kernel context object. + * + * Return the allocated object or NULL on error. + */ +struct ltt_kernel_context *trace_kernel_create_context( + struct lttng_kernel_context *ctx) +{ + struct ltt_kernel_context *kctx; + + kctx = zmalloc(sizeof(*kctx)); + if (!kctx) { + PERROR("zmalloc kernel context"); + goto error; + } + + if (ctx) { + memcpy(&kctx->ctx, ctx, sizeof(kctx->ctx)); + } + +error: + return kctx; +} + /* * Allocate and initialize a kernel event. Set name and event type. * @@ -375,6 +399,17 @@ void trace_kernel_destroy_event(struct ltt_kernel_event *event) free(event); } +/* + * Cleanup kernel context structure. + */ +void trace_kernel_destroy_context(struct ltt_kernel_context *ctx) +{ + assert(ctx); + + cds_list_del(&ctx->list); + free(ctx); +} + /* * Cleanup kernel channel structure. */ @@ -382,6 +417,7 @@ void trace_kernel_destroy_channel(struct ltt_kernel_channel *channel) { struct ltt_kernel_stream *stream, *stmp; struct ltt_kernel_event *event, *etmp; + struct ltt_kernel_context *ctx, *ctmp; int ret; assert(channel); @@ -405,11 +441,15 @@ void trace_kernel_destroy_channel(struct ltt_kernel_channel *channel) trace_kernel_destroy_event(event); } + /* For each context in the channel list */ + cds_list_for_each_entry_safe(ctx, ctmp, &channel->ctx_list, list) { + trace_kernel_destroy_context(ctx); + } + /* Remove from channel list */ cds_list_del(&channel->list); free(channel->channel); - free(channel->ctx); free(channel); } diff --git a/src/bin/lttng-sessiond/trace-kernel.h b/src/bin/lttng-sessiond/trace-kernel.h index f576e3a60..db91b2ed2 100644 --- a/src/bin/lttng-sessiond/trace-kernel.h +++ b/src/bin/lttng-sessiond/trace-kernel.h @@ -42,6 +42,11 @@ struct ltt_kernel_channel_list { struct cds_list_head head; }; +struct ltt_kernel_context { + struct lttng_kernel_context ctx; + struct cds_list_head list; +}; + /* Kernel event */ struct ltt_kernel_event { int fd; @@ -56,11 +61,7 @@ struct ltt_kernel_channel { int enabled; unsigned int stream_count; unsigned int event_count; - /* - * TODO: need internal representation to support more than a - * single context. - */ - struct lttng_kernel_context *ctx; + struct cds_list_head ctx_list; struct lttng_channel *channel; struct ltt_kernel_event_list events_list; struct ltt_kernel_stream_list stream_list; @@ -135,6 +136,8 @@ struct ltt_kernel_event *trace_kernel_create_event(struct lttng_event *ev); struct ltt_kernel_metadata *trace_kernel_create_metadata(void); struct ltt_kernel_stream *trace_kernel_create_stream(const char *name, unsigned int count); +struct ltt_kernel_context *trace_kernel_create_context( + struct lttng_kernel_context *ctx); /* * Destroy functions free() the data structure and remove from linked list if @@ -145,5 +148,6 @@ void trace_kernel_destroy_metadata(struct ltt_kernel_metadata *metadata); void trace_kernel_destroy_channel(struct ltt_kernel_channel *channel); void trace_kernel_destroy_event(struct ltt_kernel_event *event); void trace_kernel_destroy_stream(struct ltt_kernel_stream *stream); +void trace_kernel_destroy_context(struct ltt_kernel_context *ctx); #endif /* _LTT_TRACE_KERNEL_H */ diff --git a/tests/unit/test_kernel_data.c b/tests/unit/test_kernel_data.c index e2182e926..5b82672d7 100644 --- a/tests/unit/test_kernel_data.c +++ b/tests/unit/test_kernel_data.c @@ -120,7 +120,6 @@ static void test_create_kernel_channel(void) ok(chan->fd == -1 && chan->enabled == 1 && chan->stream_count == 0 && - chan->ctx == NULL && chan->channel->attr.overwrite == attr.attr.overwrite, "Validate kernel channel"); -- 2.34.1