So here is the patchses I had in some branches for a little while.
Please double check especially the 3rd patch because it removes the
use of .data.unlikely for WARN_ON_ONCE.
I'm not sure this section for the "done" state is something we also want
for printk_once(). What do you think?
Thanks.
git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
core/once
Thanks,
Frederic
---
Frederic Weisbecker (3):
core: New macro to execute code only once
core: Convert printk_once to use DO_ONCE
bug: Convert warn macros to use once and cond helpers
include/asm-generic/bug.h | 57 ++++++++++-------------------------------------
include/linux/once.h | 26 +++++++++++++++++++++
include/linux/printk.h | 10 ++-------
3 files changed, 40 insertions(+), 53 deletions(-)
Introduce DO_COND(), DO_ONCE() and DO_ONCE_COND(). These
macros should ease the consolidation of CPP code when
it is deemed to be executed only once and/or after a
condition is verified. This incluse printk_once, WARN_ON,
WARN_ON_ONCE, etc...
Signed-off-by: Frederic Weisbecker <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Liu Chuansheng <[email protected]>
Cc: Ingo Molnar <[email protected]>
---
include/linux/once.h | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
create mode 100644 include/linux/once.h
diff --git a/include/linux/once.h b/include/linux/once.h
new file mode 100644
index 0000000..9399f37
--- /dev/null
+++ b/include/linux/once.h
@@ -0,0 +1,26 @@
+#ifndef _LINUX_ONCE_H
+#define _LINUX_ONCE_H
+
+#include <linux/compiler.h>
+
+#define DO_COND(condition, to_do) ({ \
+ int __ret = !!(condition); \
+ if (unlikely(__ret)) { \
+ to_do; \
+ } \
+ unlikely(__ret); \
+})
+
+#define DO_ONCE(to_do) ({ \
+ static bool __done; \
+ \
+ if (!__done) { \
+ __done = true; \
+ to_do; \
+ } \
+})
+
+#define DO_ONCE_COND(condition, to_do) \
+ DO_COND(condition, DO_ONCE(to_do))
+
+#endif /* _LINUX_ONCE_H */
--
1.8.3.1
Unfortunately this removes the use of .data.unlikely code
section for the warned state...
Signed-off-by: Frederic Weisbecker <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Liu Chuansheng <[email protected]>
Cc: Ingo Molnar <[email protected]>
---
include/asm-generic/bug.h | 57 ++++++++++-------------------------------------
1 file changed, 12 insertions(+), 45 deletions(-)
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 7d10f96..2663ea1 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -82,29 +82,17 @@ extern void warn_slowpath_null(const char *file, const int line);
#endif
#ifndef WARN_ON
-#define WARN_ON(condition) ({ \
- int __ret_warn_on = !!(condition); \
- if (unlikely(__ret_warn_on)) \
- __WARN(); \
- unlikely(__ret_warn_on); \
-})
+#define WARN_ON(condition) \
+ DO_COND(condition, __WARN())
#endif
#ifndef WARN
-#define WARN(condition, format...) ({ \
- int __ret_warn_on = !!(condition); \
- if (unlikely(__ret_warn_on)) \
- __WARN_printf(format); \
- unlikely(__ret_warn_on); \
-})
+#define WARN(condition, format...) \
+ DO_COND(condition, __WARN_printf(format))
#endif
-#define WARN_TAINT(condition, taint, format...) ({ \
- int __ret_warn_on = !!(condition); \
- if (unlikely(__ret_warn_on)) \
- __WARN_printf_taint(taint, format); \
- unlikely(__ret_warn_on); \
-})
+#define WARN_TAINT(condition, taint, format...) \
+ DO_COND(condition, __WARN_printf_taint(taint, format))
#else /* !CONFIG_BUG */
#ifndef HAVE_ARCH_BUG
@@ -133,35 +121,14 @@ extern void warn_slowpath_null(const char *file, const int line);
#endif
-#define WARN_ON_ONCE(condition) ({ \
- static bool __section(.data.unlikely) __warned; \
- int __ret_warn_once = !!(condition); \
- \
- if (unlikely(__ret_warn_once)) \
- if (WARN_ON(!__warned)) \
- __warned = true; \
- unlikely(__ret_warn_once); \
-})
+#define WARN_ON_ONCE(condition) \
+ DO_ONCE_COND(condition, __WARN())
-#define WARN_ONCE(condition, format...) ({ \
- static bool __section(.data.unlikely) __warned; \
- int __ret_warn_once = !!(condition); \
- \
- if (unlikely(__ret_warn_once)) \
- if (WARN(!__warned, format)) \
- __warned = true; \
- unlikely(__ret_warn_once); \
-})
+#define WARN_ONCE(condition, format...) \
+ DO_ONCE_COND(condition, __WARN_printf(format))
-#define WARN_TAINT_ONCE(condition, taint, format...) ({ \
- static bool __section(.data.unlikely) __warned; \
- int __ret_warn_once = !!(condition); \
- \
- if (unlikely(__ret_warn_once)) \
- if (WARN_TAINT(!__warned, taint, format)) \
- __warned = true; \
- unlikely(__ret_warn_once); \
-})
+#define WARN_TAINT_ONCE(condition, taint, format...) \
+ DO_ONCE_COND(condition, __WARN_printf_taint(taint, format))
/*
* WARN_ON_SMP() is for cases that the warning is either
--
1.8.3.1
Signed-off-by: Frederic Weisbecker <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Liu Chuansheng <[email protected]>
Cc: Ingo Molnar <[email protected]>
---
include/linux/printk.h | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/include/linux/printk.h b/include/linux/printk.h
index e6131a78..3c5069b 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -5,6 +5,7 @@
#include <linux/init.h>
#include <linux/kern_levels.h>
#include <linux/linkage.h>
+#include <linux/once.h>
extern const char linux_banner[];
extern const char linux_proc_banner[];
@@ -252,14 +253,7 @@ extern asmlinkage void dump_stack(void) __cold;
#ifdef CONFIG_PRINTK
#define printk_once(fmt, ...) \
-({ \
- static bool __print_once; \
- \
- if (!__print_once) { \
- __print_once = true; \
- printk(fmt, ##__VA_ARGS__); \
- } \
-})
+ DO_ONCE(printk(fmt, ##__VA_ARGS__));
#else
#define printk_once(fmt, ...) \
no_printk(fmt, ##__VA_ARGS__)
--
1.8.3.1
On Tue, 2013-10-15 at 22:50 +0200, Frederic Weisbecker wrote:
[]
> diff --git a/include/linux/printk.h b/include/linux/printk.h
[]
> @@ -252,14 +253,7 @@ extern asmlinkage void dump_stack(void) __cold;
>
> #ifdef CONFIG_PRINTK
> #define printk_once(fmt, ...) \
> -({ \
> - static bool __print_once; \
> - \
> - if (!__print_once) { \
> - __print_once = true; \
> - printk(fmt, ##__VA_ARGS__); \
> - } \
> -})
> + DO_ONCE(printk(fmt, ##__VA_ARGS__));
It's hard to believe the overhead is worth it.
On Tue, Oct 15, 2013 at 02:00:05PM -0700, Joe Perches wrote:
> On Tue, 2013-10-15 at 22:50 +0200, Frederic Weisbecker wrote:
> []
> > diff --git a/include/linux/printk.h b/include/linux/printk.h
> []
> > @@ -252,14 +253,7 @@ extern asmlinkage void dump_stack(void) __cold;
> >
> > #ifdef CONFIG_PRINTK
> > #define printk_once(fmt, ...) \
> > -({ \
> > - static bool __print_once; \
> > - \
> > - if (!__print_once) { \
> > - __print_once = true; \
> > - printk(fmt, ##__VA_ARGS__); \
> > - } \
> > -})
> > + DO_ONCE(printk(fmt, ##__VA_ARGS__));
>
> It's hard to believe the overhead is worth it.
Which overhead?
On Tue, 2013-10-15 at 23:12 +0200, Frederic Weisbecker wrote:
> On Tue, Oct 15, 2013 at 02:00:05PM -0700, Joe Perches wrote:
> > On Tue, 2013-10-15 at 22:50 +0200, Frederic Weisbecker wrote:
> > []
> > > diff --git a/include/linux/printk.h b/include/linux/printk.h
> > []
> > > @@ -252,14 +253,7 @@ extern asmlinkage void dump_stack(void) __cold;
> > >
> > > #ifdef CONFIG_PRINTK
> > > #define printk_once(fmt, ...) \
> > > -({ \
> > > - static bool __print_once; \
> > > - \
> > > - if (!__print_once) { \
> > > - __print_once = true; \
> > > - printk(fmt, ##__VA_ARGS__); \
> > > - } \
> > > -})
> > > + DO_ONCE(printk(fmt, ##__VA_ARGS__));
> >
> > It's hard to believe the overhead is worth it.
>
> Which overhead?
The one you were proposing with xchg
Apparently the 1/3 series you submitted
didn't use it.
Given that it didn't, does the indirection
to DO_ONCE really help?
btw:
https://lkml.org/lkml/2009/5/21/300
Perhaps Alan's comment still applies:
https://lkml.org/lkml/2009/5/21/305
On Tue, Oct 15, 2013 at 02:24:40PM -0700, Joe Perches wrote:
> On Tue, 2013-10-15 at 23:12 +0200, Frederic Weisbecker wrote:
> > On Tue, Oct 15, 2013 at 02:00:05PM -0700, Joe Perches wrote:
> > > On Tue, 2013-10-15 at 22:50 +0200, Frederic Weisbecker wrote:
> > > []
> > > > diff --git a/include/linux/printk.h b/include/linux/printk.h
> > > []
> > > > @@ -252,14 +253,7 @@ extern asmlinkage void dump_stack(void) __cold;
> > > >
> > > > #ifdef CONFIG_PRINTK
> > > > #define printk_once(fmt, ...) \
> > > > -({ \
> > > > - static bool __print_once; \
> > > > - \
> > > > - if (!__print_once) { \
> > > > - __print_once = true; \
> > > > - printk(fmt, ##__VA_ARGS__); \
> > > > - } \
> > > > -})
> > > > + DO_ONCE(printk(fmt, ##__VA_ARGS__));
> > >
> > > It's hard to believe the overhead is worth it.
> >
> > Which overhead?
>
> The one you were proposing with xchg
>
> Apparently the 1/3 series you submitted
> didn't use it.
>
> Given that it didn't, does the indirection
> to DO_ONCE really help?
Yes, the diffstat has more "-" than "+" :o)
But we might get the xchg() in the end. Andrew proposed
a nice tradeoff against the performance issue:
if (!__warned)
return;
__old_warned = xchg(__warned, 1);
if (!__old_warned)
do_warning
>
> btw:
>
> https://lkml.org/lkml/2009/5/21/300
>
> Perhaps Alan's comment still applies:
>
> https://lkml.org/lkml/2009/5/21/305
Alan was right having such a macro handy is tempting to use in a driver or so,
and that would be most of the time for bad reasons. It's not an object synchronization.
Now I don't entirely agree with him because it consolidates existing code. It's a
good CPP library when something really needs a global state to execute something
only once.
It's especially useful for debugging. I mean I often miss such a macro. I use very
often the following pattern for debugging:
static int done;
if (!done) {
trace_printk(something);
trace_printk(something else);
trace_dump_stack();
done = 1;
}
Having a DO_ONCE() would help a lot I think.
Now we can rename it to __DO_ONCE() and put a big fat comment to avoid it
to be misused.
Hm?
On Wed, 16 Oct 2013 13:53:56 +0200
Frederic Weisbecker <[email protected]> wrote:
> static int done;
>
> if (!done) {
> trace_printk(something);
> trace_printk(something else);
> trace_dump_stack();
> done = 1;
> }
>
> Having a DO_ONCE() would help a lot I think.
>
> Now we can rename it to __DO_ONCE() and put a big fat comment to avoid it
> to be misused.
I wonder if we should make it just ONCE(), with no arguments that
should go into an if statement.
if (ONCE())
do_this_function_once();
Where ONCE() is:
({
static int __once;
int __old_once = __once;
__once = 1;
__old_once;
})
Or the xchg version:
({
static int __once;
if (!__once)
xchg(&__once, 1);
else
1;
})
-- Steve
On Wed, Oct 16, 2013 at 08:59:28AM -0400, Steven Rostedt wrote:
> On Wed, 16 Oct 2013 13:53:56 +0200
> Frederic Weisbecker <[email protected]> wrote:
>
>
> > static int done;
> >
> > if (!done) {
> > trace_printk(something);
> > trace_printk(something else);
> > trace_dump_stack();
> > done = 1;
> > }
> >
> > Having a DO_ONCE() would help a lot I think.
> >
> > Now we can rename it to __DO_ONCE() and put a big fat comment to avoid it
> > to be misused.
>
> I wonder if we should make it just ONCE(), with no arguments that
> should go into an if statement.
>
>
> if (ONCE())
> do_this_function_once();
Or TRUE_ONCE() may be?
But what don't you like in DO_ONCE()? Its upside is that it consolidate the whole
call.
Also there is still the COND() part to handle. Note that COND things
need to return the condition as well.
Thanks.
>
>
> Where ONCE() is:
>
> ({
> static int __once;
> int __old_once = __once;
>
> __once = 1;
> __old_once;
> })
>
> Or the xchg version:
>
> ({
> static int __once;
>
> if (!__once)
> xchg(&__once, 1);
> else
> 1;
> })
>
> -- Steve