[ltt-dev] new ltt-trace-control module based on debugfs(v2)

[Mathieu Desnoyers]

* Zhaolei (zhaolei at cn.fujitsu.com) wrote:
> Hello, Mathieu
> 
> Thanks for your review.
> 
> Here is my answer for your review in different mails.
> 
> >> > Hrm, I think it will cause a double declaration of ltt_root_dentry if
> >> > both ltt-relay and ltt-relay-locked are loaded. ltt_root_dentry should
> >> > probably sit in ltt-core.c.
> >> ltt_root_dentry is moved into ltt-core.c.
> >> But I think it is ltt-core.c's duty to create ltt-root in debugfs(not ltt-relay)
> >> ltt-relay and ltt-relay-locked's dir should be in ltt-root, so dir should be:
> >> debugfs/
> >> `-- ltt
> >>     |-- control
> >>     |   `-- ...
> >>     |-- relay
> >>     |   `-- ...
> >>     `-- relay-locked
> >>         `-- ...
> >> Maybe it will also affect user-mode tools, I'd like to impl it after new
> >> ltt-trace-control get merged.
> >> 
> > 
> > OK, as long as we don't have duplicate definitions for ltt_root_dentry,
> > we can proceed incrementally.
> Sorry for my poor English, I don't understand your meaning.
> What means "we can proceed incrementally"?
> 

"we can merge the patches step by step" or "we don't have to do
everything all at once".


> >> Todo:
> >>   Create ltt_root_dentry in ltt-core.c or ltt-trace.c
> >>   (now in ltt-relay.c)
> >> 
> > 
> > Hrm, that won't work. You can compile in ltt-relay-locked.ko but not
> > ltt-relay.ko. In this case, ltt-relay-locked will try to use an
> > uninitialized ltt_root_dentry. It really has to be initialized in
> > ltt-core.c.
> > 
> In code with this patch,  if we compile in ltt-relay-locked.ko but not ltt-relay.ko,
> ltt-relay-locked will initialize ltt_root_dentry itself.
>   ltt-relay-locked.c 1594:
>   ltt_root_dentry = debugfs_create_dir(LTT_RELAY_LOCKED_ROOT, NULL);
> So it is not problem.
> 
> The real problem is:
> When we compile in both ltt-relay-locked.ko and ltt-relay.ko,
> (for example, ltt-relay initialize first)
> ltt-relay will inititlize debugfs/ltt/, now ltt_root_dentry is debugfs/ltt/.
> and then, ltt-relay-locked will inititlize debugfs/ltt-locked/, now ltt_root_dentry is debugfs/ltt-locked/.
> After system starts, if we add a trace in ltt-relay, it will call ltt-relay's ltt_relay_create_dirs, but will
> create dir in debugfs/ltt-locked/ because ltt_root_dentry is debugfs/ltt-locked/.
> 
> I think final solution is make ltt-relay and ltt-relay-locked's root dir in debugfs/ltt/ as:
> `-- ltt
>     |-- control
>     |   `-- ...
>     |-- relay
>     |   `-- ...
>     `-- relay-locked
>          `-- ...
> 
> But because it will affact user-mode tools, I think we'd better solve problem one by one.
> (after we complish this todo, we can begin to change ltt-relay's root dir)
> 
> Now, maybe we can use a compatible way:
> in ltt-core:
> struct dentry *ltt_root_dentry;
> 
> struct dentry *get_ltt_debug_root() {
>     if (!ltt_root_dentry) 
>         ltt_root_dentry = debugfs_create_dir("ltt", NULL);
>     return ltt_root_dentry;
> }
> EXPORT_SYMBOL_GPL(get_ltt_debug_root);
> 
> in ltt-relay's init:
> do nothing
> 
> in ltt-relay's ltt_relay_create_dirs:
> use get_debug_root()  to get dentry of debugfs/ltt
> 
> ltt-relay-locked is not changed(same as no-patched code)
> 
> By this way, dir hiberarchy is not changed.(so we need not modify user-mode tools now)
> But ltt-trace-control.ko can get dentry of debugfs/ltt by calling get_ltt_debug_root().
> 
> In future, both ltt-relay and ltt-relay-locked will do following in init()
> ltt_relay_dentry = debugfs_create_dir(LTT_RELAY_ROOT, get_ltt_debug_root());
> (OR ltt_relay_dentry = debugfs_create_dir(LTT_RELAY_LOCKED_ROOT, get_ltt_debug_root());)
> 
> What is your opinion?
> 

Yes, that's fine with me. I just wonder why we would not create the
debugfs ltt/ directory directly in ltt-core.c init() ?

> >> if (IS_ERR_VALUE(err)) {
> >> printk(KERN_ERR "subbuf_num_write: ltt_trace_set_channel_subbufcount failed: %d\n",
> > 
> > This is probably over 80 columns. Maybe you should try
> > scripts/checkpatch.pl.
> I checked patch with scripts/checkpatch.pl.
> 
> But this line is because string is too long, I can separate string to make check pass, but IMHO,
> line-length is not a very strong rule, if no-separate make source looks better, maybe we can
> ignore this warning.
> 
> btw, get rid of this warning is also a good idea, so, what is your opinion?
> 

I habitually try to split my strings so they fit in 80 cols. e.g :

    printk(KERN_ERR "subbuf_num_write: "
                    "ltt_trace_set_channel_subbufcount failed: %d\n",


> >> if (1 != sscanf(buf, "%s", cmd)) {
> > 
> > Typically one would write
> > 
> > if (sscanf(buf, "%s", cmd) != 1) {
> Thanks, I will fix it.
> 

Yep, make sure you look around, at other sites where this is used.

> >> static const char *channels[] = {
> >> "cpu",
> >> "processes",
> >> "interrupts",
> >> "network",
> >> "modules",
> >> "metadata"
> >> };
> > 
> > Declaring this outside of the scope of the function would be preferred.
> Thanks, I will fix it.
> 
> >> tmp_den = debugfs_create_file("alloc", 0200, trace_root, NULL,
> > 
> > 0200 -> maybe we should use the standard macros for this.
> Thanks, I will fix it.
> 
> >> + trace->setting.channels[channel].overwrite = overwrite ? 1 : 0;
> >> +
> > 
> > Nitpick : could use !!overwrite   :)
> Thanks, I will fix it.
> 
> >>  err = ltt_trace_alloc(trace_name);
> >>  if (IS_ERR_VALUE(err))
> >>  return err;
> > 
> > A for() loop is clearly needed here...
> Because in this function, subbuf_size/cnt choiced from subbuf_low/mid/high.
> It is different for each channel.
> Yes, we are able to make a loop and use other way to deal with it.
> But IMHO, this function is for compatible with netlink interface, and will be deleted
> in future. So, make it easy and stupid (but right) it not a bad choose.
> 

OK, given it's temporary, it can live :)

> After I hear your opinion of this mail, i'll begin to make a v4 patch.
> 

Thanks for the good work !

Mathieu

> B.R.
> Zhaolei
> _______________________________________________
> 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