Re: [PATCH] Disable branch profiling macros when sparsed.

[Steven Rostedt]

On Sat, 10 Jan 2009, Harvey Harrison wrote:

> On Sat, 2009-01-10 at 12:35 +0300, Alexey Zaytsev wrote:
> > On Sat, Jan 10, 2009 at 10:18, Harvey Harrison
> > <harvey.harrison@xxxxxxxxx> wrote:
> > > On Fri, 2009-01-09 at 22:13 -0800, David Miller wrote:
> > >> If even sparse can't handle these things, it's no surprise
> > >> how many gcc bogus warning problems we've run into because
> > >> of this hairy if() macro.
> > >
> > > It's not that sparse can't handle it, the warning is valid,
> > > _____r and ______f are shadowed when these get nested.  It
> > > gets even worse when interacting with likely/unlikely tracing
> > > as that chose the same identifiers too.  So there the noise
> > > could be drastically reduced changing the different identifiers
> > > for the if () and __branch_check macros, but nesting will always
> > > warn.
> > >
> > > I've just been setting this to no in my allyesconfig sparse
> > > runs....just wait until kmemtrace gets to mainline, then it
> > > gets really bad :(
> > >
> > 
> > I don't really understand what is bad here. The 'unlikely' and 'if'
> > trace implementation looks quite elegant to me. Yes, they generate
> > 10kbyte spaghetti monsters (in C) for a simple WARN_ON_ONCE(),
> > but probably we should just remove a few unlekely() from the WARN_*
> > code, and I'm not sure it's even worth it. There would be no direct
> > speedup.
> > 
> > And it took only one line to disable.
> 
> I'm not saying anything about ftrace being bad here, it's a pretty
> elegant way of doing is.

Thanks!

> 
> But instead of disabling it, a patch like the following eliminates
> most of the warnings even when enabled, it relies on making the
> frace_*_update functions return the condition that is being updated
> which removes the need for an _____r temporary.
> 
> Also I changed the ______f's to be ______bc/bd (branch check, branch
> data)...but those are arbitrary.
> 
> Untested other than kills the sparse warnings that are caused by nesting
> if(likely())..nested ifs stil warning but only on _____bc which is far
> less common.
> 
> It's very possible this breaks ftrace or produces shitty code...consider
> it just an idea to add an update function that takes/returns the
> condition.

I'll give this a try on Monday (no work over the weekend, wife's orders).

Since the problem seems to stem from the branch tracer's being selected
via a allyesconfig, and most people are complaining about that,
one solution, albeit not a very good one, is to convert it into a choice
instead of a binary.

We can have:

choice
	prompt "Branch Profiling Support"
	default NO_BRANCH_PROFILING

config NO_BRANCH_PROFILING
	bool "No branch profiling"

config ANNOTATED_BRANCH_PROFILING
	bool "Profile only likely and unlikely branches"

config ALL_BRANCH_PROFILING
	bool "Profile all if conditionals"

endchoice

This will keep allyesconfig from selecting any type of branch profiling,
but it will not protect against warnings from randconfig.

Would this be an option people would prefer?

-- Steve

> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index d95da10..e8e85be 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -76,24 +76,21 @@ struct ftrace_branch_data {
>   * to disable branch tracing on a per file basis.
>   */
>  #if defined(CONFIG_TRACE_BRANCH_PROFILING) && !defined(DISABLE_BRANCH_PROFILING)
> -void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
> +int ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>  
>  #define likely_notrace(x)	__builtin_expect(!!(x), 1)
>  #define unlikely_notrace(x)	__builtin_expect(!!(x), 0)
>  
>  #define __branch_check__(x, expect) ({					\
> -			int ______r;					\
>  			static struct ftrace_branch_data		\
>  				__attribute__((__aligned__(4)))		\
>  				__attribute__((section("_ftrace_annotated_branch"))) \
> -				______f = {				\
> +				______bc = {				\
>  				.func = __func__,			\
>  				.file = __FILE__,			\
>  				.line = __LINE__,			\
>  			};						\
> -			______r = likely_notrace(x);			\
> -			ftrace_likely_update(&______f, ______r, expect); \
> -			______r;					\
> +			ftrace_likely_update(&______bc, likely_notrace(x), expect); \
>  		})
>  
>  /*
> @@ -109,27 +106,32 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>  # endif
>  
>  #ifdef CONFIG_PROFILE_ALL_BRANCHES
> +
> +static inline int ftrace_if_update(struct ftrace_branch_data *bd, int cond)
> +{
> +	if (cond)
> +		bd->hit++;
> +	else
> +		bd->miss++;
> +
> +	return cond;
> +}
> +
>  /*
>   * "Define 'is'", Bill Clinton
>   * "Define 'if'", Steven Rostedt
>   */
>  #define if(cond) if (__builtin_constant_p((cond)) ? !!(cond) :		\
>  	({								\
> -		int ______r;						\
>  		static struct ftrace_branch_data			\
>  			__attribute__((__aligned__(4)))			\
>  			__attribute__((section("_ftrace_branch")))	\
> -			______f = {					\
> +			______bd = {					\
>  				.func = __func__,			\
>  				.file = __FILE__,			\
>  				.line = __LINE__,			\
>  			};						\
> -		______r = !!(cond);					\
> -		if (______r)						\
> -			______f.hit++;					\
> -		else							\
> -			______f.miss++;					\
> -		______r;						\
> +		ftrace_if_update(&______bd, !!(cond));			\
>  	}))
>  #endif /* CONFIG_PROFILE_ALL_BRANCHES */
>  
> diff --git a/kernel/trace/trace_branch.c b/kernel/trace/trace_branch.c
> index 6c00feb..385d608 100644
> --- a/kernel/trace/trace_branch.c
> +++ b/kernel/trace/trace_branch.c
> @@ -165,7 +165,7 @@ void trace_likely_condition(struct ftrace_branch_data *f, int val, int expect)
>  }
>  #endif /* CONFIG_BRANCH_TRACER */
>  
> -void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect)
> +int ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect)
>  {
>  	/*
>  	 * I would love to have a trace point here instead, but the
> @@ -180,6 +180,8 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect)
>  		f->correct++;
>  	else
>  		f->incorrect++;
> +
> +	return val;
>  }
>  EXPORT_SYMBOL(ftrace_likely_update);
>  
> 
> 
> 
> 
> 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/