Date:	Thu, 08 Aug 2013 17:30:06 +0900
From:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
To:	Tom Zanussi <tom.zanussi@...ux.intel.com>
Cc:	rostedt@...dmis.org, linux-kernel@...r.kernel.org,
	Oleg Nesterov <oleg@...hat.com>
Subject: Re: [PATCH v4 02/11] tracing: add basic event trigger framework

(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.

> +
> +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.

> +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.

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

Thank you,

-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@...achi.com


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

Powered by blists - more mailing lists