Messages in this thread | | | Date | Thu, 07 May 2009 11:51:08 +0800 | From | Li Zefan <> | 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; > +} > +
| |