[ltt-dev] [PATCH] Separate ltt_trace_create() intoltt_trace_create() and ltt_trace_alloc()

[zhaolei@cn.fujitsu.com (Zhaolei)]

* Mathieu Desnoyers <compudj at krystal.dyndns.org> wrote:
>* Zhaolei (zhaolei at cn.fujitsu.com) wrote:
>> This is step1 of:
>> "switch marker list and marker activation (currently in /proc/ltt) to debugfs"
>> 
> 
> Excellent start ! :)
> 
> I think you are in the right direction. See below,
Hello, Mathieu

Thanks for your suggestion.
We will fix step1 on your suggestion and continue to do step2.

But I have some question below:

>>  struct ltt_traces {
>> - struct list_head head; /* Traces list */
>> + struct list_head prealloc_head; /* Pre-allocated traces list */
> 
> pre-allocated sounds like "let's allocate 10 entries before the user
> asks for the first one". I think "setup_head" would be a better name.
Thanks, I agree.

>> @@ -218,6 +235,12 @@ struct ltt_trace_struct {
>>  struct ltt_channel_struct *modules;
>>  struct ltt_channel_struct *metadata;
>>  } channel;
>> + struct {
>> + struct {
>> + unsigned subbuf_size;
>> + unsigned subbuf_cnt;
>> + } channels[NR_LTT_CHANNELS];
> 
> You could probably put the
>   struct ltt_channel_struct       *channel;
> 
>   in there.
> 
>> + } setting;
> 
> setting seems not really useful here.
> 
>  struct ltt_channel_struct channel[NR_LTT_CHANNELS];
>   
> And adding subbuf_size and nr_subbufs fields to struct
> ltt_channel_struct seems better.   
But channel is allocated in other function as ltt_relay_create_channel:
static int ltt_relay_create_channel(char *trace_name,
 struct ltt_trace_struct *trace, struct dentry *dir,
 char *channel_name, struct ltt_channel_struct **ltt_chan,
 unsigned int subbuf_size, unsigned int n_subbufs,
 int overwrite)
{
 ...
 *ltt_chan = kzalloc(sizeof(struct ltt_channel_struct), GFP_KERNEL);
 ...

If we define struct ltt_channel_struct channel[NR_LTT_CHANNELS](now is pointer)
We need to modify ltt_relay_create_channel and other functions and make
 things complex.

So, IMHO, we can use current struct at present, and select to impl your idea
 after "switch marker list and marker activation to debugfs" finished.

>> +/*
>> + * 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
>> + */
>> +static char *chan_names[] = {
>> + LTT_CPU_CHANNEL,
>> + LTT_PROCESSES_CHANNEL,
>> + LTT_INTERRUPTS_CHANNEL,
>> + LTT_NETWORK_CHANNEL,
>> + LTT_MODULES_CHANNEL,
>> + LTT_METADATA_CHANNEL,
>> +};
>> +
> 
> If you add a const char *name field into struct ltt_channel_struct, you won't
> need this.
It have relation with prev qusetion.
If we don't define struct ltt_channel_struct channel[NR_LTT_CHANNELS]
 in ltt_trace_struct, we need to get channel-name from channel-id by this struct.
But it is a temp struct, because it is better to use a unifed struct for both
 ltt-tracer.c and ltt-marker-control.c

> > Those below should really be turned into a loop once you can simply
> iterate on the channel [] elements in the trace setup structure.
Thanks, I agree.

> 
> 
> Mathieu
>