The macros produce lots of unneeded warnings when
recursive if(({ .. if() {..} ..})) {..} and such
are substituted. And there is no point in sparsing
them anyway. This is useful if someone decides to
sparse an allyesconfig kernel.
Signed-off-by: Alexey Zaytsev <[email protected]>
---
Also, there is little point in profiling unlikely() inside
WARN_ON() and friends, so maybe they should be replaced
with the _notrace counterparts?
include/linux/compiler.h | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index d95da10..f5f173b 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -17,6 +17,7 @@
# define __cond_lock(x,c) ((c) ? ({ __acquire(x); 1; }) : 0)
extern void __chk_user_ptr(const volatile void __user *);
extern void __chk_io_ptr(const volatile void __iomem *);
+# define DISABLE_BRANCH_PROFILING
#else
# define __user
# define __kernel
From: Alexey Zaytsev <[email protected]>
Date: Sat, 10 Jan 2009 08:57:28 +0300
> The macros produce lots of unneeded warnings when
> recursive if(({ .. if() {..} ..})) {..} and such
> are substituted. And there is no point in sparsing
> them anyway. This is useful if someone decides to
> sparse an allyesconfig kernel.
>
> Signed-off-by: Alexey Zaytsev <[email protected]>
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.
I don't think it's a good precedence to do this. It's giving
up on trying to implement the branch tracer in a way that
results in a kernel build that produces useful warnings.
On Fri, 2009-01-09 at 22:13 -0800, David Miller wrote:
> From: Alexey Zaytsev <[email protected]>
> Date: Sat, 10 Jan 2009 08:57:28 +0300
>
> > The macros produce lots of unneeded warnings when
> > recursive if(({ .. if() {..} ..})) {..} and such
> > are substituted. And there is no point in sparsing
> > them anyway. This is useful if someone decides to
> > sparse an allyesconfig kernel.
> >
> > Signed-off-by: Alexey Zaytsev <[email protected]>
>
> 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 :(
Cheers,
Harvey
On Sat, Jan 10, 2009 at 10:18, Harvey Harrison
<[email protected]> wrote:
> On Fri, 2009-01-09 at 22:13 -0800, David Miller wrote:
>> From: Alexey Zaytsev <[email protected]>
>> Date: Sat, 10 Jan 2009 08:57:28 +0300
>>
>> > The macros produce lots of unneeded warnings when
>> > recursive if(({ .. if() {..} ..})) {..} and such
>> > are substituted. And there is no point in sparsing
>> > them anyway. This is useful if someone decides to
>> > sparse an allyesconfig kernel.
>> >
>> > Signed-off-by: Alexey Zaytsev <[email protected]>
>>
>> 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.
On Sat, 2009-01-10 at 12:35 +0300, Alexey Zaytsev wrote:
> On Sat, Jan 10, 2009 at 10:18, Harvey Harrison
> <[email protected]> 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);
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
> > <[email protected]> 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);
>
>
>
>
>
>
>
>