From mboxrd@z Thu Jan 1 00:00:00 1970 From: mathieu.desnoyers@polymtl.ca (Mathieu Desnoyers) Date: Thu, 30 Oct 2008 10:49:44 -0400 Subject: [ltt-dev] [PATCH] Separate ltt_trace_create()intoltt_trace_create() and ltt_trace_alloc() In-Reply-To: References: Message-ID: <20081030144944.GB13812@Krystal> * 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 > Cc: ltt-dev at lists.casi.polymtl.ca > X-Mailer: Microsoft Outlook Express 6.00.2900.5512 > From: Zhaolei > Subject: Re: [ltt-dev] [PATCH] Separate ltt_trace_create() > intoltt_trace_create() and ltt_trace_alloc() > > * Mathieu Desnoyers 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