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

[Frederic Weisbecker <fweisbec@gmail.com>]

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!