Message-ID: <4A025AAC.2000201@cn.fujitsu.com>
Date:	Thu, 07 May 2009 11:51:08 +0800
From:	Li Zefan <lizf@...fujitsu.com>
To:	Steven Rostedt <rostedt@...dmis.org>
CC:	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...e.hu>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Christoph Hellwig <hch@....de>
Subject: Re: [PATCH 7/7] tracing: add hierarchical enabling of events

> With the current event directory, you can only enable individual events.
> The file debugfs/tracing/set_event is used to be able to enable or
> disable several events at once. But that can still be awkward.
> 
> This patch adds hierarchical enabling of events. That is, each directory
> in debugfs/tracing/events has an "enable" file. This file can enable
> or disable all events within the directory and below.
> 
>  # echo 1 > /debugfs/tracing/events/enable
> 
> will enable all events.
> 
>  # echo 1 > /debugfs/tracing/events/sched/enable
> 
> will enable all events in the sched subsystem.
> 
>  # echo 1 > /debugfs/tracing/events/enable
>  # echo 0 > /debugfs/tracing/events/irq/enable
> 
> will enable all events, but then disable just the irq subsystem events.
> 
> When reading one of these enable files, there are four results:
> 
>  0 - all events this file affects are disabled
>  1 - all events this file affects are enabled
>  X - there is a mixture of events enabled and disabled
>  ? - this file does not affect any event

I would expect reading an enable file will let me know exactly
which events are disabled and which are enabled.

I think this is useful especially for events/system/enable.

Like this:

$ cat events/irq/enable
0 irq_handler_entry
0 irq_handler_exit
1 softirq_entry
1 softirq_exit

> +static ssize_t
> +system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
> +		   loff_t *ppos)
> +{
> +	const char *system = filp->private_data;
> +	struct ftrace_event_call *call;
> +	char buf[2];
> +	int set = -1;
> +	int all = 0;
> +	int ret;
> +
> +	if (system[0] == '*')
> +		all = 1;
> +
> +	mutex_lock(&event_mutex);
> +	list_for_each_entry(call, &ftrace_events, list) {
> +		if (!call->name || !call->regfunc)
> +			continue;
> +
> +		if (!all && strcmp(call->system, system) != 0)
> +			continue;
> +
> +		/*
> +		 * We need to find out if all the events are set
> +		 * or if all events or cleared, or if we have
> +		 * a mixture.
> +		 */
> +		if (call->enabled) {
> +			switch (set) {
> +			case -1:
> +				set = 1;
> +				break;
> +			case 0:
> +				set = 2;
> +				break;
> +			}
> +		} else {
> +			switch (set) {
> +			case -1:
> +				set = 0;
> +				break;
> +			case 1:
> +				set = 2;
> +				break;
> +			}
> +		}
> +		/*
> +		 * If we have a mixture, no need to look further.
> +		 */
> +		if (set == 2)
> +			break;

How about:

int set = 0;

...
set |= (1 << call->enabled);
...

set == 0: '?'
set == 1: '0'
set == 2: '1'
set == 3: 'X'

Will this make the code simpler? :)

Or we can go even further:

char result[4] = { '?', '0', '1', 'X' };
...
buf[0] = result[set];

> +	}
> +	mutex_unlock(&event_mutex);
> +
> +	buf[1] = '\n';
> +	switch (set) {
> +	case 0:
> +		buf[0] = '0';
> +		break;
> +	case 1:
> +		buf[0] = '1';
> +		break;
> +	case 2:
> +		buf[0] = 'X';
> +		break;
> +	default:
> +		buf[0] = '?';
> +	}
> +
> +	ret = simple_read_from_buffer(ubuf, cnt, ppos, buf, 2);
> +
> +	return ret;
> +}
> +
> +static ssize_t
> +system_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
> +		    loff_t *ppos)
> +{
> +	const char *system = filp->private_data;
> +	unsigned long val;
> +	char *command;
> +	char buf[64];
> +	ssize_t ret;
> +
> +	if (cnt >= sizeof(buf))
> +		return -EINVAL;
> +
> +	if (copy_from_user(&buf, ubuf, cnt))
> +		return -EFAULT;
> +
> +	buf[cnt] = 0;
> +
> +	ret = strict_strtoul(buf, 10, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = tracing_update_buffers();
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (val) {
> +	case 0:
> +	case 1:
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	command = kstrdup(system, GFP_KERNEL);
> +	if (!command)
> +		return -ENOMEM;
> +
> +	ret = ftrace_set_clr_event(command, val);

I think we should pass "sched:" or "sched:*", instead of "sched",
the comment in ftrace_set_clr_event():

         *  <name> (no ':') means all events in a subsystem with
         *  the name <name> or any event that matches <name>

> +	if (ret)
> +		goto out_free;
> +
> +	ret = cnt;
> +
> + out_free:
> +	kfree(command);
> +
> +	*ppos += cnt;
> +
> +	return ret;
> +}
> +

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