From mboxrd@z Thu Jan 1 00:00:00 1970 From: mathieu.desnoyers@polymtl.ca (Mathieu Desnoyers) Date: Wed, 3 Dec 2008 00:35:48 -0500 Subject: [ltt-dev] [PATCH 2/2] Make ltt support enable/disable each channel In-Reply-To: <232931D7C1804812BF50E571921A750B@zhaoleiwin> References: <49351A0F.5060500@cn.fujitsu.com> <49351A77.5070808@cn.fujitsu.com> <20081202170647.GD19095@Krystal> <232931D7C1804812BF50E571921A750B@zhaoleiwin> Message-ID: <20081203053548.GB13701@Krystal> * Zhaolei (zhaolei at cn.fujitsu.com) wrote: > * From: "Mathieu Desnoyers" > >* Zhaolei (zhaolei at cn.fujitsu.com) wrote: > >> It can reduce system loads when user only need to trace some > >> channels. > >> > >> Signed-off-by: Zhao Lei > >> --- > >> include/linux/ltt-tracer.h | 2 + > >> ltt/ltt-serialize.c | 4 + > >> ltt/ltt-tracer.c | 268 +++++++++++++++++++++++++------------- > >> ltt/probes/ltt-type-serializer.c | 4 + > >> 4 files changed, 190 insertions(+), 88 deletions(-) > >> > >> diff --git a/include/linux/ltt-tracer.h b/include/linux/ltt-tracer.h > >> index a18edd1..87696a9 100644 > >> --- a/include/linux/ltt-tracer.h > >> +++ b/include/linux/ltt-tracer.h > >> @@ -711,6 +711,8 @@ int ltt_trace_set_channel_subbufsize(const char *trace_name, > >> const char *channel_name, unsigned size); > >> int ltt_trace_set_channel_subbufcount(const char *trace_name, > >> const char *channel_name, unsigned cnt); > >> +int ltt_trace_set_channel_enable(const char *trace_name, > >> + const char *channel_name, unsigned enable); > >> int ltt_trace_set_channel_overwrite(const char *trace_name, > >> const char *channel_name, unsigned overwrite); > >> int ltt_trace_alloc(const char *trace_name); > >> diff --git a/ltt/ltt-serialize.c b/ltt/ltt-serialize.c > >> index 3b31a30..9d7ce59 100644 > >> --- a/ltt/ltt-serialize.c > >> +++ b/ltt/ltt-serialize.c > >> @@ -633,6 +633,10 @@ notrace void ltt_vtrace(void *probe_data, void *call_data, > >> rflags = 0; > >> #endif > >> channel = ltt_get_channel_from_index(trace, channel_index); > >> + if (!channel) > >> + /* channel is NULL if it is not enabled */ > >> + continue; > >> + > >> /* reserve space : header and data */ > >> ret = ltt_reserve_slot(trace, channel, &transport_data, > >> data_size, &slot_size, &buf_offset, > >> diff --git a/ltt/ltt-tracer.c b/ltt/ltt-tracer.c > >> index 341074d..a14fd5f 100644 > >> --- a/ltt/ltt-tracer.c > >> +++ b/ltt/ltt-tracer.c > >> @@ -67,6 +67,69 @@ int (*ltt_statedump_functor)(struct ltt_trace_struct *trace) = > >> ltt_statedump_default; > >> struct module *ltt_statedump_owner; > >> > >> +/* > >> + * Todo: > >> + * Make similar function in channel.c, > >> + * so it will be useful for both ltt-tracer.c and ltt-marker-control.c > >> + */ > >> +/* > >> + * Its order is MUST be same with enum ltt_channels > >> + */ > >> +struct chan_info_struct { > >> + const char *name; > >> + unsigned int channel_index; > >> + unsigned int def_subbufsize; > >> + unsigned int def_subbufcount; > >> +} chan_infos[] = { > >> + [LTT_CHANNEL_CPU] = { > >> + LTT_CPU_CHANNEL, > >> + GET_CHANNEL_INDEX(cpu), > >> + LTT_DEFAULT_SUBBUF_SIZE_HIGH, > >> + LTT_DEFAULT_N_SUBBUFS_HIGH, > >> + }, > >> + [LTT_CHANNEL_PROCESSES] = { > >> + LTT_PROCESSES_CHANNEL, > >> + GET_CHANNEL_INDEX(processes), > >> + LTT_DEFAULT_SUBBUF_SIZE_MED, > >> + LTT_DEFAULT_N_SUBBUFS_MED, > >> + }, > >> + [LTT_CHANNEL_INTERRUPTS] = { > >> + LTT_INTERRUPTS_CHANNEL, > >> + GET_CHANNEL_INDEX(interrupts), > >> + LTT_DEFAULT_SUBBUF_SIZE_LOW, > >> + LTT_DEFAULT_N_SUBBUFS_LOW, > >> + }, > >> + [LTT_CHANNEL_NETWORK] = { > >> + LTT_NETWORK_CHANNEL, > >> + GET_CHANNEL_INDEX(network), > >> + LTT_DEFAULT_SUBBUF_SIZE_LOW, > >> + LTT_DEFAULT_N_SUBBUFS_LOW, > >> + }, > >> + [LTT_CHANNEL_MODULES] = { > >> + LTT_MODULES_CHANNEL, > >> + GET_CHANNEL_INDEX(modules), > >> + LTT_DEFAULT_SUBBUF_SIZE_LOW, > >> + LTT_DEFAULT_N_SUBBUFS_LOW, > >> + }, > >> + [LTT_CHANNEL_METADATA] = { > >> + LTT_METADATA_CHANNEL, > >> + GET_CHANNEL_INDEX(metadata), > >> + LTT_DEFAULT_SUBBUF_SIZE_LOW, > >> + LTT_DEFAULT_N_SUBBUFS_LOW, > >> + }, > >> +}; > >> + > >> +static enum ltt_channels get_channel_type_from_name(const char *name) > >> +{ > >> + int i; > >> + > >> + for (i = 0; i < ARRAY_SIZE(chan_infos); i++) > >> + if (!strcmp(name, chan_infos[i].name)) > >> + return (enum ltt_channels)i; > >> + > >> + return NR_LTT_CHANNELS; > >> +} > >> + > >> /** > >> * ltt_module_register : LTT module registration > >> * @name : module type > >> @@ -216,13 +279,17 @@ EXPORT_SYMBOL_GPL(ltt_write_trace_header); > >> > >> static void trace_async_wakeup(struct ltt_trace_struct *trace) > >> { > >> + int i; > >> + struct ltt_channel_struct *chan; > >> + > >> /* Must check each channel for pending read wakeup */ > >> - trace->ops->wakeup_channel(trace->channel.metadata); > >> - trace->ops->wakeup_channel(trace->channel.interrupts); > >> - trace->ops->wakeup_channel(trace->channel.processes); > >> - trace->ops->wakeup_channel(trace->channel.modules); > >> - trace->ops->wakeup_channel(trace->channel.network); > >> - trace->ops->wakeup_channel(trace->channel.cpu); > >> + for (i = 0; i < NR_LTT_CHANNELS; i++) { > >> + chan = *(struct ltt_channel_struct **)((char *)trace > >> + + chan_infos[i].channel_index); > > > > Hrm, this is the third place I see we have these ugly casts (it is also > > in ltt_trace_alloc). Can we create a wrapper for it ? e.g. in > > ltt-tracer.c : > > > > static struct ltt_channel_struct ** > > ltt_get_trace_channel(struct ltt_trace_struct *trace, unsigned int idx) > > { > > return (struct ltt_channel_struct **) > > ((char *)trace + chan_infos[idx].channel_index); > > } > Hello, Mathieu > > Thanks for your revice and merge. > > IMHO, It is ugly, one of reason is because we use "offset of channel's pointer in ltt_trace_struct" > as ltt_active_marker.channel. > > I think we can use other way to link a channel to marker, for example: > set ltt_active_marker.channel = struct ltt_channel_struct *. > or > set ltt_active_marker.channel = enum ltt_channels. > > We can change it at same time we make channel dynamic(user can create new channel). > Ideally, if we can use a struct ltt_channel_struct * directly, this would remove the need for having a base + offset. Yes, that would be good. Feel free to try implementing this improvement in the new LTTng 0.61. Mathieu > B.R. > Zhaolei > > > > > > > >> + /* chan is NULL if it is not traced */ > >> + if (chan) > >> + trace->ops->wakeup_channel(chan); > >> + } > >> } > >> > >> /* Timer to send async wakeups to the readers */ > >> @@ -355,6 +422,14 @@ int ltt_trace_setup(const char *trace_name) > >> } > >> > >> strncpy(new_trace->trace_name, trace_name, NAME_MAX); > >> + > >> + /* > >> + * Datas in metadata channel(marker info) is necessary to be able to > >> + * read the trace, it is enabled initially, and can't be disabled later. > >> + */ > >> + new_trace->setting.channels[LTT_CHANNEL_METADATA].flags |= > >> + CHANNEL_FLAG_ENABLE; > >> + > >> list_add(&new_trace->list, <t_traces.setup_head); > >> > >> ltt_unlock_traces(); > >> @@ -412,69 +487,6 @@ traces_error: > >> } > >> EXPORT_SYMBOL_GPL(ltt_trace_set_type); > >> > >> -/* > >> - * Todo: > >> - * Make similar function in channel.c, > >> - * so it will be useful for both ltt-tracer.c and ltt-marker-control.c > >> - */ > >> -/* > >> - * Its order is MUST be same with enum ltt_channels > >> - */ > >> -struct chan_info_struct { > >> - const char *name; > >> - unsigned int channel_index; > >> - unsigned int def_subbufsize; > >> - unsigned int def_subbufcount; > >> -} chan_infos[] = { > >> - [LTT_CHANNEL_CPU] = { > >> - LTT_CPU_CHANNEL, > >> - GET_CHANNEL_INDEX(cpu), > >> - LTT_DEFAULT_SUBBUF_SIZE_HIGH, > >> - LTT_DEFAULT_N_SUBBUFS_HIGH, > >> - }, > >> - [LTT_CHANNEL_PROCESSES] = { > >> - LTT_PROCESSES_CHANNEL, > >> - GET_CHANNEL_INDEX(processes), > >> - LTT_DEFAULT_SUBBUF_SIZE_MED, > >> - LTT_DEFAULT_N_SUBBUFS_MED, > >> - }, > >> - [LTT_CHANNEL_INTERRUPTS] = { > >> - LTT_INTERRUPTS_CHANNEL, > >> - GET_CHANNEL_INDEX(interrupts), > >> - LTT_DEFAULT_SUBBUF_SIZE_LOW, > >> - LTT_DEFAULT_N_SUBBUFS_LOW, > >> - }, > >> - [LTT_CHANNEL_NETWORK] = { > >> - LTT_NETWORK_CHANNEL, > >> - GET_CHANNEL_INDEX(network), > >> - LTT_DEFAULT_SUBBUF_SIZE_LOW, > >> - LTT_DEFAULT_N_SUBBUFS_LOW, > >> - }, > >> - [LTT_CHANNEL_MODULES] = { > >> - LTT_MODULES_CHANNEL, > >> - GET_CHANNEL_INDEX(modules), > >> - LTT_DEFAULT_SUBBUF_SIZE_LOW, > >> - LTT_DEFAULT_N_SUBBUFS_LOW, > >> - }, > >> - [LTT_CHANNEL_METADATA] = { > >> - LTT_METADATA_CHANNEL, > >> - GET_CHANNEL_INDEX(metadata), > >> - LTT_DEFAULT_SUBBUF_SIZE_LOW, > >> - LTT_DEFAULT_N_SUBBUFS_LOW, > >> - }, > >> -}; > >> - > >> -static enum ltt_channels get_channel_type_from_name(const char *name) > >> -{ > >> - int i; > >> - > >> - for (i = 0; i < ARRAY_SIZE(chan_infos); i++) > >> - if (!strcmp(name, chan_infos[i].name)) > >> - return (enum ltt_channels)i; > >> - > >> - return NR_LTT_CHANNELS; > >> -} > >> - > >> int ltt_trace_set_channel_subbufsize(const char *trace_name, > >> const char *channel_name, unsigned size) > >> { > >> @@ -539,6 +551,52 @@ traces_error: > >> } > >> EXPORT_SYMBOL_GPL(ltt_trace_set_channel_subbufcount); > >> > >> +int ltt_trace_set_channel_enable(const char *trace_name, > >> + const char *channel_name, unsigned enable) > >> +{ > >> + int err = 0; > >> + struct ltt_trace_struct *trace; > >> + enum ltt_channels channel; > >> + > >> + ltt_lock_traces(); > >> + > >> + trace = _ltt_trace_find_setup(trace_name); > >> + if (!trace) { > >> + printk(KERN_ERR "LTT : Trace not found %s\n", trace_name); > >> + err = -ENOENT; > >> + goto traces_error; > >> + } > >> + > >> + channel = get_channel_type_from_name(channel_name); > >> + if (channel == NR_LTT_CHANNELS) { > >> + printk(KERN_ERR "LTT : Channel %s is not present.\n", > >> + channel_name); > >> + err = -EINVAL; > >> + goto traces_error; > >> + } > >> + > >> + /* > >> + * Datas in metadata channel(marker info) is necessary to be able to > >> + * read the trace, we always enable this channel. > >> + */ > >> + if (!enable && channel == LTT_CHANNEL_METADATA) { > >> + err = -EINVAL; > >> + goto traces_error; > >> + } > >> + > >> + if (enable) > >> + trace->setting.channels[channel].flags |= > >> + CHANNEL_FLAG_ENABLE; > >> + else > >> + trace->setting.channels[channel].flags &= > >> + ~CHANNEL_FLAG_ENABLE; > >> + > >> +traces_error: > >> + ltt_unlock_traces(); > >> + return err; > >> +} > >> +EXPORT_SYMBOL_GPL(ltt_trace_set_channel_enable); > >> + > >> int ltt_trace_set_channel_overwrite(const char *trace_name, > >> const char *channel_name, unsigned overwrite) > >> { > >> @@ -563,6 +621,12 @@ int ltt_trace_set_channel_overwrite(const char *trace_name, > >> goto traces_error; > >> } > >> > >> + /* > >> + * Always put the metadata channel in non-overwrite mode : > >> + * This is a very low traffic channel and it can't afford to have its > >> + * data overwritten : this data (marker info) is necessary to be > >> + * able to read the trace. > >> + */ > >> if (overwrite && channel == LTT_CHANNEL_METADATA) { > >> err = -EINVAL; > >> goto traces_error; > >> @@ -635,14 +699,11 @@ int ltt_trace_alloc(const char *trace_name) > >> * Config each channel's default buffersize/cnt, instead of > >> * LTT_DEFAULT_SUBBUF_SIZE_LOW, ... > >> */ > >> - > >> - /* > >> - * Always put the metadata channel in non-overwrite mode : > >> - * This is a very low traffic channel and it can't afford to have its > >> - * data overwritten : this data (marker info) is necessary to be > >> - * able to read the trace. > >> - */ > >> for (chan = 0; chan < NR_LTT_CHANNELS; chan++) { > >> + if (!(trace->setting.channels[chan].flags > >> + & CHANNEL_FLAG_ENABLE)) > >> + continue; > >> + > >> subbuf_size = trace->setting.channels[chan].subbuf_size; > >> subbuf_cnt = trace->setting.channels[chan].subbuf_cnt; > >> prepare_chan_size_num(&subbuf_size, &subbuf_cnt, > >> @@ -724,6 +785,10 @@ static int ltt_trace_create(const char *trace_name, const char *trace_type, > >> if (IS_ERR_VALUE(err)) > >> return err; > >> > >> + err = ltt_trace_set_channel_enable(trace_name, LTT_METADATA_CHANNEL, 1); > >> + if (IS_ERR_VALUE(err)) > >> + return err; > >> + > >> err = ltt_trace_set_channel_overwrite(trace_name, > >> LTT_METADATA_CHANNEL, > >> is_channel_overwrite(LTT_CHANNEL_METADATA, mode)); > >> @@ -740,6 +805,11 @@ static int ltt_trace_create(const char *trace_name, const char *trace_type, > >> if (IS_ERR_VALUE(err)) > >> return err; > >> > >> + err = ltt_trace_set_channel_enable(trace_name, LTT_INTERRUPTS_CHANNEL, > >> + 1); > >> + if (IS_ERR_VALUE(err)) > >> + return err; > >> + > >> err = ltt_trace_set_channel_overwrite(trace_name, > >> LTT_INTERRUPTS_CHANNEL, > >> is_channel_overwrite(LTT_CHANNEL_INTERRUPTS, mode)); > >> @@ -756,6 +826,11 @@ static int ltt_trace_create(const char *trace_name, const char *trace_type, > >> if (IS_ERR_VALUE(err)) > >> return err; > >> > >> + err = ltt_trace_set_channel_enable(trace_name, LTT_PROCESSES_CHANNEL, > >> + 1); > >> + if (IS_ERR_VALUE(err)) > >> + return err; > >> + > >> err = ltt_trace_set_channel_overwrite(trace_name, > >> LTT_PROCESSES_CHANNEL, > >> is_channel_overwrite(LTT_CHANNEL_PROCESSES, mode)); > >> @@ -772,6 +847,10 @@ static int ltt_trace_create(const char *trace_name, const char *trace_type, > >> if (IS_ERR_VALUE(err)) > >> return err; > >> > >> + err = ltt_trace_set_channel_enable(trace_name, LTT_MODULES_CHANNEL, 1); > >> + if (IS_ERR_VALUE(err)) > >> + return err; > >> + > >> err = ltt_trace_set_channel_overwrite(trace_name, > >> LTT_MODULES_CHANNEL, > >> is_channel_overwrite(LTT_CHANNEL_MODULES, mode)); > >> @@ -788,6 +867,10 @@ static int ltt_trace_create(const char *trace_name, const char *trace_type, > >> if (IS_ERR_VALUE(err)) > >> return err; > >> > >> + err = ltt_trace_set_channel_enable(trace_name, LTT_NETWORK_CHANNEL, 1); > >> + if (IS_ERR_VALUE(err)) > >> + return err; > >> + > >> err = ltt_trace_set_channel_overwrite(trace_name, > >> LTT_NETWORK_CHANNEL, > >> is_channel_overwrite(LTT_CHANNEL_NETWORK, mode)); > >> @@ -804,6 +887,10 @@ static int ltt_trace_create(const char *trace_name, const char *trace_type, > >> if (IS_ERR_VALUE(err)) > >> return err; > >> > >> + err = ltt_trace_set_channel_enable(trace_name, LTT_CPU_CHANNEL, 1); > >> + if (IS_ERR_VALUE(err)) > >> + return err; > >> + > >> err = ltt_trace_set_channel_overwrite(trace_name, > >> LTT_CPU_CHANNEL, is_channel_overwrite(LTT_CHANNEL_CPU, mode)); > >> if (IS_ERR_VALUE(err)) > >> @@ -855,12 +942,16 @@ traces_error: > >> /* Sleepable part of the destroy */ > >> static void __ltt_trace_destroy(struct ltt_trace_struct *trace) > >> { > >> - trace->ops->finish_channel(trace->channel.metadata); > >> - trace->ops->finish_channel(trace->channel.interrupts); > >> - trace->ops->finish_channel(trace->channel.processes); > >> - trace->ops->finish_channel(trace->channel.modules); > >> - trace->ops->finish_channel(trace->channel.network); > >> - trace->ops->finish_channel(trace->channel.cpu); > >> + int i; > >> + struct ltt_channel_struct *chan; > >> + > >> + for (i = 0; i < NR_LTT_CHANNELS; i++) { > >> + chan = *(struct ltt_channel_struct **)((char *)trace > >> + + chan_infos[i].channel_index); > > > > Same here, wrapper should be used > > > >> + /* chan is NULL if it is not traced */ > >> + if (chan) > >> + trace->ops->finish_channel(chan); > >> + } > >> > >> flush_scheduled_work(); > >> > >> @@ -871,12 +962,13 @@ static void __ltt_trace_destroy(struct ltt_trace_struct *trace) > >> */ > >> trace_async_wakeup(trace); > >> > >> - trace->ops->remove_channel(trace->channel.metadata); > >> - trace->ops->remove_channel(trace->channel.interrupts); > >> - trace->ops->remove_channel(trace->channel.processes); > >> - trace->ops->remove_channel(trace->channel.modules); > >> - trace->ops->remove_channel(trace->channel.network); > >> - trace->ops->remove_channel(trace->channel.cpu); > >> + for (i = 0; i < NR_LTT_CHANNELS; i++) { > >> + chan = *(struct ltt_channel_struct **)((char *)trace > >> + + chan_infos[i].channel_index); > > > > And here. > > > > I'll merge your two patches and create the wrapper in a following patch. > > It will be in the next LTTng version. Thanks! > > > > Mathieu > > > >> + /* chan is NULL if it is not traced */ > >> + if (chan) > >> + trace->ops->remove_channel(chan); > >> + } > >> > >> kref_put(&trace->ltt_transport_kref, ltt_release_transport); > >> > >> diff --git a/ltt/probes/ltt-type-serializer.c b/ltt/probes/ltt-type-serializer.c > >> index 1f93181..317d329 100644 > >> --- a/ltt/probes/ltt-type-serializer.c > >> +++ b/ltt/probes/ltt-type-serializer.c > >> @@ -55,6 +55,10 @@ notrace void _ltt_specialized_trace(void *probe_data, > >> rflags = 0; > >> #endif > >> channel = ltt_get_channel_from_index(trace, channel_index); > >> + if (!channel) > >> + /* channel is NULL if it is not enabled */ > >> + continue; > >> + > >> /* reserve space : header and data */ > >> ret = ltt_reserve_slot(trace, channel, &transport_data, > >> data_size, &slot_size, &buf_offset, > >> -- > >> 1.5.5.3 > >> > >> > > > > -- > > Mathieu Desnoyers > > OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 > > > > > -- Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68