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

[Zhaolei]

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"?

>> 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?

>> 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?

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

>> 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.

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

B.R.
Zhaolei