From b389abbe14d653f704fcf8f952539cc5ce775c53 Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Tue, 16 Aug 2011 09:55:30 -0400 Subject: [PATCH] Modify default kernel channel size/number Too small kernel channels (4kB) leads to unwelcome delay at trace indexing and adds buffer switch overhead. Use a default closer to the 0.x LTTng for buffer size: 256kB subbuffer 4 subbuffers/buffer for 1MB per channel per CPU in normal tracing mode, and 1MB + 256kB per channel per CPU in overwrite mode. We currently plan to still use 4kB * 8 sub-buffers for UST because we allocate these buffers per process, which makes memory consumption more of a concern. The metadata channel is kept small (4kB), but we keep a number of subbuffer of 8 for now, as the behavior between "start" and spawning the consumer is in the wrong order: we will need to spawn the consumer before start is performed, because "start" returns blocks for 10 seconds and returns an error if the buffers are too small to write metadata (and they are not consumed). Also fix a memory leak in init_default_channel. process_client_msg() now uses a check / alloc / failure for channel lookup rather than an endless retry, which could have led to denial of service in case of internal error. Signed-off-by: Mathieu Desnoyers --- include/lttng-share.h | 32 ++++++---- ltt-sessiond/main.c | 100 +++++++++++++++++++------------ ltt-sessiond/trace.c | 4 +- lttng/commands/enable_channels.c | 25 ++++++-- tests/test_kernel_data_trace.c | 4 +- 5 files changed, 106 insertions(+), 59 deletions(-) diff --git a/include/lttng-share.h b/include/lttng-share.h index 9d3a259a6..d076ef3ad 100644 --- a/include/lttng-share.h +++ b/include/lttng-share.h @@ -28,20 +28,30 @@ typedef uint64_t u64; typedef __s64 s64; /* Default channel attributes */ -#define DEFAULT_CHANNEL_NAME "channel0" -#define DEFAULT_CHANNEL_OVERWRITE 0 /* usec */ +#define DEFAULT_CHANNEL_NAME "channel0" +#define DEFAULT_CHANNEL_OVERWRITE 0 /* usec */ /* DEFAULT_CHANNEL_SUBBUF_SIZE must always be a power of 2 */ -#define DEFAULT_CHANNEL_SUBBUF_SIZE 4096 /* bytes */ +#define DEFAULT_CHANNEL_SUBBUF_SIZE 4096 /* bytes */ /* DEFAULT_CHANNEL_SUBBUF_NUM must always be a power of 2 */ -#define DEFAULT_CHANNEL_SUBBUF_NUM 8 -#define DEFAULT_CHANNEL_SWITCH_TIMER 0 /* usec */ -#define DEFAULT_CHANNEL_READ_TIMER 200 /* usec */ -/* See lttng-kernel.h enum lttng_kernel_output for channel output */ -#define DEFAULT_KERNEL_CHANNEL_OUTPUT LTTNG_EVENT_SPLICE +#define DEFAULT_CHANNEL_SUBBUF_NUM 8 +#define DEFAULT_CHANNEL_SWITCH_TIMER 0 /* usec */ +#define DEFAULT_CHANNEL_READ_TIMER 200 /* usec */ +#define DEFAULT_CHANNEL_OUTPUT LTTNG_EVENT_MMAP + +#define DEFAULT_METADATA_SUBBUF_SIZE 4096 +#define DEFAULT_METADATA_SUBBUF_NUM 8 +//TODO: keeping value to 8 currently because consumer is only spawned after +//start, so it cannot empty the metadata buffer at trace start. +//#define DEFAULT_METADATA_SUBBUF_NUM 2 -/* == NOT IMPLEMENTED == -#define DEFAULT_UST_CHANNEL_OUTPUT LTTNG_UST_MMAP -*/ +/* Kernel has different defaults */ + +/* DEFAULT_KERNEL_CHANNEL_SUBBUF_SIZE must always be a power of 2 */ +#define DEFAULT_KERNEL_CHANNEL_SUBBUF_SIZE 262144 /* bytes */ +/* DEFAULT_KERNEL_CHANNEL_SUBBUF_NUM must always be a power of 2 */ +#define DEFAULT_KERNEL_CHANNEL_SUBBUF_NUM 4 +/* See lttng-kernel.h enum lttng_kernel_output for channel output */ +#define DEFAULT_KERNEL_CHANNEL_OUTPUT LTTNG_EVENT_SPLICE /* * lttng user-space instrumentation type diff --git a/ltt-sessiond/main.c b/ltt-sessiond/main.c index 652798d34..470ab1818 100644 --- a/ltt-sessiond/main.c +++ b/ltt-sessiond/main.c @@ -1241,7 +1241,8 @@ static int notify_kernel_pollfd(void) /* * Allocate a channel structure and fill it. */ -static struct lttng_channel *init_default_channel(char *name) +static struct lttng_channel *init_default_channel(enum lttng_domain_type domain_type, + char *name) { struct lttng_channel *chan; @@ -1253,18 +1254,29 @@ static struct lttng_channel *init_default_channel(char *name) if (snprintf(chan->name, NAME_MAX, "%s", name) < 0) { perror("snprintf channel name"); - return NULL; + goto error; } chan->attr.overwrite = DEFAULT_CHANNEL_OVERWRITE; - chan->attr.subbuf_size = DEFAULT_CHANNEL_SUBBUF_SIZE; - chan->attr.num_subbuf = DEFAULT_CHANNEL_SUBBUF_NUM; chan->attr.switch_timer_interval = DEFAULT_CHANNEL_SWITCH_TIMER; chan->attr.read_timer_interval = DEFAULT_CHANNEL_READ_TIMER; - chan->attr.output = DEFAULT_KERNEL_CHANNEL_OUTPUT; -error: + switch (domain_type) { + case LTTNG_DOMAIN_KERNEL: + chan->attr.subbuf_size = DEFAULT_KERNEL_CHANNEL_SUBBUF_SIZE; + chan->attr.num_subbuf = DEFAULT_KERNEL_CHANNEL_SUBBUF_NUM; + chan->attr.output = DEFAULT_KERNEL_CHANNEL_OUTPUT; + break; + /* TODO: add UST */ + default: + goto error; /* Not implemented */ + } + return chan; + +error: + free(chan); + return NULL; } /* @@ -1713,26 +1725,32 @@ static int process_client_msg(struct command_ctx *cmd_ctx) switch (cmd_ctx->lsm->domain.type) { case LTTNG_DOMAIN_KERNEL: - do { + kchan = get_kernel_channel_by_name(channel_name, + cmd_ctx->session->kernel_session); + if (kchan == NULL) { + DBG("Channel not found. Creating channel %s", channel_name); + + chan = init_default_channel(cmd_ctx->lsm->domain.type, channel_name); + if (chan == NULL) { + ret = LTTCOMM_FATAL; + goto error; + } + + ret = kernel_create_channel(cmd_ctx->session->kernel_session, + chan, cmd_ctx->session->kernel_session->trace_path); + if (ret < 0) { + ret = LTTCOMM_KERN_CHAN_FAIL; + goto error; + } kchan = get_kernel_channel_by_name(channel_name, cmd_ctx->session->kernel_session); if (kchan == NULL) { - DBG("Channel not found. Creating channel %s", channel_name); - - chan = init_default_channel(channel_name); - if (chan == NULL) { - ret = LTTCOMM_FATAL; - goto error; - } - - ret = kernel_create_channel(cmd_ctx->session->kernel_session, - chan, cmd_ctx->session->kernel_session->trace_path); - if (ret < 0) { - ret = LTTCOMM_KERN_CHAN_FAIL; - goto error; - } + ERR("Channel %s not found after creation. Internal error, giving up.", + channel_name); + ret = LTTCOMM_FATAL; + goto error; } - } while (kchan == NULL); + } kevent = get_kernel_event_by_name(cmd_ctx->lsm->u.enable.event.name, kchan); if (kevent == NULL) { @@ -1785,26 +1803,32 @@ static int process_client_msg(struct command_ctx *cmd_ctx) switch (cmd_ctx->lsm->domain.type) { case LTTNG_DOMAIN_KERNEL: - do { + kchan = get_kernel_channel_by_name(channel_name, + cmd_ctx->session->kernel_session); + if (kchan == NULL) { + DBG("Channel not found. Creating channel %s", channel_name); + + chan = init_default_channel(cmd_ctx->lsm->domain.type, channel_name); + if (chan == NULL) { + ret = LTTCOMM_FATAL; + goto error; + } + + ret = kernel_create_channel(cmd_ctx->session->kernel_session, + chan, cmd_ctx->session->kernel_session->trace_path); + if (ret < 0) { + ret = LTTCOMM_KERN_CHAN_FAIL; + goto error; + } kchan = get_kernel_channel_by_name(channel_name, cmd_ctx->session->kernel_session); if (kchan == NULL) { - DBG("Channel not found. Creating channel %s", channel_name); - - chan = init_default_channel(channel_name); - if (chan == NULL) { - ret = LTTCOMM_FATAL; - goto error; - } - - ret = kernel_create_channel(cmd_ctx->session->kernel_session, - chan, cmd_ctx->session->kernel_session->trace_path); - if (ret < 0) { - ret = LTTCOMM_KERN_CHAN_FAIL; - goto error; - } + ERR("Channel %s not found after creation. Internal error, giving up.", + channel_name); + ret = LTTCOMM_FATAL; + goto error; } - } while (kchan == NULL); + } /* For each event in the kernel session */ cds_list_for_each_entry(kevent, &kchan->events_list.head, list) { diff --git a/ltt-sessiond/trace.c b/ltt-sessiond/trace.c index 774321e59..621e4dd3f 100644 --- a/ltt-sessiond/trace.c +++ b/ltt-sessiond/trace.c @@ -247,8 +247,8 @@ struct ltt_kernel_metadata *trace_create_kernel_metadata(char *path) /* Set default attributes */ chan->attr.overwrite = DEFAULT_CHANNEL_OVERWRITE; - chan->attr.subbuf_size = DEFAULT_CHANNEL_SUBBUF_SIZE; - chan->attr.num_subbuf = DEFAULT_CHANNEL_SUBBUF_NUM; + chan->attr.subbuf_size = DEFAULT_METADATA_SUBBUF_SIZE; + chan->attr.num_subbuf = DEFAULT_METADATA_SUBBUF_NUM; chan->attr.switch_timer_interval = DEFAULT_CHANNEL_SWITCH_TIMER; chan->attr.read_timer_interval = DEFAULT_CHANNEL_READ_TIMER; chan->attr.output = DEFAULT_KERNEL_CHANNEL_OUTPUT; diff --git a/lttng/commands/enable_channels.c b/lttng/commands/enable_channels.c index e49559e4f..bd732f22f 100644 --- a/lttng/commands/enable_channels.c +++ b/lttng/commands/enable_channels.c @@ -162,12 +162,25 @@ error: */ static void init_channel_config(void) { - chan.attr.overwrite = DEFAULT_CHANNEL_OVERWRITE; - chan.attr.subbuf_size = DEFAULT_CHANNEL_SUBBUF_SIZE; - chan.attr.num_subbuf = DEFAULT_CHANNEL_SUBBUF_NUM; - chan.attr.switch_timer_interval = DEFAULT_CHANNEL_SWITCH_TIMER; - chan.attr.read_timer_interval = DEFAULT_CHANNEL_READ_TIMER; - chan.attr.output = DEFAULT_KERNEL_CHANNEL_OUTPUT; + if (opt_kernel) { + /* kernel default */ + chan.attr.overwrite = DEFAULT_CHANNEL_OVERWRITE; + chan.attr.switch_timer_interval = DEFAULT_CHANNEL_SWITCH_TIMER; + chan.attr.read_timer_interval = DEFAULT_CHANNEL_READ_TIMER; + + chan.attr.subbuf_size = DEFAULT_KERNEL_CHANNEL_SUBBUF_SIZE; + chan.attr.num_subbuf = DEFAULT_KERNEL_CHANNEL_SUBBUF_NUM; + chan.attr.output = DEFAULT_KERNEL_CHANNEL_OUTPUT; + } else { + /* default behavior, used by UST. */ + chan.attr.overwrite = DEFAULT_CHANNEL_OVERWRITE; + chan.attr.switch_timer_interval = DEFAULT_CHANNEL_SWITCH_TIMER; + chan.attr.read_timer_interval = DEFAULT_CHANNEL_READ_TIMER; + + chan.attr.subbuf_size = DEFAULT_CHANNEL_SUBBUF_SIZE; + chan.attr.num_subbuf = DEFAULT_CHANNEL_SUBBUF_NUM; + chan.attr.output = DEFAULT_CHANNEL_OUTPUT; + } } /* diff --git a/tests/test_kernel_data_trace.c b/tests/test_kernel_data_trace.c index 7a1f076c6..2892ab236 100644 --- a/tests/test_kernel_data_trace.c +++ b/tests/test_kernel_data_trace.c @@ -96,9 +96,9 @@ static void create_kernel_metadata(void) assert(kern->metadata->conf->attr.overwrite == DEFAULT_CHANNEL_OVERWRITE); assert(kern->metadata->conf->attr.subbuf_size - == DEFAULT_CHANNEL_SUBBUF_SIZE); + == DEFAULT_METADATA_SUBBUF_SIZE); assert(kern->metadata->conf->attr.num_subbuf - == DEFAULT_CHANNEL_SUBBUF_NUM); + == DEFAULT_METADATA_SUBBUF_NUM); assert(kern->metadata->conf->attr.switch_timer_interval == DEFAULT_CHANNEL_SWITCH_TIMER); assert(kern->metadata->conf->attr.read_timer_interval -- 2.34.1