[ltt-dev] [PATCH 2/2] Make ltt support enable/disable each channel

Mathieu Desnoyers mathieu.desnoyers at polymtl.ca
Wed Dec 3 00:35:48 EST 2008 

* Zhaolei (zhaolei at cn.fujitsu.com) wrote:
> * From: "Mathieu Desnoyers" <mathieu.desnoyers at polymtl.ca>
> >* 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 <zhaolei at cn.fujitsu.com>
> >> ---
> >>  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, &ltt_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

More information about the lttng-dev mailing list