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

* [ltt-dev] [PATCH] Separate ltt_trace_create()intoltt_trace_create() and ltt_trace_alloc()
       [not found] <B42BD3DB473F4EA5AC9BE686AEF2338E@zhaoleiwin>
@ 2008-10-30 14:49 ` Mathieu Desnoyers
  2008-10-31  1:13   ` [ltt-dev] [PATCH] Separateltt_trace_create()intoltt_trace_create() " Zhaolei
  2008-10-31  2:00   ` Zhaolei
  0 siblings, 2 replies; 3+ messages in thread
From: Mathieu Desnoyers @ 2008-10-30 14:49 UTC (permalink / raw)

* Zhaolei (zhaolei at cn.fujitsu.com) wrote:
> Hello, Mathieu
> 
> I forgot that i can't send mail to compudj at krystal.dyndns.org.
> So I follow it yo you.
> 
> Another thing:
> 
> `-- debugfs
>     `-- ltt
>         |-- buffers
>         |   `-- ...
>         |-- control
>         |   `-- trace1
>         |       |-- alloc
>         |       |-- channel
>         |       |   |-- control
>         |       |   |   |-- interrupts
>         |       |   |   |   |-- subbuf_num
>         |       |   |   |   `-- subbuf_size
>         |       |   |   |-- metadata
>         |       |   |   |   |-- subbuf_num
>         |       |   |   |   `-- subbuf_size
>         |       |   |   |-- modules
>         |       |   |   |   |-- subbuf_num
>         |       |   |   |   `-- subbuf_size
>         |       |   |   |-- network
>         |       |   |   |   |-- subbuf_num
>         |       |   |   |   `-- subbuf_size
>         |       |   |   `-- processes
>         |       |   |       |-- subbuf_num
>         |       |   |       `-- subbuf_size
>         |       |   `-- cpu
>         |       |       |-- mode
>         |       |       |-- subbuf_num
>         |       |       `-- subbuf_size
>         |       `-- enabled
>         `-- setup_trace
> 
> It lakes control-file for destroy a trace and setup trace_type.
> So I think we should change tree struct as follow,
> Do you hace some comment?
> 
> `-- debugfs
>     `-- ltt
>         |-- buffers
>         |   `-- ...
>         |-- control
>         |   `-- trace1
>         |       |-- alloc

echo 1 > alloc -> allocate
echo 0 > alloc -> free ?

>         |       |-- channel
>         |       |   |-- control
>         |       |   |   |-- interrupts
>         |       |   |   |   |-- subbuf_num
>         |       |   |   |   `-- subbuf_size
                              \-- overwrite (1/0)
>         |       |   |   |-- metadata
>         |       |   |   |   |-- subbuf_num
>         |       |   |   |   `-- subbuf_size
                              \-- overwrite
>         |       |   |   |-- modules
>         |       |   |   |   |-- subbuf_num
>         |       |   |   |   `-- subbuf_size
                              \-- overwrite
>         |       |   |   |-- network
>         |       |   |   |   |-- subbuf_num
>         |       |   |   |   `-- subbuf_size
                              \-- overwrite
>         |       |   |   `-- processes
>         |       |   |       |-- subbuf_num
>         |       |   |       `-- subbuf_size
                              \-- overwrite
>         |       |   `-- cpu
>         |       |       |-- mode
>         |       |       |-- subbuf_num
>         |       |       `-- subbuf_size
>         |       |-- enabled
>         |       `-- type ******new******

Nope, the overwrite above will be more flexible. Then people can select
per-channel which one they want in flight recorder mode (overwrite = 1).
Default would be non-flight (normal mode, overwrite = 0). Hybrid is
really just a combination of the two.

Note that the metadata channel must be forced to "normal" mode. This is
a special-case.

>         |-- setup_trace
>         `-- destroy_trace ******new******

Not needed if we echo 0 > alloc.

Thanks,

Mathieu

> 
> 
> B.R.
> Zhaolei
> Date: Wed, 29 Oct 2008 12:04:30 +0800
> To: Mathieu Desnoyers <compudj at krystal.dyndns.org>
> Cc: ltt-dev at lists.casi.polymtl.ca
> X-Mailer: Microsoft Outlook Express 6.00.2900.5512
> From: Zhaolei <zhaolei@cn.fujitsu.com>
> Subject: Re: [ltt-dev] [PATCH] Separate ltt_trace_create()
> 	intoltt_trace_create() and ltt_trace_alloc()
> 
> * 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
> > 
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
> 
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

^ permalink raw reply	[flat|nested] 3+ messages in thread