From mboxrd@z Thu Jan  1 00:00:00 1970
From: zhaolei@cn.fujitsu.com (Zhaolei)
Date: Wed, 3 Dec 2008 08:46:06 +0800
Subject: [ltt-dev] [PATCH 2/2] Make ltt support enable/disable each
	channel
References: <49351A0F.5060500@cn.fujitsu.com> <49351A77.5070808@cn.fujitsu.com>
	<20081202170647.GD19095@Krystal>
Message-ID: <232931D7C1804812BF50E571921A750B@zhaoleiwin>

* From: "Mathieu Desnoyers" <mathieu.desnoyers@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).

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
> 
>