Re: [PATCH v8 02/10] tracing: Add basic event trigger framework

[Tom Zanussi <tom.zanussi@linux.intel.com>]

On Thu, 2013-09-05 at 13:21 -0400, Steven Rostedt wrote:
> On Mon,  2 Sep 2013 22:52:18 -0500
> Tom Zanussi <tom.zanussi@linux.intel.com> wrote:
> 

[...]

> > +{
> > +	struct event_trigger_data *data;
> > +
> > +	if (list_empty(&file->triggers))
> > +		return;
> > +
> > +	preempt_disable_notrace();
> 
> What's the reason for preempt_disable()? This should be called with
> rcu_read_lock held, right?
> 

You're right, it is called under rcu_read_lock() and the
preempt_disable() is completely pointless - I'll remove these.

> > +	list_for_each_entry_rcu(data, &file->triggers, list)
> > +		data->ops->func(data);
> > +	preempt_enable_notrace();
> > +}
> > +EXPORT_SYMBOL_GPL(event_triggers_call);
> > +
> > +static void *trigger_next(struct seq_file *m, void *t, loff_t *pos)
> > +{
> > +	struct ftrace_event_file *event_file = event_file_data(m->private);
> > +
> > +	return seq_list_next(t, &event_file->triggers, pos);
> > +}
> > +
> > +static void *trigger_start(struct seq_file *m, loff_t *pos)
> > +{
> > +	struct ftrace_event_file *event_file;
> > +
> > +	/* ->stop() is called even if ->start() fails */
> > +	mutex_lock(&event_mutex);
> > +	event_file = event_file_data(m->private);
> > +	if (unlikely(!event_file))
> > +		return ERR_PTR(-ENODEV);
> > +
> > +	return seq_list_start(&event_file->triggers, *pos);
> > +}
> > +
> > +static void trigger_stop(struct seq_file *m, void *t)
> > +{
> > +	mutex_unlock(&event_mutex);
> > +}
> > +
> > +static int trigger_show(struct seq_file *m, void *v)
> > +{
> > +	struct event_trigger_data *data;
> > +
> > +	data = list_entry(v, struct event_trigger_data, list);
> > +	data->ops->print(m, data->ops, data);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct seq_operations event_triggers_seq_ops = {
> > +	.start = trigger_start,
> > +	.next = trigger_next,
> > +	.stop = trigger_stop,
> > +	.show = trigger_show,
> > +};
> > +
> > +static int event_trigger_regex_open(struct inode *inode, struct file *file)
> > +{
> > +	int ret = 0;
> > +
> > +	mutex_lock(&event_mutex);
> > +
> > +	if (unlikely(!event_file_data(file))) {
> > +		mutex_unlock(&event_mutex);
> > +		return -ENODEV;
> > +	}
> > +
> > +	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 = file;
> > +		}
> > +	}
> > +
> > +	mutex_unlock(&event_mutex);
> > +
> > +	return ret;
> > +}
> > +
> > +static int trigger_process_regex(struct ftrace_event_file *file,
> > +				 char *buff, int enable)
> > +{
> > +	char *command, *next = buff;
> > +	struct event_command *p;
> > +	int ret = -EINVAL;
> > +
> > +	command = strsep(&next, ": \t");
> > +	command = (command[0] != '!') ? command : command + 1;
> > +
> > +	mutex_lock(&trigger_cmd_mutex);
> 
> What exactly is trigger_cmd_mutex protecting? It is only called here,
> and the event_mutex() is already held by its only caller, so this mutex
> is basically doing nothing.
> 

trigger_cmd_mutex is also passed into and used by
register/unregister_event_command() (as *cmd_list_mutex) and protects
writers setting trigger commands against changes in the command list.

The next question would be why pass the mutex and command list in to the
register/unregister command instead of having those functions use them
directly?  It's because these functions were originally meant to manage
multiple command lists, for both ftrace and event commands, but since it
now handles only event commands, I'll remove those extra params and just
use them directly.

Thanks,

Tom