[PATCH 1/2] tracing/events: clean up for ftrace_set_clr_event()

[PATCH 1/2] tracing/events: clean up for ftrace_set_clr_event()

[Li Zefan]

Add a helper function __ftrace_set_clr_event(), and replace some
ftrace_set_clr_event() calls with this helper, thus we don't need any
kstrdup() or kmalloc().

As a side effect, this patch fixes an issue in self tests code, which is
similar to the one fixed in commit d6bf81ef0f7474434c2a049e8bf3c9146a14dd96
("tracing: append ":*" to internal setting of system events")

It's a small issue and won't cause any bug in fact, but we should do things
right anyway.

[ Impact: clean up ]

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/trace/trace_events.c |  126 ++++++++++++++++---------------------------
 1 files changed, 46 insertions(+), 80 deletions(-)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 8d0fae3..45f1099 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -111,11 +111,44 @@ static void ftrace_event_enable_disable(struct ftrace_event_call *call,
 	}
 }
 
-static int ftrace_set_clr_event(char *buf, int set)
+/*
+ * __ftrace_set_clr_event(NULL, NULL, NULL, set) will set/unset all events.
+ */
+static int __ftrace_set_clr_event(const char *match, const char *sub,
+				  const char *event, int set)
 {
 	struct ftrace_event_call *call;
+	int ret;
+
+	mutex_lock(&event_mutex);
+	list_for_each_entry(call, &ftrace_events, list) {
+
+		if (!call->name || !call->regfunc)
+			continue;
+
+		if (match &&
+		    strcmp(match, call->name) != 0 &&
+		    strcmp(match, call->system) != 0)
+			continue;
+
+		if (sub && strcmp(sub, call->system) != 0)
+			continue;
+
+		if (event && strcmp(event, call->name) != 0)
+			continue;
+
+		ftrace_event_enable_disable(call, set);
+
+		ret = 0;
+	}
+	mutex_unlock(&event_mutex);
+
+	return ret;
+}
+
+static int ftrace_set_clr_event(char *buf, int set)
+{
 	char *event = NULL, *sub = NULL, *match;
-	int ret = -EINVAL;
 
 	/*
 	 * The buf format can be <subsystem>:<event-name>
@@ -141,30 +174,7 @@ static int ftrace_set_clr_event(char *buf, int set)
 			event = NULL;
 	}
 
-	mutex_lock(&event_mutex);
-	list_for_each_entry(call, &ftrace_events, list) {
-
-		if (!call->name || !call->regfunc)
-			continue;
-
-		if (match &&
-		    strcmp(match, call->name) != 0 &&
-		    strcmp(match, call->system) != 0)
-			continue;
-
-		if (sub && strcmp(sub, call->system) != 0)
-			continue;
-
-		if (event && strcmp(event, call->name) != 0)
-			continue;
-
-		ftrace_event_enable_disable(call, set);
-
-		ret = 0;
-	}
-	mutex_unlock(&event_mutex);
-
-	return ret;
+	return __ftrace_set_clr_event(match, sub, event, set);
 }
 
 /* 128 should be much more than enough */
@@ -408,18 +418,14 @@ system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
 	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)
+		if (system && strcmp(call->system, system) != 0)
 			continue;
 
 		/*
@@ -480,7 +486,6 @@ system_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
 {
 	const char *system = filp->private_data;
 	unsigned long val;
-	char *command;
 	char buf[64];
 	ssize_t ret;
 
@@ -500,30 +505,16 @@ system_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
 	if (ret < 0)
 		return ret;
 
-	switch (val) {
-	case 0:
-	case 1:
-		break;
-
-	default:
+	if (val != 0 && val != 1)
 		return -EINVAL;
-	}
 
-	/* +3 for the ":*\0" */
-	command = kmalloc(strlen(system)+3, GFP_KERNEL);
-	if (!command)
-		return -ENOMEM;
-	sprintf(command, "%s:*", system);
-
-	ret = ftrace_set_clr_event(command, val);
+	ret = __ftrace_set_clr_event(NULL, system, NULL, val);
 	if (ret)
-		goto out_free;
+		goto out;
 
 	ret = cnt;
 
- out_free:
-	kfree(command);
-
+out:
 	*ppos += cnt;
 
 	return ret;
@@ -1181,7 +1172,7 @@ static __init int event_trace_init(void)
 			  &ftrace_show_header_fops);
 
 	trace_create_file("enable", 0644, d_events,
-			  "*", &ftrace_system_enable_fops);
+			  NULL, &ftrace_system_enable_fops);
 
 	for_each_event(call, __start_ftrace_events, __stop_ftrace_events) {
 		/* The linker may leave blanks */
@@ -1259,7 +1250,6 @@ static __init void event_trace_self_tests(void)
 {
 	struct ftrace_event_call *call;
 	struct event_subsystem *system;
-	char *sysname;
 	int ret;
 
 	pr_info("Running tests on trace events:\n");
@@ -1305,14 +1295,7 @@ static __init void event_trace_self_tests(void)
 
 		pr_info("Testing event system %s: ", system->name);
 
-		/* ftrace_set_clr_event can modify the name passed in. */
-		sysname = kstrdup(system->name, GFP_KERNEL);
-		if (WARN_ON(!sysname)) {
-			pr_warning("Can't allocate memory, giving up!\n");
-			return;
-		}
-		ret = ftrace_set_clr_event(sysname, 1);
-		kfree(sysname);
+		ret = __ftrace_set_clr_event(NULL, system->name, NULL, 1);
 		if (WARN_ON_ONCE(ret)) {
 			pr_warning("error enabling system %s\n",
 				   system->name);
@@ -1321,14 +1304,7 @@ static __init void event_trace_self_tests(void)
 
 		event_test_stuff();
 
-		sysname = kstrdup(system->name, GFP_KERNEL);
-		if (WARN_ON(!sysname)) {
-			pr_warning("Can't allocate memory, giving up!\n");
-			return;
-		}
-		ret = ftrace_set_clr_event(sysname, 0);
-		kfree(sysname);
-
+		ret = __ftrace_set_clr_event(NULL, system->name, NULL, 0);
 		if (WARN_ON_ONCE(ret))
 			pr_warning("error disabling system %s\n",
 				   system->name);
@@ -1341,15 +1317,8 @@ static __init void event_trace_self_tests(void)
 	pr_info("Running tests on all trace events:\n");
 	pr_info("Testing all events: ");
 
-	sysname = kmalloc(4, GFP_KERNEL);
-	if (WARN_ON(!sysname)) {
-		pr_warning("Can't allocate memory, giving up!\n");
-		return;
-	}
-	memcpy(sysname, "*:*", 4);
-	ret = ftrace_set_clr_event(sysname, 1);
+	ret = __ftrace_set_clr_event(NULL, NULL, NULL, 1);
 	if (WARN_ON_ONCE(ret)) {
-		kfree(sysname);
 		pr_warning("error enabling all events\n");
 		return;
 	}
@@ -1357,10 +1326,7 @@ static __init void event_trace_self_tests(void)
 	event_test_stuff();
 
 	/* reset sysname */
-	memcpy(sysname, "*:*", 4);
-	ret = ftrace_set_clr_event(sysname, 0);
-	kfree(sysname);
-
+	ret = __ftrace_set_clr_event(NULL, NULL, NULL, 0);
 	if (WARN_ON_ONCE(ret)) {
 		pr_warning("error disabling all events\n");
 		return;
-- 
1.5.4.rc3

[PATCH 2/2] tracing/events: simplify system_enable_read()

[Li Zefan]

A smarter way to figure out the output of an enable file.

[ Impact: clean up ]

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/trace/trace_events.c |   40 ++++++----------------------------------
 1 files changed, 6 insertions(+), 34 deletions(-)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 45f1099..df394bc 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -414,10 +414,11 @@ static ssize_t
 system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
 		   loff_t *ppos)
 {
+	const char set_to_char[4] = { '?', '0', '1', 'X' };
 	const char *system = filp->private_data;
 	struct ftrace_event_call *call;
 	char buf[2];
-	int set = -1;
+	int set = 0;
 	int ret;
 
 	mutex_lock(&event_mutex);
@@ -433,47 +434,18 @@ system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
 		 * 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;
-			}
-		}
+		set |= (1 << !!call->enabled);
+
 		/*
 		 * If we have a mixture, no need to look further.
 		 */
-		if (set == 2)
+		if (set == 3)
 			break;
 	}
 	mutex_unlock(&event_mutex);
 
+	buf[0] = set_to_char[set];
 	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);
 
