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

[Harvey Harrison]

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.

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.

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/