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

[Mathieu Desnoyers]

* Mathieu Desnoyers (compudj at krystal.dyndns.org) wrote:
> 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)
> > 

Oops, forgot to answer to this one :

Yes, the "mode" concept can go away form the kernel, it's not required
anymore. Having one "overwrite" per channel is a superset of "mode"
(flight, normal, hybrid) which is basically just fixing overwrite modes
to specific channels.

If you plan to leave a "mode" file for transition (I don't think it is
necessary), it should not go into
debugfs/control/trace_name/channel/cpu/mode

but rather in

debugfs/control/trace_name/mode

because it fixes the mode for the whole trace, not only for a particular
channel.

Mathieu


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