[RFC PATCH 0/3] headers: Consolidate once/cond style macros

[RFC PATCH 0/3] headers: Consolidate once/cond style macros

[Frederic Weisbecker]

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(-)

[PATCH 1/3] core: New macro to execute code only once

[Frederic Weisbecker]

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 <fweisbec@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Liu Chuansheng <chuansheng.liu@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
---
 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

[PATCH 2/3] core: Convert printk_once to use DO_ONCE

[Frederic Weisbecker]

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Liu Chuansheng <chuansheng.liu@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
---
 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

Re: [PATCH 2/3] core: Convert printk_once to use DO_ONCE

[Joe Perches]

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.

Re: [PATCH 2/3] core: Convert printk_once to use DO_ONCE

[Frederic Weisbecker]

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?

Re: [PATCH 2/3] core: Convert printk_once to use DO_ONCE

[Joe Perches]

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

Re: [PATCH 2/3] core: Convert printk_once to use DO_ONCE

[Frederic Weisbecker]

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?

Re: [PATCH 2/3] core: Convert printk_once to use DO_ONCE

[Steven Rostedt]

On Wed, 16 Oct 2013 13:53:56 +0200
Frederic Weisbecker <fweisbec@gmail.com> 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

Re: [PATCH 2/3] core: Convert printk_once to use DO_ONCE

[Frederic Weisbecker]

On Wed, Oct 16, 2013 at 08:59:28AM -0400, Steven Rostedt wrote:
> On Wed, 16 Oct 2013 13:53:56 +0200
> Frederic Weisbecker <fweisbec@gmail.com> 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

[PATCH 3/3] bug: Convert warn macros to use once and cond helpers

[Frederic Weisbecker]

Unfortunately this removes the use of .data.unlikely code
section for the warned state...

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Liu Chuansheng <chuansheng.liu@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
---
 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