-- 
1.5.4.rc3

Re: [PATCH 2/2] tracing/events: simplify system_enable_read()

[Steven Rostedt]

On Fri, 8 May 2009, Li Zefan wrote:

> A smarter way to figure out the output of an enable file.

I wouldn't say "smarter way", but I will not argue a "cleaner way".

-- Steve

> 
> [ Impact: clean up ]
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/trace/trace_events.c |   40 ++++++----------------------------------
>  1 files changed, 6 insertions(+), 34 deletions(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 45f1099..df394bc 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -414,10 +414,11 @@ static ssize_t
>  system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
>  		   loff_t *ppos)
>  {
> +	const char set_to_char[4] = { '?', '0', '1', 'X' };
>  	const char *system = filp->private_data;
>  	struct ftrace_event_call *call;
>  	char buf[2];
> -	int set = -1;
> +	int set = 0;
>  	int ret;
>  
>  	mutex_lock(&event_mutex);
> @@ -433,47 +434,18 @@ system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
>  		 * 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;
> -			}
> -		}
> +		set |= (1 << !!call->enabled);
> +
>  		/*
>  		 * If we have a mixture, no need to look further.
>  		 */
> -		if (set == 2)
> +		if (set == 3)
>  			break;
>  	}
>  	mutex_unlock(&event_mutex);
>  
> +	buf[0] = set_to_char[set];
>  	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);
>  
> -- 
> 1.5.4.rc3
> 
> 

Re: [PATCH 1/2] tracing/events: clean up for ftrace_set_clr_event()

[Steven Rostedt]

On Fri, 8 May 2009, Li Zefan wrote:

> Add a helper function __ftrace_set_clr_event(), and replace some
> ftrace_set_clr_event() calls with this helper, thus we don't need any
> kstrdup() or kmalloc().
> 
> As a side effect, this patch fixes an issue in self tests code, which is
> similar to the one fixed in commit d6bf81ef0f7474434c2a049e8bf3c9146a14dd96
> ("tracing: append ":*" to internal setting of system events")
> 
> It's a small issue and won't cause any bug in fact, but we should do things
> right anyway.
> 
> [ Impact: clean up ]
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/trace/trace_events.c |  126 ++++++++++++++++---------------------------
>  1 files changed, 46 insertions(+), 80 deletions(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 8d0fae3..45f1099 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -111,11 +111,44 @@ static void ftrace_event_enable_disable(struct ftrace_event_call *call,
>  	}
>  }
>  
> -static int ftrace_set_clr_event(char *buf, int set)
> +/*
> + * __ftrace_set_clr_event(NULL, NULL, NULL, set) will set/unset all events.
> + */
> +static int __ftrace_set_clr_event(const char *match, const char *sub,
> +				  const char *event, int set)
>  {
>  	struct ftrace_event_call *call;
> +	int ret;
> +
> +	mutex_lock(&event_mutex);
> +	list_for_each_entry(call, &ftrace_events, list) {
> +
> +		if (!call->name || !call->regfunc)
> +			continue;
> +
> +		if (match &&
> +		    strcmp(match, call->name) != 0 &&
> +		    strcmp(match, call->system) != 0)
> +			continue;
> +
> +		if (sub && strcmp(sub, call->system) != 0)
> +			continue;
> +
> +		if (event && strcmp(event, call->name) != 0)
> +			continue;
> +
> +		ftrace_event_enable_disable(call, set);
> +
> +		ret = 0;
> +	}
> +	mutex_unlock(&event_mutex);
> +
> +	return ret;
> +}
> +
> +static int ftrace_set_clr_event(char *buf, int set)
> +{
>  	char *event = NULL, *sub = NULL, *match;
> -	int ret = -EINVAL;
>  
>  	/*
>  	 * The buf format can be <subsystem>:<event-name>
> @@ -141,30 +174,7 @@ static int ftrace_set_clr_event(char *buf, int set)
>  			event = NULL;
>  	}
>  
> -	mutex_lock(&event_mutex);
> -	list_for_each_entry(call, &ftrace_events, list) {
> -
> -		if (!call->name || !call->regfunc)
> -			continue;
> -
> -		if (match &&
> -		    strcmp(match, call->name) != 0 &&
> -		    strcmp(match, call->system) != 0)
> -			continue;
> -
> -		if (sub && strcmp(sub, call->system) != 0)
> -			continue;
> -
> -		if (event && strcmp(event, call->name) != 0)
> -			continue;
> -
> -		ftrace_event_enable_disable(call, set);
> -
> -		ret = 0;
> -	}
> -	mutex_unlock(&event_mutex);
> -
> -	return ret;
> +	return __ftrace_set_clr_event(match, sub, event, set);
>  }
>  
>  /* 128 should be much more than enough */
> @@ -408,18 +418,14 @@ system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
>  	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)
> +		if (system && strcmp(call->system, system) != 0)
>  			continue;
>  
>  		/*
> @@ -480,7 +486,6 @@ system_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
>  {
>  	const char *system = filp->private_data;
>  	unsigned long val;
> -	char *command;
>  	char buf[64];
>  	ssize_t ret;
>  
> @@ -500,30 +505,16 @@ system_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
>  	if (ret < 0)
>  		return ret;
>  
> -	switch (val) {
> -	case 0:
> -	case 1:
> -		break;
> -
> -	default:
> +	if (val != 0 && val != 1)

What about "if (val & ~1UL)"?

>  		return -EINVAL;
> -	}
>  
> -	/* +3 for the ":*\0" */
> -	command = kmalloc(strlen(system)+3, GFP_KERNEL);
> -	if (!command)
> -		return -ENOMEM;
> -	sprintf(command, "%s:*", system);
> -
> -	ret = ftrace_set_clr_event(command, val);
> +	ret = __ftrace_set_clr_event(NULL, system, NULL, val);
>  	if (ret)
> -		goto out_free;
> +		goto out;
>  
>  	ret = cnt;
>  
> - out_free:
> -	kfree(command);
> -
> +out:
>  	*ppos += cnt;
>  
>  	return ret;
> @@ -1181,7 +1172,7 @@ static __init int event_trace_init(void)
>  			  &ftrace_show_header_fops);
>  
>  	trace_create_file("enable", 0644, d_events,
> -			  "*", &ftrace_system_enable_fops);
> +			  NULL, &ftrace_system_enable_fops);
>  
>  	for_each_event(call, __start_ftrace_events, __stop_ftrace_events) {
>  		/* The linker may leave blanks */
> @@ -1259,7 +1250,6 @@ static __init void event_trace_self_tests(void)
>  {
>  	struct ftrace_event_call *call;
>  	struct event_subsystem *system;
> -	char *sysname;
>  	int ret;
>  
>  	pr_info("Running tests on trace events:\n");
> @@ -1305,14 +1295,7 @@ static __init void event_trace_self_tests(void)
>  
>  		pr_info("Testing event system %s: ", system->name);
>  
> -		/* ftrace_set_clr_event can modify the name passed in. */
> -		sysname = kstrdup(system->name, GFP_KERNEL);
> -		if (WARN_ON(!sysname)) {
> -			pr_warning("Can't allocate memory, giving up!\n");
> -			return;
> -		}
> -		ret = ftrace_set_clr_event(sysname, 1);
> -		kfree(sysname);
> +		ret = __ftrace_set_clr_event(NULL, system->name, NULL, 1);
>  		if (WARN_ON_ONCE(ret)) {
>  			pr_warning("error enabling system %s\n",
>  				   system->name);
> @@ -1321,14 +1304,7 @@ static __init void event_trace_self_tests(void)
>  
>  		event_test_stuff();
>  
> -		sysname = kstrdup(system->name, GFP_KERNEL);
> -		if (WARN_ON(!sysname)) {
> -			pr_warning("Can't allocate memory, giving up!\n");
> -			return;
> -		}
> -		ret = ftrace_set_clr_event(sysname, 0);
> -		kfree(sysname);
> -
> +		ret = __ftrace_set_clr_event(NULL, system->name, NULL, 0);
>  		if (WARN_ON_ONCE(ret))
>  			pr_warning("error disabling system %s\n",
>  				   system->name);
> @@ -1341,15 +1317,8 @@ static __init void event_trace_self_tests(void)
>  	pr_info("Running tests on all trace events:\n");
>  	pr_info("Testing all events: ");
>  
> -	sysname = kmalloc(4, GFP_KERNEL);
> -	if (WARN_ON(!sysname)) {
> -		pr_warning("Can't allocate memory, giving up!\n");
> -		return;
> -	}
> -	memcpy(sysname, "*:*", 4);
> -	ret = ftrace_set_clr_event(sysname, 1);
> +	ret = __ftrace_set_clr_event(NULL, NULL, NULL, 1);
>  	if (WARN_ON_ONCE(ret)) {
> -		kfree(sysname);
>  		pr_warning("error enabling all events\n");
>  		return;
>  	}
> @@ -1357,10 +1326,7 @@ static __init void event_trace_self_tests(void)
>  	event_test_stuff();
>  
>  	/* reset sysname */
> -	memcpy(sysname, "*:*", 4);
> -	ret = ftrace_set_clr_event(sysname, 0);
> -	kfree(sysname);
> -
> +	ret = __ftrace_set_clr_event(NULL, NULL, NULL, 0);
>  	if (WARN_ON_ONCE(ret)) {
>  		pr_warning("error disabling all events\n");
>  		return;

Other than my one little optimization above, it looks good.

-- Steve

Re: [PATCH 1/2] tracing/events: clean up for ftrace_set_clr_event()

[Li Zefan]

>> @@ -500,30 +505,16 @@ system_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	switch (val) {
>> -	case 0:
>> -	case 1:
>> -		break;
>> -
>> -	default:
>> +	if (val != 0 && val != 1)
> 
> What about "if (val & ~1UL)"?
> 

I tried it just now:

   text    data     bss     dec     hex filename
   7480     104       8    7592    1da8 kernel/trace/trace_events.o.orig
   text    data     bss     dec     hex filename
   7482     104       8    7594    1daa kernel/trace/trace_events.o

It increased text size by 2 bytes, gcc did a better work. ;)

And it's not so intuitive what's the limit of val.

Re: [PATCH 1/2] tracing/events: clean up for ftrace_set_clr_event()

[Frederic Weisbecker]

On Fri, May 08, 2009 at 10:31:42AM +0800, Li Zefan wrote:
> Add a helper function __ftrace_set_clr_event(), and replace some
> ftrace_set_clr_event() calls with this helper, thus we don't need any
> kstrdup() or kmalloc().
> 
> As a side effect, this patch fixes an issue in self tests code, which is
> similar to the one fixed in commit d6bf81ef0f7474434c2a049e8bf3c9146a14dd96
> ("tracing: append ":*" to internal setting of system events")
> 
> It's a small issue and won't cause any bug in fact, but we should do things
> right anyway.
> 
> [ Impact: clean up ]



If this fixes an issue like you described, then it's more than a cleanup :)



> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/trace/trace_events.c |  126 ++++++++++++++++---------------------------
>  1 files changed, 46 insertions(+), 80 deletions(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 8d0fae3..45f1099 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -111,11 +111,44 @@ static void ftrace_event_enable_disable(struct ftrace_event_call *call,
>  	}
>  }
>  
> -static int ftrace_set_clr_event(char *buf, int set)
> +/*
> + * __ftrace_set_clr_event(NULL, NULL, NULL, set) will set/unset all events.
> + */
> +static int __ftrace_set_clr_event(const char *match, const char *sub,
> +				  const char *event, int set)
>  {
>  	struct ftrace_event_call *call;
> +	int ret;
> +
> +	mutex_lock(&event_mutex);
> +	list_for_each_entry(call, &ftrace_events, list) {
> +
> +		if (!call->name || !call->regfunc)
> +			continue;
> +
> +		if (match &&
> +		    strcmp(match, call->name) != 0 &&
> +		    strcmp(match, call->system) != 0)
> +			continue;
> +
> +		if (sub && strcmp(sub, call->system) != 0)
> +			continue;
> +
> +		if (event && strcmp(event, call->name) != 0)
> +			continue;


Neat: You can simply use !strcmp(...)


> +
> +		ftrace_event_enable_disable(call, set);
> +
> +		ret = 0;
> +	}
> +	mutex_unlock(&event_mutex);
> +
> +	return ret;
> +}
> +
> +static int ftrace_set_clr_event(char *buf, int set)
> +{
>  	char *event = NULL, *sub = NULL, *match;
> -	int ret = -EINVAL;
>  
>  	/*
>  	 * The buf format can be <subsystem>:<event-name>
> @@ -141,30 +174,7 @@ static int ftrace_set_clr_event(char *buf, int set)
>  			event = NULL;
>  	}
>  
> -	mutex_lock(&event_mutex);
> -	list_for_each_entry(call, &ftrace_events, list) {
> -
> -		if (!call->name || !call->regfunc)
> -			continue;
> -
> -		if (match &&
> -		    strcmp(match, call->name) != 0 &&
> -		    strcmp(match, call->system) != 0)
> -			continue;
> -
> -		if (sub && strcmp(sub, call->system) != 0)
> -			continue;
> -
> -		if (event && strcmp(event, call->name) != 0)
> -			continue;
> -
> -		ftrace_event_enable_disable(call, set);
> -
> -		ret = 0;
> -	}
> -	mutex_unlock(&event_mutex);
> -
> -	return ret;
> +	return __ftrace_set_clr_event(match, sub, event, set);
>  }
>  
>  /* 128 should be much more than enough */
> @@ -408,18 +418,14 @@ system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
>  	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)
> +		if (system && strcmp(call->system, system) != 0)
>  			continue;
>  
>  		/*
> @@ -480,7 +486,6 @@ system_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
>  {
>  	const char *system = filp->private_data;
>  	unsigned long val;
> -	char *command;
>  	char buf[64];
>  	ssize_t ret;
>  
> @@ -500,30 +505,16 @@ system_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
>  	if (ret < 0)
>  		return ret;
>  
> -	switch (val) {
> -	case 0:
> -	case 1:
> -		break;
> -
> -	default:
> +	if (val != 0 && val != 1)
>  		return -EINVAL;
> -	}
>  
> -	/* +3 for the ":*\0" */
> -	command = kmalloc(strlen(system)+3, GFP_KERNEL);
> -	if (!command)
> -		return -ENOMEM;
> -	sprintf(command, "%s:*", system);
> -
> -	ret = ftrace_set_clr_event(command, val);
> +	ret = __ftrace_set_clr_event(NULL, system, NULL, val);
>  	if (ret)
> -		goto out_free;
> +		goto out;
>  
>  	ret = cnt;
>  
> - out_free:
> -	kfree(command);
> -
> +out:
>  	*ppos += cnt;
>  
>  	return ret;
> @@ -1181,7 +1172,7 @@ static __init int event_trace_init(void)
>  			  &ftrace_show_header_fops);
>  
>  	trace_create_file("enable", 0644, d_events,
> -			  "*", &ftrace_system_enable_fops);
> +			  NULL, &ftrace_system_enable_fops);
>  
>  	for_each_event(call, __start_ftrace_events, __stop_ftrace_events) {
>  		/* The linker may leave blanks */
> @@ -1259,7 +1250,6 @@ static __init void event_trace_self_tests(void)
>  {
>  	struct ftrace_event_call *call;
>  	struct event_subsystem *system;
> -	char *sysname;
>  	int ret;
>  
>  	pr_info("Running tests on trace events:\n");
> @@ -1305,14 +1295,7 @@ static __init void event_trace_self_tests(void)
>  
>  		pr_info("Testing event system %s: ", system->name);
>  
> -		/* ftrace_set_clr_event can modify the name passed in. */
> -		sysname = kstrdup(system->name, GFP_KERNEL);
> -		if (WARN_ON(!sysname)) {
> -			pr_warning("Can't allocate memory, giving up!\n");
> -			return;
> -		}
> -		ret = ftrace_set_clr_event(sysname, 1);
> -		kfree(sysname);
> +		ret = __ftrace_set_clr_event(NULL, system->name, NULL, 1);
>  		if (WARN_ON_ONCE(ret)) {
>  			pr_warning("error enabling system %s\n",
>  				   system->name);
> @@ -1321,14 +1304,7 @@ static __init void event_trace_self_tests(void)
>  
>  		event_test_stuff();
>  
> -		sysname = kstrdup(system->name, GFP_KERNEL);
> -		if (WARN_ON(!sysname)) {
> -			pr_warning("Can't allocate memory, giving up!\n");
> -			return;
> -		}
> -		ret = ftrace_set_clr_event(sysname, 0);
> -		kfree(sysname);
> -
> +		ret = __ftrace_set_clr_event(NULL, system->name, NULL, 0);
>  		if (WARN_ON_ONCE(ret))
>  			pr_warning("error disabling system %s\n",
>  				   system->name);
> @@ -1341,15 +1317,8 @@ static __init void event_trace_self_tests(void)
>  	pr_info("Running tests on all trace events:\n");
>  	pr_info("Testing all events: ");
>  
> -	sysname = kmalloc(4, GFP_KERNEL);
> -	if (WARN_ON(!sysname)) {
> -		pr_warning("Can't allocate memory, giving up!\n");
> -		return;
> -	}
> -	memcpy(sysname, "*:*", 4);
> -	ret = ftrace_set_clr_event(sysname, 1);
> +	ret = __ftrace_set_clr_event(NULL, NULL, NULL, 1);
>  	if (WARN_ON_ONCE(ret)) {
> -		kfree(sysname);
>  		pr_warning("error enabling all events\n");
>  		return;
>  	}
> @@ -1357,10 +1326,7 @@ static __init void event_trace_self_tests(void)
>  	event_test_stuff();
>  
>  	/* reset sysname */
> -	memcpy(sysname, "*:*", 4);
> -	ret = ftrace_set_clr_event(sysname, 0);
> -	kfree(sysname);
> -
> +	ret = __ftrace_set_clr_event(NULL, NULL, NULL, 0);
>  	if (WARN_ON_ONCE(ret)) {
>  		pr_warning("error disabling all events\n");
>  		return;
> -- 
> 1.5.4.rc3
> 


I haven't reviewed deeply, but it looks good and bring a rather
good simplification.

Re: [PATCH 2/2] tracing/events: simplify system_enable_read()

[Frederic Weisbecker]

On Fri, May 08, 2009 at 10:32:05AM +0800, Li Zefan wrote:
> A smarter way to figure out the output of an enable file.
> 
> [ Impact: clean up ]
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/trace/trace_events.c |   40 ++++++----------------------------------
>  1 files changed, 6 insertions(+), 34 deletions(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 45f1099..df394bc 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -414,10 +414,11 @@ static ssize_t
>  system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
>  		   loff_t *ppos)
>  {
> +	const char set_to_char[4] = { '?', '0', '1', 'X' };
>  	const char *system = filp->private_data;
>  	struct ftrace_event_call *call;
>  	char buf[2];
> -	int set = -1;
> +	int set = 0;
>  	int ret;
>  
>  	mutex_lock(&event_mutex);
> @@ -433,47 +434,18 @@ system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
>  		 * 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;
> -			}
> -		}
> +		set |= (1 << !!call->enabled);
> +
>  		/*
>  		 * If we have a mixture, no need to look further.
>  		 */
> -		if (set == 2)
> +		if (set == 3)
>  			break;
>  	}
>  	mutex_unlock(&event_mutex);
>  
> +	buf[0] = set_to_char[set];
>  	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);
>  
> -- 
> 1.5.4.rc3
> 

Nice!

Acked-by: Frederic Weisbecker <fweisbec@gmail.com>

Re: [PATCH 1/2] tracing/events: clean up for ftrace_set_clr_event()

[Li Zefan]

Frederic Weisbecker wrote:
> On Fri, May 08, 2009 at 10:31:42AM +0800, Li Zefan wrote:
>> Add a helper function __ftrace_set_clr_event(), and replace some
>> ftrace_set_clr_event() calls with this helper, thus we don't need any
>> kstrdup() or kmalloc().
>>
>> As a side effect, this patch fixes an issue in self tests code, which is
>> similar to the one fixed in commit d6bf81ef0f7474434c2a049e8bf3c9146a14dd96
>> ("tracing: append ":*" to internal setting of system events")
>>
>> It's a small issue and won't cause any bug in fact, but we should do things
>> right anyway.
>>
>> [ Impact: clean up ]
> 
> If this fixes an issue like you described, then it's more than a cleanup :)
> 

That issue causes no bug, and that's why I call it a cleanup.

How about (mainly stealed from commit d6bf81ef0f7474434c2a049e8bf3c9146a14dd96):

[ Impact: prevent accidental enabling of events with same name as a system in self tests ]

But it excceeds 80 char..

I sometimes feel it hard to write Impact line (one of the reason is my limit
English skill). I've explained the impact of this patch in detail, but I'm
still required to add a one-line summary. :(

> 
...
>> +		if (event && strcmp(event, call->name) != 0)
>> +			continue;
> 
> 
> Neat: You can simply use !strcmp(...)
> 

Actually it's arguable which is better, and both styles are used in kernel code.

And that 'if (!ptr)' vs 'if (ptr == NULL)'..

Re: [PATCH 1/2] tracing/events: clean up for ftrace_set_clr_event()

[Frederic Weisbecker]

On Fri, May 08, 2009 at 07:16:08PM +0800, Li Zefan wrote:
> Frederic Weisbecker wrote:
> > On Fri, May 08, 2009 at 10:31:42AM +0800, Li Zefan wrote:
> >> Add a helper function __ftrace_set_clr_event(), and replace some
> >> ftrace_set_clr_event() calls with this helper, thus we don't need any
> >> kstrdup() or kmalloc().
> >>
> >> As a side effect, this patch fixes an issue in self tests code, which is
> >> similar to the one fixed in commit d6bf81ef0f7474434c2a049e8bf3c9146a14dd96
> >> ("tracing: append ":*" to internal setting of system events")
> >>
> >> It's a small issue and won't cause any bug in fact, but we should do things
> >> right anyway.
> >>
> >> [ Impact: clean up ]
> > 
> > If this fixes an issue like you described, then it's more than a cleanup :)
> > 
> 
> That issue causes no bug, and that's why I call it a cleanup.
> 
> How about (mainly stealed from commit d6bf81ef0f7474434c2a049e8bf3c9146a14dd96):
> 
> [ Impact: prevent accidental enabling of events with same name as a system in self tests ]
> 
> But it excceeds 80 char..
> 
> I sometimes feel it hard to write Impact line (one of the reason is my limit
> English skill). I've explained the impact of this patch in detail, but I'm
> still required to add a one-line summary. :(


Well, I also find hard to write straightforward and good matching
impact lines.
And I'm certainly not well suited to give any advices about how
to write good impact lines.

But IMHO you can sum up your above impact line.

[ Impact: prevent spurious events enabling in tracing selftests ]

They usually don't need more details, those details can be placed in
the changelog. It's more about the general pratical impact, not a
detailed one.

Frederic.


> > 
> ...
> >> +		if (event && strcmp(event, call->name) != 0)
> >> +			continue;
> > 
> > 
> > Neat: You can simply use !strcmp(...)
> > 
> 
> Actually it's arguable which is better, and both styles are used in kernel code.
> 
> And that 'if (!ptr)' vs 'if (ptr == NULL)'..
> 

Re: [PATCH 1/2] tracing/events: clean up for ftrace_set_clr_event()

[Steven Rostedt]

On Fri, 8 May 2009, Frederic Weisbecker wrote:
> >  
> > -static int ftrace_set_clr_event(char *buf, int set)
> > +/*
> > + * __ftrace_set_clr_event(NULL, NULL, NULL, set) will set/unset all events.
> > + */
> > +static int __ftrace_set_clr_event(const char *match, const char *sub,
> > +				  const char *event, int set)
> >  {
> >  	struct ftrace_event_call *call;
> > +	int ret;
> > +
> > +	mutex_lock(&event_mutex);
> > +	list_for_each_entry(call, &ftrace_events, list) {
> > +
> > +		if (!call->name || !call->regfunc)
> > +			continue;
> > +
> > +		if (match &&
> > +		    strcmp(match, call->name) != 0 &&
> > +		    strcmp(match, call->system) != 0)
> > +			continue;
> > +
> > +		if (sub && strcmp(sub, call->system) != 0)
> > +			continue;
> > +
> > +		if (event && strcmp(event, call->name) != 0)
> > +			continue;
> 
> 
> Neat: You can simply use !strcmp(...)

Hehe, no he can't. It would be "strcmp(...)" for the true case. This is 
exactly why I prefer to use "strcmp(...) != 0" over "!strcmp(...)". 
Because, like you, I've confused "!strcmp(...)" too many times as "not a 
match" when it in fact means "is a match".

I've made this mistake enough that I've given up on using just "strcmp" or 
"!strcmp". "strcmp() != 0" and "strcmp() == 0" show what you want much 
better.

-- Steve

Re: [PATCH 1/2] tracing/events: clean up for ftrace_set_clr_event()

[Frederic Weisbecker]

On Fri, May 08, 2009 at 07:35:22AM -0400, Steven Rostedt wrote:
> 
> On Fri, 8 May 2009, Frederic Weisbecker wrote:
> > >  
> > > -static int ftrace_set_clr_event(char *buf, int set)
> > > +/*
> > > + * __ftrace_set_clr_event(NULL, NULL, NULL, set) will set/unset all events.
> > > + */
> > > +static int __ftrace_set_clr_event(const char *match, const char *sub,
> > > +				  const char *event, int set)
> > >  {
> > >  	struct ftrace_event_call *call;
> > > +	int ret;
> > > +
> > > +	mutex_lock(&event_mutex);
> > > +	list_for_each_entry(call, &ftrace_events, list) {
> > > +
> > > +		if (!call->name || !call->regfunc)
> > > +			continue;
> > > +
> > > +		if (match &&
> > > +		    strcmp(match, call->name) != 0 &&
> > > +		    strcmp(match, call->system) != 0)
> > > +			continue;
> > > +
> > > +		if (sub && strcmp(sub, call->system) != 0)
> > > +			continue;
> > > +
> > > +		if (event && strcmp(event, call->name) != 0)
> > > +			continue;
> > 
> > 
> > Neat: You can simply use !strcmp(...)
> 
> Hehe, no he can't. It would be "strcmp(...)" for the true case. This is 
> exactly why I prefer to use "strcmp(...) != 0" over "!strcmp(...)". 
> Because, like you, I've confused "!strcmp(...)" too many times as "not a 
> match" when it in fact means "is a match".
> 
> I've made this mistake enough that I've given up on using just "strcmp" or 
> "!strcmp". "strcmp() != 0" and "strcmp() == 0" show what you want much 
> better.
> 

You're right. It provides a good disambiguation.
The C philosophy has this 0 == SUCCESS convention which doesn't match
the human brain logic that expect 0 is a false and 1 is a true....

I guess that's because "!" provides quick checks about non-failures
and detailed errors can then fit in custom values. But still, I guess
we all stuck in this scheme, at least in a last remaining nerve cell
which says "hell, but look! wtf...", though this poor neurone ends
up being punched and kicked by the rest of the brain folks...


Frederic.

Re: [PATCH 1/2] tracing/events: clean up for ftrace_set_clr_event()

[Steven Rostedt]

On Fri, 8 May 2009, Li Zefan wrote:

> Frederic Weisbecker wrote:
> > On Fri, May 08, 2009 at 10:31:42AM +0800, Li Zefan wrote:
> >> Add a helper function __ftrace_set_clr_event(), and replace some
> >> ftrace_set_clr_event() calls with this helper, thus we don't need any
> >> kstrdup() or kmalloc().
> >>
> >> As a side effect, this patch fixes an issue in self tests code, which is
> >> similar to the one fixed in commit d6bf81ef0f7474434c2a049e8bf3c9146a14dd96
> >> ("tracing: append ":*" to internal setting of system events")

Actually, what does this patch solve that the above mentioned commit does 
not?

> >>
> >> It's a small issue and won't cause any bug in fact, but we should do things
> >> right anyway.
> >>
> >> [ Impact: clean up ]
> > 
> > If this fixes an issue like you described, then it's more than a cleanup :)
> > 
> 
> That issue causes no bug, and that's why I call it a cleanup.
> 
> How about (mainly stealed from commit d6bf81ef0f7474434c2a049e8bf3c9146a14dd96):
> 
> [ Impact: prevent accidental enabling of events with same name as a system in self tests ]
> 
> But it excceeds 80 char..
> 
> I sometimes feel it hard to write Impact line (one of the reason is my limit
> English skill). I've explained the impact of this patch in detail, but I'm
> still required to add a one-line summary. :(

I find the Impact line much easier to write when I think of it as the 
reason for the patch (think, why am I writing this). Here I would have 
written:

[ Impact: remove use of kmalloc and kstrdup with cleaner code ]

> 
> > 
> ...
> >> +		if (event && strcmp(event, call->name) != 0)
> >> +			continue;
> > 
> > 
> > Neat: You can simply use !strcmp(...)
> > 
> 
> Actually it's arguable which is better, and both styles are used in kernel code.
> 
> And that 'if (!ptr)' vs 'if (ptr == NULL)'..
> 

I preferer if (!ptr) over if (ptr == NULL), but this has nothing to do 
with the reason for the "strcmp(...) != 0". It is totally different, and 
it fooled you too ;-)

As I mentioned to Frederic, !strcmp() actually means "these strings 
match". Which fools the human brain all too easy. Thus we see,
"strcmp(...) == 0" as "is a match" and "strcmp(...) != 0" as not a match, 
because our brain focuses on the "==" and the "!=". Those that program in 
C, default "!" as not.

Note, strcmp being 0 for match has nothing to do with the C convention of 
0 being non error. But because it is used in sorting algorithms. strcmp 
will return > 0 if it deems str1 > str2, or it will return < 0 if it deems 
str1 < str2, and of course it returns 0 on match.

-- Steve

Re: [PATCH 1/2] tracing/events: clean up for ftrace_set_clr_event()

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Fri, May 08, 2009 at 07:16:08PM +0800, Li Zefan wrote:
> > Frederic Weisbecker wrote:
> > > On Fri, May 08, 2009 at 10:31:42AM +0800, Li Zefan wrote:
> > >> Add a helper function __ftrace_set_clr_event(), and replace some
> > >> ftrace_set_clr_event() calls with this helper, thus we don't need any
> > >> kstrdup() or kmalloc().
> > >>
> > >> As a side effect, this patch fixes an issue in self tests code, which is
> > >> similar to the one fixed in commit d6bf81ef0f7474434c2a049e8bf3c9146a14dd96
> > >> ("tracing: append ":*" to internal setting of system events")
> > >>
> > >> It's a small issue and won't cause any bug in fact, but we should do things
> > >> right anyway.
> > >>
> > >> [ Impact: clean up ]
> > > 
> > > If this fixes an issue like you described, then it's more than a cleanup :)
> > > 
> > 
> > That issue causes no bug, and that's why I call it a cleanup.
> > 
> > How about (mainly stealed from commit d6bf81ef0f7474434c2a049e8bf3c9146a14dd96):
> > 
> > [ Impact: prevent accidental enabling of events with same name as a system in self tests ]
> > 
> > But it excceeds 80 char..
> > 
> > I sometimes feel it hard to write Impact line (one of the reason is my limit
> > English skill). I've explained the impact of this patch in detail, but I'm
> > still required to add a one-line summary. :(
> 
> 
> Well, I also find hard to write straightforward and good matching
> impact lines.
> And I'm certainly not well suited to give any advices about how
> to write good impact lines.
> 
> But IMHO you can sum up your above impact line.
> 
> [ Impact: prevent spurious events enabling in tracing selftests ]

Thanks, i used this minor variant of it:

[ Impact: prevent spurious event-enabling in tracing self-tests ]

> They usually don't need more details, those details can be placed 
> in the changelog. It's more about the general pratical impact, not 
> a detailed one.

Yeah. There's two ends of the spectrum. The too terse:

 [ Impact: fix ]

that one is unhelpful beacause it's largely meaningless.

The too verbose:

 [ Impact: fix crash in foo while bar was setting baz to more
           than alice when charlie saw the full moon, by setting
           blah to bleh ]

Which is unhelpful because long impact lines tend to include 
implementational details while the goal is a 'quick practical impact 
on system summary'.

Best one is to find some middle ground. In any case, dont worry 
about getting it wrong either, it's an iterative process.

	Ingo

[tip:tracing/core] tracing/events: clean up for ftrace_set_clr_event()

[tip-bot for Li Zefan]

Commit-ID:  8f31bfe538ebafac187d2d4465a92e1d9ee6d8c2
Gitweb:     http://git.kernel.org/tip/8f31bfe538ebafac187d2d4465a92e1d9ee6d8c2
Author:     Li Zefan <lizf@cn.fujitsu.com>
AuthorDate: Fri, 8 May 2009 10:31:42 +0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 8 May 2009 14:00:35 +0200

tracing/events: clean up for ftrace_set_clr_event()

Add a helper function __ftrace_set_clr_event(), and replace some
ftrace_set_clr_event() calls with this helper, thus we don't need any
kstrdup() or kmalloc().

As a side effect, this patch fixes an issue in self tests code, which is
similar to the one fixed in commit d6bf81ef0f7474434c2a049e8bf3c9146a14dd96
("tracing: append ":*" to internal setting of system events")

It's a small issue and won't cause any bug in fact, but we should do things
right anyway.

[ Impact: prevent spurious event-enabling in tracing self-tests ]

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
LKML-Reference: <4A03998E.3020503@cn.fujitsu.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 kernel/trace/trace_events.c |  126 ++++++++++++++++---------------------------
 1 files changed, 46 insertions(+), 80 deletions(-)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 8d0fae3..45f1099 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -111,11 +111,44 @@ static void ftrace_event_enable_disable(struct ftrace_event_call *call,
 	}
 }
 
-static int ftrace_set_clr_event(char *buf, int set)
+/*
+ * __ftrace_set_clr_event(NULL, NULL, NULL, set) will set/unset all events.
+ */
+static int __ftrace_set_clr_event(const char *match, const char *sub,
+				  const char *event, int set)
 {
 	struct ftrace_event_call *call;
+	int ret;
+
+	mutex_lock(&event_mutex);
+	list_for_each_entry(call, &ftrace_events, list) {
+
+		if (!call->name || !call->regfunc)
+			continue;
+
+		if (match &&
+		    strcmp(match, call->name) != 0 &&
+		    strcmp(match, call->system) != 0)
+			continue;
+
+		if (sub && strcmp(sub, call->system) != 0)
+			continue;
+
+		if (event && strcmp(event, call->name) != 0)
+			continue;
+
+		ftrace_event_enable_disable(call, set);
+
+		ret = 0;
+	}
+	mutex_unlock(&event_mutex);
+
+	return ret;
+}
+
+static int ftrace_set_clr_event(char *buf, int set)
+{
 	char *event = NULL, *sub = NULL, *match;
-	int ret = -EINVAL;
 
 	/*
 	 * The buf format can be <subsystem>:<event-name>
@@ -141,30 +174,7 @@ static int ftrace_set_clr_event(char *buf, int set)
 			event = NULL;
 	}
 
-	mutex_lock(&event_mutex);
-	list_for_each_entry(call, &ftrace_events, list) {
-
-		if (!call->name || !call->regfunc)
-			continue;
-
-		if (match &&
-		    strcmp(match, call->name) != 0 &&
-		    strcmp(match, call->system) != 0)
-			continue;
-
-		if (sub && strcmp(sub, call->system) != 0)
-			continue;
-
-		if (event && strcmp(event, call->name) != 0)
-			continue;
-
-		ftrace_event_enable_disable(call, set);
-
-		ret = 0;
-	}
-	mutex_unlock(&event_mutex);
-
-	return ret;
+	return __ftrace_set_clr_event(match, sub, event, set);
 }
 
 /* 128 should be much more than enough */
@@ -408,18 +418,14 @@ system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
 	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)
+		if (system && strcmp(call->system, system) != 0)
 			continue;
 
 		/*
@@ -480,7 +486,6 @@ system_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
 {
 	const char *system = filp->private_data;
 	unsigned long val;
-	char *command;
 	char buf[64];
 	ssize_t ret;
 
@@ -500,30 +505,16 @@ system_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
 	if (ret < 0)
 		return ret;
 
-	switch (val) {
-	case 0:
-	case 1:
-		break;
-
-	default:
+	if (val != 0 && val != 1)
 		return -EINVAL;
-	}
 
-	/* +3 for the ":*\0" */
-	command = kmalloc(strlen(system)+3, GFP_KERNEL);
-	if (!command)
-		return -ENOMEM;
-	sprintf(command, "%s:*", system);
-
-	ret = ftrace_set_clr_event(command, val);
+	ret = __ftrace_set_clr_event(NULL, system, NULL, val);
 	if (ret)
-		goto out_free;
+		goto out;
 
 	ret = cnt;
 
- out_free:
-	kfree(command);
-
+out:
 	*ppos += cnt;
 
 	return ret;
@@ -1181,7 +1172,7 @@ static __init int event_trace_init(void)
 			  &ftrace_show_header_fops);
 
 	trace_create_file("enable", 0644, d_events,
-			  "*", &ftrace_system_enable_fops);
+			  NULL, &ftrace_system_enable_fops);
 
 	for_each_event(call, __start_ftrace_events, __stop_ftrace_events) {
 		/* The linker may leave blanks */
@@ -1259,7 +1250,6 @@ static __init void event_trace_self_tests(void)
 {
 	struct ftrace_event_call *call;
 	struct event_subsystem *system;
-	char *sysname;
 	int ret;
 
 	pr_info("Running tests on trace events:\n");
@@ -1305,14 +1295,7 @@ static __init void event_trace_self_tests(void)
 
 		pr_info("Testing event system %s: ", system->name);
 
-		/* ftrace_set_clr_event can modify the name passed in. */
-		sysname = kstrdup(system->name, GFP_KERNEL);
-		if (WARN_ON(!sysname)) {
-			pr_warning("Can't allocate memory, giving up!\n");
-			return;
-		}
-		ret = ftrace_set_clr_event(sysname, 1);
-		kfree(sysname);
+		ret = __ftrace_set_clr_event(NULL, system->name, NULL, 1);
 		if (WARN_ON_ONCE(ret)) {
 			pr_warning("error enabling system %s\n",
 				   system->name);
@@ -1321,14 +1304,7 @@ static __init void event_trace_self_tests(void)
 
 		event_test_stuff();
 
-		sysname = kstrdup(system->name, GFP_KERNEL);
-		if (WARN_ON(!sysname)) {
-			pr_warning("Can't allocate memory, giving up!\n");
-			return;
-		}
-		ret = ftrace_set_clr_event(sysname, 0);
-		kfree(sysname);
-
+		ret = __ftrace_set_clr_event(NULL, system->name, NULL, 0);
 		if (WARN_ON_ONCE(ret))
 			pr_warning("error disabling system %s\n",
 				   system->name);
@@ -1341,15 +1317,8 @@ static __init void event_trace_self_tests(void)
 	pr_info("Running tests on all trace events:\n");
 	pr_info("Testing all events: ");
 
-	sysname = kmalloc(4, GFP_KERNEL);
-	if (WARN_ON(!sysname)) {
-		pr_warning("Can't allocate memory, giving up!\n");
-		return;
-	}
-	memcpy(sysname, "*:*", 4);
-	ret = ftrace_set_clr_event(sysname, 1);
+	ret = __ftrace_set_clr_event(NULL, NULL, NULL, 1);
 	if (WARN_ON_ONCE(ret)) {
-		kfree(sysname);
 		pr_warning("error enabling all events\n");
 		return;
 	}
@@ -1357,10 +1326,7 @@ static __init void event_trace_self_tests(void)
 	event_test_stuff();
 
 	/* reset sysname */
-	memcpy(sysname, "*:*", 4);
-	ret = ftrace_set_clr_event(sysname, 0);
-	kfree(sysname);
-
+	ret = __ftrace_set_clr_event(NULL, NULL, NULL, 0);
 	if (WARN_ON_ONCE(ret)) {
 		pr_warning("error disabling all events\n");
 		return;

[tip:tracing/core] tracing/events: simplify system_enable_read()

[tip-bot for Li Zefan]

Commit-ID:  c142b15dc56ee6d55cb97a062e3c8e9c61e384c0
Gitweb:     http://git.kernel.org/tip/c142b15dc56ee6d55cb97a062e3c8e9c61e384c0
Author:     Li Zefan <lizf@cn.fujitsu.com>
AuthorDate: Fri, 8 May 2009 10:32:05 +0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 8 May 2009 14:00:36 +0200

tracing/events: simplify system_enable_read()

A smarter way to figure out the output of an enable file.

[ Impact: clean up ]

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
LKML-Reference: <4A0399A5.2080603@cn.fujitsu.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 kernel/trace/trace_events.c |   40 ++++++----------------------------------
 1 files changed, 6 insertions(+), 34 deletions(-)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 45f1099..df394bc 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -414,10 +414,11 @@ static ssize_t
 system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
 		   loff_t *ppos)
 {
+	const char set_to_char[4] = { '?', '0', '1', 'X' };
 	const char *system = filp->private_data;
 	struct ftrace_event_call *call;
 	char buf[2];
-	int set = -1;
+	int set = 0;
 	int ret;
 
 	mutex_lock(&event_mutex);
@@ -433,47 +434,18 @@ system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
 		 * 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;
-			}
-		}
+		set |= (1 << !!call->enabled);
+
 		/*
 		 * If we have a mixture, no need to look further.
 		 */
-		if (set == 2)
+		if (set == 3)
 			break;
 	}
 	mutex_unlock(&event_mutex);
 
+	buf[0] = set_to_char[set];
 	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);
 

Re: [PATCH 2/2] tracing/events: simplify system_enable_read()

[Andrew Morton]

On Fri, 08 May 2009 10:32:05 +0800
Li Zefan <lizf@cn.fujitsu.com> wrote:

> A smarter way to figure out the output of an enable file.
> 
> [ Impact: clean up ]
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/trace/trace_events.c |   40 ++++++----------------------------------
>  1 files changed, 6 insertions(+), 34 deletions(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 45f1099..df394bc 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -414,10 +414,11 @@ static ssize_t
>  system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
>  		   loff_t *ppos)
>  {
> +	const char set_to_char[4] = { '?', '0', '1', 'X' };

I always worry when I see this, but gcc does manage to add the missing
`static' all by itself.  For which versions of gcc that is true I don't
know.

> +	buf[0] = set_to_char[set];

Meh.  Real men do

	buf[0] = "?01X"[set];

Re: [PATCH 2/2] tracing/events: simplify system_enable_read()

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Fri, 08 May 2009 10:32:05 +0800
> Li Zefan <lizf@cn.fujitsu.com> wrote:
> 
> > A smarter way to figure out the output of an enable file.
> > 
> > [ Impact: clean up ]
> > 
> > Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> > ---
> >  kernel/trace/trace_events.c |   40 ++++++----------------------------------
> >  1 files changed, 6 insertions(+), 34 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> > index 45f1099..df394bc 100644
> > --- a/kernel/trace/trace_events.c
> > +++ b/kernel/trace/trace_events.c
> > @@ -414,10 +414,11 @@ static ssize_t
> >  system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
> >  		   loff_t *ppos)
> >  {
> > +	const char set_to_char[4] = { '?', '0', '1', 'X' };
> 
> I always worry when I see this, but gcc does manage to add the missing
> `static' all by itself.  For which versions of gcc that is true I don't
> know.
> 
> > +	buf[0] = set_to_char[set];
> 
> Meh.  Real men do
> 
> 	buf[0] = "?01X"[set];

heh. That deserves a commit ...

	Ingo