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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
