| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1231623238.5714.60.camel@brick>
Date: Sat, 10 Jan 2009 13:33:58 -0800
From: Harvey Harrison <harvey.harrison@...il.com>
To: Alexey Zaytsev <alexey.zaytsev@...il.com>
Cc: David Miller <davem@...emloft.net>, linux-kernel@...r.kernel.org,
mingo@...e.hu, rostedt@...dmis.org
Subject: Re: [PATCH] Disable branch profiling macros when sparsed.
On Sat, 2009-01-10 at 12:35 +0300, Alexey Zaytsev wrote:
> On Sat, Jan 10, 2009 at 10:18, Harvey Harrison
> <harvey.harrison@...il.com> 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists