[PATCH 1/2] tracing/function: fix the return value of ftrace_trace_onoff_callback()

[PATCH 1/2] tracing/function: fix the return value of ftrace_trace_onoff_callback()

[Xiao Guangrong]

ftrace_trace_onoff_callback() will return error even if we do the
right operation, for example:

# echo _spin_*:traceon:10 > set_ftrace_filter
-bash: echo: write error: Invalid argument
# cat set_ftrace_filter
#### all functions enabled ####
_spin_trylock_bh:traceon:count=10
_spin_unlock_irq:traceon:count=10
_spin_unlock_bh:traceon:count=10
_spin_lock_irq:traceon:count=10
_spin_unlock:traceon:count=10
_spin_trylock:traceon:count=10
_spin_unlock_irqrestore:traceon:count=10
_spin_lock_irqsave:traceon:count=10
_spin_lock_bh:traceon:count=10
_spin_lock:traceon:count=10

We want to set _spin_*:traceon:10 to set_ftrace_filter, it complain
with "Invalid argument", but the operation is successful.
So, this patch fix it.

Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
---
 kernel/trace/trace_functions.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index 7402144..75ef000 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -363,7 +363,7 @@ ftrace_trace_onoff_callback(char *glob, char *cmd, char *param, int enable)
  out_reg:
 	ret = register_ftrace_function_probe(glob, ops, count);
 
-	return ret;
+	return ret < 0 ? ret : 0;
 }
 
 static struct ftrace_func_command ftrace_traceon_cmd = {
-- 
1.6.1.2

[PATCH 2/2] tracing/function: simplify for __ftrace_replace_code()

[Xiao Guangrong]

Rewrite the __ftrace_replace_code() function, let it's simple, but
not change the code's logic.

First, we get the state we want to set, if the record has the same
state, then do nothing, if not, enable/disable it.

Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
---
 kernel/trace/ftrace.c |   75 +++++++++++++-----------------------------------
 1 files changed, 21 insertions(+), 54 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 4521c77..18aeb8d 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1016,71 +1016,38 @@ static int
 __ftrace_replace_code(struct dyn_ftrace *rec, int enable)
 {
 	unsigned long ftrace_addr;
-	unsigned long ip, fl;
+	unsigned long flag = 0UL;
 
 	ftrace_addr = (unsigned long)FTRACE_ADDR;
 
-	ip = rec->ip;
-
 	/*
-	 * If this record is not to be traced and
-	 * it is not enabled then do nothing.
+	 * If this record is not to be traced or we want to disable it,
+	 * then disable it.
 	 *
-	 * If this record is not to be traced and
-	 * it is enabled then disable it.
+	 * If we want to enable it and filtering is off, then enable it.
 	 *
+	 * If we want to enable it and filtering is on, enable it only if
+	 * it's filtered
 	 */
-	if (rec->flags & FTRACE_FL_NOTRACE) {
-		if (rec->flags & FTRACE_FL_ENABLED)
-			rec->flags &= ~FTRACE_FL_ENABLED;
-		else
-			return 0;
-
-	} else if (ftrace_filtered && enable) {
-		/*
-		 * Filtering is on:
-		 */
-
-		fl = rec->flags & (FTRACE_FL_FILTER | FTRACE_FL_ENABLED);
-
-		/* Record is filtered and enabled, do nothing */
-		if (fl == (FTRACE_FL_FILTER | FTRACE_FL_ENABLED))
-			return 0;
-
-		/* Record is not filtered or enabled, do nothing */
-		if (!fl)
-			return 0;
-
-		/* Record is not filtered but enabled, disable it */
-		if (fl == FTRACE_FL_ENABLED)
-			rec->flags &= ~FTRACE_FL_ENABLED;
-		else
-		/* Otherwise record is filtered but not enabled, enable it */
-			rec->flags |= FTRACE_FL_ENABLED;
-	} else {
-		/* Disable or not filtered */
-
-		if (enable) {
-			/* if record is enabled, do nothing */
-			if (rec->flags & FTRACE_FL_ENABLED)
-				return 0;
-
-			rec->flags |= FTRACE_FL_ENABLED;
-
-		} else {
+	if (enable && !(rec->flags & FTRACE_FL_NOTRACE)) {
+		if (ftrace_filtered) {
+			if (rec->flags & FTRACE_FL_FILTER)
+				flag = FTRACE_FL_ENABLED;
+		} else
+			flag = FTRACE_FL_ENABLED;
+	}
 
-			/* if record is not enabled, do nothing */
-			if (!(rec->flags & FTRACE_FL_ENABLED))
-				return 0;
+	/* If not change this record's state, then do nothing */
+	if ((rec->flags & FTRACE_FL_ENABLED) == flag)
+		return 0;
 
-			rec->flags &= ~FTRACE_FL_ENABLED;
-		}
+	if (flag) {
+		rec->flags |= FTRACE_FL_ENABLED;
+		return ftrace_make_call(rec, ftrace_addr);
 	}
 
-	if (rec->flags & FTRACE_FL_ENABLED)
-		return ftrace_make_call(rec, ftrace_addr);
-	else
-		return ftrace_make_nop(NULL, rec, ftrace_addr);
+	rec->flags &= ~FTRACE_FL_ENABLED;
+	return ftrace_make_nop(NULL, rec, ftrace_addr);
 }
 
 static void ftrace_replace_code(int enable)
-- 
1.6.1.2

Re: [PATCH 2/2] tracing/function: simplify for __ftrace_replace_code()

[Li Zefan]

Xiao Guangrong wrote:
> Rewrite the __ftrace_replace_code() function, let it's simple, but
> not change the code's logic.
> 
> First, we get the state we want to set, if the record has the same
> state, then do nothing, if not, enable/disable it.
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>

Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>

> ---
>  kernel/trace/ftrace.c |   75 +++++++++++++-----------------------------------
>  1 files changed, 21 insertions(+), 54 deletions(-)

Re: [PATCH 2/2] tracing/function: simplify for __ftrace_replace_code()

[Frederic Weisbecker]

On Wed, Jul 15, 2009 at 12:32:15PM +0800, Xiao Guangrong wrote:
> Rewrite the __ftrace_replace_code() function, let it's simple, but
> not change the code's logic.
> 
> First, we get the state we want to set, if the record has the same
> state, then do nothing, if not, enable/disable it.
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> ---
>  kernel/trace/ftrace.c |   75 +++++++++++++-----------------------------------
>  1 files changed, 21 insertions(+), 54 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 4521c77..18aeb8d 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1016,71 +1016,38 @@ static int
>  __ftrace_replace_code(struct dyn_ftrace *rec, int enable)
>  {
>  	unsigned long ftrace_addr;
> -	unsigned long ip, fl;
> +	unsigned long flag = 0UL;
>  
>  	ftrace_addr = (unsigned long)FTRACE_ADDR;
>  
> -	ip = rec->ip;
> -
>  	/*
> -	 * If this record is not to be traced and
> -	 * it is not enabled then do nothing.
> +	 * If this record is not to be traced or we want to disable it,
> +	 * then disable it.
>  	 *
> -	 * If this record is not to be traced and
> -	 * it is enabled then disable it.
> +	 * If we want to enable it and filtering is off, then enable it.
>  	 *
> +	 * If we want to enable it and filtering is on, enable it only if
> +	 * it's filtered
>  	 */
> -	if (rec->flags & FTRACE_FL_NOTRACE) {
> -		if (rec->flags & FTRACE_FL_ENABLED)
> -			rec->flags &= ~FTRACE_FL_ENABLED;
> -		else
> -			return 0;
> -
> -	} else if (ftrace_filtered && enable) {
> -		/*
> -		 * Filtering is on:
> -		 */
> -
> -		fl = rec->flags & (FTRACE_FL_FILTER | FTRACE_FL_ENABLED);
> -
> -		/* Record is filtered and enabled, do nothing */
> -		if (fl == (FTRACE_FL_FILTER | FTRACE_FL_ENABLED))
> -			return 0;
> -
> -		/* Record is not filtered or enabled, do nothing */
> -		if (!fl)
> -			return 0;
> -
> -		/* Record is not filtered but enabled, disable it */
> -		if (fl == FTRACE_FL_ENABLED)
> -			rec->flags &= ~FTRACE_FL_ENABLED;
> -		else
> -		/* Otherwise record is filtered but not enabled, enable it */
> -			rec->flags |= FTRACE_FL_ENABLED;
> -	} else {
> -		/* Disable or not filtered */
> -
> -		if (enable) {
> -			/* if record is enabled, do nothing */
> -			if (rec->flags & FTRACE_FL_ENABLED)
> -				return 0;
> -
> -			rec->flags |= FTRACE_FL_ENABLED;
> -
> -		} else {
> +	if (enable && !(rec->flags & FTRACE_FL_NOTRACE)) {
> +		if (ftrace_filtered) {
> +			if (rec->flags & FTRACE_FL_FILTER)
> +				flag = FTRACE_FL_ENABLED;
> +		} else
> +			flag = FTRACE_FL_ENABLED;


The above can be factorized in

if (!ftrace_filtered || rec->flags & FTRACE_FL_FILTER)
	flag = FTRACE_FL_ENABLED



> +	}
>  
> -			/* if record is not enabled, do nothing */
> -			if (!(rec->flags & FTRACE_FL_ENABLED))
> -				return 0;
> +	/* If not change this record's state, then do nothing */
> +	if ((rec->flags & FTRACE_FL_ENABLED) == flag)
> +		return 0;
>  
> -			rec->flags &= ~FTRACE_FL_ENABLED;
> -		}
> +	if (flag) {
> +		rec->flags |= FTRACE_FL_ENABLED;
> +		return ftrace_make_call(rec, ftrace_addr);
>  	}
>  
> -	if (rec->flags & FTRACE_FL_ENABLED)
> -		return ftrace_make_call(rec, ftrace_addr);
> -	else
> -		return ftrace_make_nop(NULL, rec, ftrace_addr);
> +	rec->flags &= ~FTRACE_FL_ENABLED;
> +	return ftrace_make_nop(NULL, rec, ftrace_addr);
>  }
>  
>  static void ftrace_replace_code(int enable)
> -- 
> 1.6.1.2
> 
> 


Nice simplification.
I'm queuing it for .32 (I will just change with my comment above).

Thanks!

Re: [PATCH 2/2] tracing/function: simplify for __ftrace_replace_code()

[Li Zefan]

>> +	if (enable && !(rec->flags & FTRACE_FL_NOTRACE)) {
>> +		if (ftrace_filtered) {
>> +			if (rec->flags & FTRACE_FL_FILTER)
>> +				flag = FTRACE_FL_ENABLED;
>> +		} else
>> +			flag = FTRACE_FL_ENABLED;
> 
> 
> The above can be factorized in
> 
> if (!ftrace_filtered || rec->flags & FTRACE_FL_FILTER)
> 	flag = FTRACE_FL_ENABLED
> 

I think it's better to put the latter condition into parentheses:

if (!ftrace_filtered || (rec->flags & FTRACE_FL_FILTER))
	flag = FTRACE_FL_ENABLED

Re: [PATCH 2/2] tracing/function: simplify for __ftrace_replace_code()

[Frederic Weisbecker]

On Fri, Jul 17, 2009 at 12:30:07PM +0800, Li Zefan wrote:
> >> +	if (enable && !(rec->flags & FTRACE_FL_NOTRACE)) {
> >> +		if (ftrace_filtered) {
> >> +			if (rec->flags & FTRACE_FL_FILTER)
> >> +				flag = FTRACE_FL_ENABLED;
> >> +		} else
> >> +			flag = FTRACE_FL_ENABLED;
> > 
> > 
> > The above can be factorized in
> > 
> > if (!ftrace_filtered || rec->flags & FTRACE_FL_FILTER)
> > 	flag = FTRACE_FL_ENABLED
> > 
> 
> I think it's better to put the latter condition into parentheses:
> 
> if (!ftrace_filtered || (rec->flags & FTRACE_FL_FILTER))
> 	flag = FTRACE_FL_ENABLED
> 

Ok. Done.

Re: [PATCH 1/2] tracing/function: fix the return value of ftrace_trace_onoff_callback()

[Frederic Weisbecker]

On Wed, Jul 15, 2009 at 12:29:06PM +0800, Xiao Guangrong wrote:
> ftrace_trace_onoff_callback() will return error even if we do the
> right operation, for example:
> 
> # echo _spin_*:traceon:10 > set_ftrace_filter
> -bash: echo: write error: Invalid argument
> # cat set_ftrace_filter
> #### all functions enabled ####
> _spin_trylock_bh:traceon:count=10
> _spin_unlock_irq:traceon:count=10
> _spin_unlock_bh:traceon:count=10
> _spin_lock_irq:traceon:count=10
> _spin_unlock:traceon:count=10
> _spin_trylock:traceon:count=10
> _spin_unlock_irqrestore:traceon:count=10
> _spin_lock_irqsave:traceon:count=10
> _spin_lock_bh:traceon:count=10
> _spin_lock:traceon:count=10
> 
> We want to set _spin_*:traceon:10 to set_ftrace_filter, it complain
> with "Invalid argument", but the operation is successful.
> So, this patch fix it.
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>



Looks good, thanks.

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


> ---
>  kernel/trace/trace_functions.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> index 7402144..75ef000 100644
> --- a/kernel/trace/trace_functions.c
> +++ b/kernel/trace/trace_functions.c
> @@ -363,7 +363,7 @@ ftrace_trace_onoff_callback(char *glob, char *cmd, char *param, int enable)
>   out_reg:
>  	ret = register_ftrace_function_probe(glob, ops, count);
>  
> -	return ret;
> +	return ret < 0 ? ret : 0;
>  }
>  
>  static struct ftrace_func_command ftrace_traceon_cmd = {
> -- 
> 1.6.1.2
> 

Re: [PATCH 1/2] tracing/function: fix the return value of ftrace_trace_onoff_callback()

[Li Zefan]

>> ftrace_trace_onoff_callback() will return error even if we do the
>> right operation, for example:
>>
>> # echo _spin_*:traceon:10 > set_ftrace_filter
>> -bash: echo: write error: Invalid argument
>> # cat set_ftrace_filter
>> #### all functions enabled ####
>> _spin_trylock_bh:traceon:count=10
>> _spin_unlock_irq:traceon:count=10
>> _spin_unlock_bh:traceon:count=10
>> _spin_lock_irq:traceon:count=10
>> _spin_unlock:traceon:count=10
>> _spin_trylock:traceon:count=10
>> _spin_unlock_irqrestore:traceon:count=10
>> _spin_lock_irqsave:traceon:count=10
>> _spin_lock_bh:traceon:count=10
>> _spin_lock:traceon:count=10
>>
>> We want to set _spin_*:traceon:10 to set_ftrace_filter, it complain
>> with "Invalid argument", but the operation is successful.
>> So, this patch fix it.
>>
>> Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> 
> Looks good, thanks.
> 
> Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
> 

Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>

Re: [PATCH 1/2] tracing/function: fix the return value of ftrace_trace_onoff_callback()

[Frederic Weisbecker]

On Thu, Jul 16, 2009 at 05:29:27PM +0800, Li Zefan wrote:
> >> ftrace_trace_onoff_callback() will return error even if we do the
> >> right operation, for example:
> >>
> >> # echo _spin_*:traceon:10 > set_ftrace_filter
> >> -bash: echo: write error: Invalid argument
> >> # cat set_ftrace_filter
> >> #### all functions enabled ####
> >> _spin_trylock_bh:traceon:count=10
> >> _spin_unlock_irq:traceon:count=10
> >> _spin_unlock_bh:traceon:count=10
> >> _spin_lock_irq:traceon:count=10
> >> _spin_unlock:traceon:count=10
> >> _spin_trylock:traceon:count=10
> >> _spin_unlock_irqrestore:traceon:count=10
> >> _spin_lock_irqsave:traceon:count=10
> >> _spin_lock_bh:traceon:count=10
> >> _spin_lock:traceon:count=10
> >>
> >> We want to set _spin_*:traceon:10 to set_ftrace_filter, it complain
> >> with "Invalid argument", but the operation is successful.
> >> So, this patch fix it.
> >>
> >> Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> > 
> > Looks good, thanks.
> > 
> > Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
> > 
> 
> Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>
> 


Thanks!
I'm queuing it in the fixes for 2.6.31 and also add a Cc: stable@kernel.org
tag because the fix also applies on .30

I've also detailed a bit more the changelog, see below:
---
>From 04aef32d39cc4ef80087c0ce8ed113c6d64f1a6b Mon Sep 17 00:00:00 2001
From: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
Date: Wed, 15 Jul 2009 12:29:06 +0800
Subject: [PATCH] tracing/function: Fix the return value of ftrace_trace_onoff_callback()

ftrace_trace_onoff_callback() will return an error even if we do the
right operation, for example:

 # echo _spin_*:traceon:10 > set_ftrace_filter
 -bash: echo: write error: Invalid argument
 # cat set_ftrace_filter
 #### all functions enabled ####
 _spin_trylock_bh:traceon:count=10
 _spin_unlock_irq:traceon:count=10
 _spin_unlock_bh:traceon:count=10
 _spin_lock_irq:traceon:count=10
 _spin_unlock:traceon:count=10
 _spin_trylock:traceon:count=10
 _spin_unlock_irqrestore:traceon:count=10
 _spin_lock_irqsave:traceon:count=10
 _spin_lock_bh:traceon:count=10
 _spin_lock:traceon:count=10

We want to set _spin_*:traceon:10 to set_ftrace_filter, it complains
with "Invalid argument", but the operation is successful.

This is because ftrace_process_regex() returns the number of functions that
matched the pattern. If the number is not 0, this value is returned
by ftrace_regex_write() whereas we want to return the number of bytes
virtually written.
Also the file offset pointer is not updated in this case.

If the number of matched functions is lower than the number of bytes written
by the user, this results to a reprocessing of the string given by the user with
a lower size, leading to a malformed ftrace regex and then a -EINVAL returned.

So, this patch fixes it by returning 0 if no error occured.
The fix also applies on 2.6.30

Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>
Cc: stable@kernel.org
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/trace/trace_functions.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index 7402144..75ef000 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -363,7 +363,7 @@ ftrace_trace_onoff_callback(char *glob, char *cmd, char *param, int enable)
  out_reg:
 	ret = register_ftrace_function_probe(glob, ops, count);
 
-	return ret;
+	return ret < 0 ? ret : 0;
 }
 
 static struct ftrace_func_command ftrace_traceon_cmd = {
-- 
1.6.2.3