by Tom Zanussi

[permalink] [raw]

Subject: Re: [PATCH v4 02/11] tracing: add basic event trigger framework

On Thu, 2013-08-08 at 17:30 +0900, Masami Hiramatsu wrote:
> (2013/07/30 1:40), Tom Zanussi wrote:
> > +struct event_command {
> > + struct list_head list;
> > + char *name;
> > + enum trigger_mode trigger_mode;
> > + bool post_trigger;
> > + int (*func)(struct event_command *cmd_ops,
> > + void *cmd_data, char *glob, char *cmd,
> > + char *params, int enable);
> > + int (*reg)(char *glob,
> > + struct event_trigger_ops *trigger_ops,
> > + void *trigger_data, void *cmd_data);
> > + void (*unreg)(char *glob,
> > + struct event_trigger_ops *trigger_ops,
> > + void *trigger_data, void *cmd_data);
> > + int (*set_filter)(char *filter_str,
> > + void *trigger_data,
> > + void *cmd_data);
>
> I think you should pass trace_event_file *file (see below) instead of ambiguous
> void *cmd_data, because all handler implementations expect that.
>
>
> [...]
> > +#include <linux/module.h>
> > +#include <linux/ctype.h>
> > +#include <linux/mutex.h>
> > +#include <linux/slab.h>
> > +
> > +#include "trace.h"
> > +
> > +static LIST_HEAD(trigger_commands);
> > +static DEFINE_MUTEX(trigger_cmd_mutex);
> > +
> > +struct event_trigger_data {
> > + struct ftrace_event_file *file;
> > + unsigned long count;
> > + int ref;
> > + bool enable;
> > + struct event_trigger_ops *ops;
> > + enum trigger_mode mode;
> > + struct event_filter *filter;
> > + char *filter_str;
> > + struct list_head list;
> > +};
> > +
> > +struct trigger_iterator {
> > + struct ftrace_event_file *file;
> > +};
>
> Why would you define this trigger_iterator even if it has
> only one member? This means all iterators can be replaced
> by the file. I'd like to keep it simple.
>

Right, this and the void *cmd_data are vestiges of the original patchset
where I tried to keep the ftrace_triggers and event_triggers unified.
This was meant to be a common iterator abstration which obviously is no
longer needed and has been removed in the new patchset.

> > +
> > +void event_triggers_call(struct ftrace_event_file *file)
> > +{
> > + struct event_trigger_data *data;
> > +
> > + if (list_empty(&file->triggers))
> > + return;
> > +
> > + preempt_disable_notrace();
> > + list_for_each_entry_rcu(data, &file->triggers, list)
> > + data->ops->func((void **)&data);
> > + preempt_enable_notrace();
> > +}
> > +EXPORT_SYMBOL_GPL(event_triggers_call);
> > +
> > +static void *trigger_next(struct seq_file *m, void *t, loff_t *pos)
> > +{
> > + struct trigger_iterator *iter = m->private;
> > +
> > + return seq_list_next(t, &iter->file->triggers, pos);
> > +}
> > +
> > +static void *trigger_start(struct seq_file *m, loff_t *pos)
> > +{
> > + struct trigger_iterator *iter = m->private;
> > +
> > + mutex_lock(&event_mutex);
> > +
> > + return seq_list_start(&iter->file->triggers, *pos);
> > +}
>
> By Oleg's bugfixes, we are now using event_file_data(filp)
> please refer the f_start/trace_format_open implementation
> which is the closest usage of the trigger file.
>
>
> > +static int event_trigger_regex_open(struct inode *inode, struct file *file)
> > +{
> > + struct trigger_iterator *iter;
> > + int ret = 0;
> > +
> > + iter = kzalloc(sizeof(*iter), GFP_KERNEL);
> > + if (!iter)
> > + return -ENOMEM;
> > +
> > + mutex_lock(&event_mutex);
> > +
> > + iter->file = inode->i_private;
> > +
> > + if (file->f_mode & FMODE_READ) {
> > + ret = seq_open(file, &event_triggers_seq_ops);
> > + if (!ret) {
> > + struct seq_file *m = file->private_data;
> > + m->private = iter;
> > + } else {
> > + /* Failed */
> > + kfree(iter);
> > + }
> > + } else
> > + file->private_data = iter;
> > +
> > + mutex_unlock(&event_mutex);
> > +
> > + return ret;
> > +}
>
> As you can see in trace_format_open(), now file->private_data and
> m->private will point struct file *, don't need to allocate something.
>

Right, and this has all been simplified now too as a result..

> > +static ssize_t event_trigger_regex_write(struct file *file,
> > + const char __user *ubuf,
> > + size_t cnt, loff_t *ppos, int enable)
> > +{
> > + struct trigger_iterator *iter = file->private_data;
> > + ssize_t ret;
> > + char *buf;
> > +
> > + if (!cnt)
> > + return 0;
> > +
> > + if (cnt >= PAGE_SIZE)
> > + return -EINVAL;
> > +
> > + if (file->f_mode & FMODE_READ) {
> > + struct seq_file *m = file->private_data;
> > + iter = m->private;
> > + } else
> > + iter = file->private_data;
> > +
> > + buf = (char *)__get_free_page(GFP_TEMPORARY);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + if (copy_from_user(buf, ubuf, cnt)) {
> > + free_page((unsigned long) buf);
> > + return -EFAULT;
> > + }
> > + buf[cnt] = '\0';
> > + strim(buf);
> > +
> > + ret = trigger_process_regex(iter, buf, enable);
>
> You also need to get event_mutex while processing event_file to
> prevent unexpected releasing.
>

Thanks for catching that too.

> > +
> > + free_page((unsigned long) buf);
> > + if (ret < 0)
> > + goto out;
> > +
> > + *ppos += cnt;
> > + ret = cnt;
> > + out:
> > + return ret;
> > +}
>
> Thank you,
>

Thank you.

Tom

2013-08-22 23:26:37

by Tom Zanussi

[permalink] [raw]

Subject: Re: [PATCH v4 00/11] tracing: trace event triggers

On Thu, 2013-08-22 at 14:48 -0500, Tom Zanussi wrote:
> Hi Masami,
>
> Just getting back to this after returning from vacation - I'll be
> sending an update to this patchset addressing your comments shortly...
>
> On Thu, 2013-08-08 at 11:02 +0900, Masami Hiramatsu wrote:
> > Hi,
> >
> > (2013/07/30 1:40), Tom Zanussi wrote:
> > > Hi,
> > >
> > > This is v4 of the trace event triggers patchset, addressing more
> > > comments from Masami Hiramatsu (thanks for the review and comments).
> > >
> > > One of Masami's comments was on event_trigger_regex_open's use of
> > > inode->i_private and that the same problem was being worked on by Oleg
> > > Nesterov in other places. That still seems to be the case, but in
> > > order to address that, this patchset is built on top of the current
> > > linux-trace/for-next but also including v2 of Oleg Nesterov's tracing:
> > > open/delete fixes (but with v3 of the 6/6 patch).
> >
> > Does this patchset supports multibuffer? It seems that setting a
> > trigger in an event of an instance affects the default event, but not
> > the instance's event.
>
> You're right of course - I went through the trouble of fixing up the
> event filters to better support multibuffer, but neglected the
> triggers. :-( But as you point out in a later comment, the fix is
> simple and I've updated the patchset to do that..
>
> > e.g.
> >
> > # mkdir instances/hoge
> > # echo 'enable_event:mce:mce_record' > instances/hoge/events/syscalls/sys_enter_symlink/trigger
> > # cat instances/hoge/events/syscalls/sys_enter_symlink/enable
> > 0*
> > # cat instances/hoge/events/mce/mce_record/enable
> > 0
> > # cat events/mce/mce_record/enable
> > 0*
> > # ln -sf /dev/null /tmp
> > # cat instances/hoge/events/mce/mce_record/enable
> > 0
> > # cat events/mce/mce_record/enable
> > 1*
> >
> > This looks odd, I expected enabling mce/mce_record under instances/hoge.
> >
> > And, there is a bug of ftrace itself (not introduced by this series) I've found.
> > After the above operation, we can delete the instance "hoge", but the soft-mode
> > flag of mce_record is not cleared, even though there is no trigger referring
> > the event.
> >
> > # rmdir instances/hoge
> > # cat events/mce/mce_record/enable
> > 1*
> >
> > This is because the ftrace actually failed to remove(disable) the event trigger
> > associated with the instance when doing rmdir, but it just removed that interface.
> >
> > > v4:
> > > - made some changes to the soft-disable for syscall patch, according
> > > to Masami's suggestions. Actually, since there's now an array of
> > > ftrace_files for syscalls that can serve the same purpose, the
> > > enabled_enter/exit_syscalls bit arrays became redundant and were
> > > removed.
> > > - moved all the remaining common functions out of the
> > > traceon/traceoff patch and into the basic trigger framework patch
> > > and added comments to all the common functions.
> > > - extensively commented the event_trigger_ops and event_command ops.
> > > - made the register/unregister_command functions __init. Since that
> > > code was originally inspired by similar ftrace code, a new patch
> > > was added to do the same thing for the register/unregister of the
> > > ftrace commands (patch 10/11).
> > > - fixed the event_trigger_regex_open i_private problem noted by
> > > Masami that's currently being addressed by Oleg Nesterov's fixes
> > > for this. Note that that patchset also affects patch 8/11 (update
> > > filters for multi-buffer, since it touches event filters as well).
> > > Patch 11/11 depends on that patchset and also moves
> > > event_file_data() to trace.h.b
> >
> > OK, but I think the last 2 patches should be merged to 2/11 as updates.
> >
>
> I did merge the last patch into the new series, but left 10/11 separate
> because it really is just a cleanup independent of the trigger code.
>
> > And also, could you rebase your patches on trace/for-next branch?
> > Since that branch includes most of the latest fixes, it is better to
> > review with it.
> >
>
> Sure, but since now everything in for-next is in rc6, I've rebased on
> rc6...
>

Looks like I spoke too soon - in the few hours between testing this
patchset and posting it, some new commits hit for-next.

for-next rebase coming up...

Tom

> Thanks for all your comments,
>
> Tom
>
> > Thank you,
> >
>