[ltt-dev] [PATCH v3 4/4] Add ltt-trace-control module

[Mathieu Desnoyers]

Hi Zhaolei,

* Zhaolei (zhaolei at cn.fujitsu.com) wrote:
> Hello, Mathieu
> 
> Thanks for your review.
> 
> Here is my answer for your review in different mails.
> 
> >> Todo:
> >>   Make ltt-relay and ltt-relay-locked to create their dir in ltt's root dir as:
> >>   debugfs/ltt/relay and debugfs/ltt/relay-locked
> >> 
> > 
> > Maybe it's not that important to do the distinction between relay and
> > relay-locked using subdirectories. It will just confuse users.
> > 
> > The only thing I care about is that if a user creates a trace backed
> > with "relay" and then another trace with the same name, but backed with
> > "relay-locked", it does not crash the kernel and return the appropriate
> > error message. I think it will all work fine as-is now.
> Thanks, I agree.
> 
> >> +static inline struct dentry *dir_lookup(struct dentry *parent,
> > 
> > static should be enough here, without "inline". The compiler will inline
> > it anyway. It's mostly a matter of habits : generally, inline is only
> > used for headers. Given this is a C file, let's let the compiler decide
> > if it wants to inline it or not.
> Thanks, I agree.
> 
> >> + switch (cmd[0]) {
> >> + case 'Y':
> >> + case 'y':
> >> + case '1':
> >> + err = ltt_trace_start(
> >> + file->f_dentry->d_parent->d_name.name);
> > 
> > does it need to be on 2 lines ? (80 cols ?)
> Thanks, I agree.
> 
> >> + /*
> >> + * ltt_trace_start return +ERRNO,
> >> + * so we can't use IS_ERR_VALUE(err)
> >> + */
> >> + if (err) {
> >> + printk(KERN_ERR
> >> + "enabled_write: ltt_trace_start failed: %d\n",
> >> + err);
> >> + err = -EPERM;
> >> + goto err_start_trace;
> >> + }
> >> + break;
> >> + case 'N':
> >> + case 'n':
> >> + case '0':
> >> + err = ltt_trace_stop(
> >> + file->f_dentry->d_parent->d_name.name);
> > 
> > does it need to be on 2 lines ?
> > 
> Thanks, I agree.
> 
> >> + if (!i) {
> > 
> > Hrm ? What is special about i == 0 ?
> > 
> >> + /* debugfs/control/trace_name/channel/cpu/mode */
> >> + tmp_den = debugfs_create_file("mode", S_IWUSR,
> >> + channel_den, NULL, &ltt_mode_operations);
> Because we need to create control file of
> ebugfs/control/trace_name/channel/cpu/mode only in cpu channel's dir as your
> suggestion.
> 
> But IMHO, mode is not necessary now, because we can control each channel's
> overwrite.
> So maybe I'd like to remove conception of "mode" in ltt's kernel source.
> It will delete some struct member, some function, and modify some function,
> so we can do it with another patch.
> User-mode tools can have conception of "mode". They can set each channel's
> overwrite attribute by mode.
> (OR let user-mode tools get rid of  conception of mode, and control each
> channel's overwrite attribute directly)
> 
> I am considering how to make user-mode tools both flexible and simple by use
> new ltt-trace-control.ko.
> 1: Is user-mode tools need to know "setup" period of a trace
> 2: Is user-mode tools need to control each channel's subbuf cnt and size
> 3: Is user-mode tools need to control each channel's overwrite
> 4: Is user-mode tools need to select trace's channel(relay or relay-locked)
> (Current version of lttctl is "no" for all above questions)

For question 4, ltt-control currently supports specifying the transport
(either relay or relay-locked) with -T.

-T            Type of trace (ex. relay)

> 
> More powerful control is not bad, but maybe have risk of makeing our user
> hard to understand.
> 
> What about your opinion?
> 

It's up to the userspace tools to make it simple to users. Let's keep
all the flexibility we can within the kernel.

Also, don't spend too much time making a compatible in-kernel API when
it would be faster to just modify the userspace tools. It its current
phase, the LTTng project does not claims to have compatible
kernel-userspace interfaces between releases. The list of compabile
versions is provided in the compatibility list on the website.
Therefore, we can leverage this particularity to speed up development.

> >> + err = ltt_trace_destroy(trace_name);
> >> + /*
> >> + * ltt_trace_destroy return +ERRNO,
> >> + * so we can't use IS_ERR_VALUE(err)
> > 
> > ltt_trace_destroy returns a int, not a pointer to some variable which
> > can have to take negative value to be an error. In this case, just
> > checking for non-zero return value is OK. Do we really need to leave
> > this comment here ?
> Thanks, I'll remove these comments.
> (But IMHO, return -ERRNO is better, because it is more common in kernel,
> and we can use IS_ERR_VALUE to check it.)
> 

Hrm, if it's the case, then we should change LTTng to make it more
standard.

Mathieu

> >> +module_init(ltt_trace_control_init);
> >> +module_exit(ltt_trace_control_exit);
> >> +
> >> +MODULE_LICENSE("GPL");
> > 
> > You could add :
> > 
> > MODULE_AUTHOR(...);
> > MODULE_DESCRIPTION(...);
> Thanks, I'll add it.
> 
> 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