[ltt-dev] [PATCH] Separate ltt_trace_create()intoltt_trace_create() and ltt_trace_alloc()
Mathieu Desnoyers
mathieu.desnoyers at polymtl.ca
Thu Oct 30 10:49:44 EDT 2008
* 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 at 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
More information about the lttng-dev
mailing list