[PATCH 0/3] Introduce and use DO_ONCE statement expression macro

[PATCH 0/3] Introduce and use DO_ONCE statement expression macro

[Joe Perches]

The printk_once macro in kernel.h is limited to printk
This generalizes the functionality of printk_once and
allows statements like DO_ONCE(pr_info("foo\n"))
and DO_ONCE(initialize(foo));

Joe Perches (3):
  kernel.h: Add DO_ONCE statement expression macro
  arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c: Use DO_ONCE & spelling fix
  kernel.h: Remove unused printk_once

 arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c |    4 ++--
 include/linux/kernel.h                     |   27 ++++++++++++---------------
 2 files changed, 14 insertions(+), 17 deletions(-)

[PATCH 1/3] kernel.h: Add DO_ONCE statement expression macro

[Joe Perches]

Add a DO_ONCE statement expression analogous to printk_once
that executes any arbitrary statement exactly once.

This will take the place of printk_once so that
DO_ONCE(pr_<foo>) or any other statement performed
a single time may be easily written.

Signed-off-by: Joe Perches <joe@perches.com>
---
 include/linux/kernel.h |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 883cd44..a5520c9 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -637,6 +637,18 @@ static inline void ftrace_dump(void) { }
 #define swap(a, b) \
 	do { typeof(a) __tmp = (a); (a) = (b); (b) = __tmp; } while (0)
 
+/*
+ * Do something once (analogous to WARN_ONCE() et al):
+ */
+#define DO_ONCE(x...) ({			\
+	static bool __done = false;		\
+						\
+	if (!__done) {				\
+		x;				\
+		__done = true;			\
+	}					\
+})
+
 /**
  * container_of - cast a member of a structure out to the containing structure
  * @ptr:	the pointer to the member.
-- 
1.6.3.1.10.g659a0.dirty

[PATCH 2/3] arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c: Use DO_ONCE & spelling fix

[Joe Perches]

Allows removal of printk_once.

Signed-off-by: Joe Perches <joe@perches.com>
---
 arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
index 208ecf6..f569cff 100644
--- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
+++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
@@ -693,8 +693,8 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	if (perf->control_register.space_id == ACPI_ADR_SPACE_FIXED_HARDWARE &&
 	    policy->cpuinfo.transition_latency > 20 * 1000) {
 		policy->cpuinfo.transition_latency = 20 * 1000;
-			printk_once(KERN_INFO "Capping off P-state tranision"
-				    " latency at 20 uS\n");
+		DO_ONCE(printk(KERN_INFO
+			       "P-state transition latency capped at 20 uS\n"));
 	}
 
 	/* table init */
-- 
1.6.3.1.10.g659a0.dirty

[PATCH 3/3] kernel.h: Remove unused printk_once

[Joe Perches]

Signed-off-by: Joe Perches <joe@perches.com>
---
 include/linux/kernel.h |   15 ---------------
 1 files changed, 0 insertions(+), 15 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index a5520c9..5e37171 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -243,18 +243,6 @@ extern int printk_ratelimit(void);
 extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
 				   unsigned int interval_msec);
 
-/*
- * Print a one-time message (analogous to WARN_ONCE() et al):
- */
-#define printk_once(x...) ({			\
-	static int __print_once = 1;		\
-						\
-	if (__print_once) {			\
-		__print_once = 0;		\
-		printk(x);			\
-	}					\
-})
-
 void log_buf_kexec_setup(void);
 #else
 static inline int vprintk(const char *s, va_list args)
@@ -268,9 +256,6 @@ static inline bool printk_timed_ratelimit(unsigned long *caller_jiffies, \
 					  unsigned int interval_msec)	\
 		{ return false; }
 
-/* No effect, but we still get type checking even in the !PRINTK case: */
-#define printk_once(x...) printk(x)
-
 static inline void log_buf_kexec_setup(void)
 {
 }
-- 
1.6.3.1.10.g659a0.dirty

Re: [PATCH 2/3] arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c: Use DO_ONCE & spelling fix

[Dave Jones]

On Thu, May 21, 2009 at 04:00:23PM -0700, Joe Perches wrote:
 > Allows removal of printk_once.
 > 
 > Signed-off-by: Joe Perches <joe@perches.com>
 > ---
 >  arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c |    4 ++--
 >  1 files changed, 2 insertions(+), 2 deletions(-)
 > 
 > diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
 > index 208ecf6..f569cff 100644
 > --- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
 > +++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
 > @@ -693,8 +693,8 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
 >  	if (perf->control_register.space_id == ACPI_ADR_SPACE_FIXED_HARDWARE &&
 >  	    policy->cpuinfo.transition_latency > 20 * 1000) {
 >  		policy->cpuinfo.transition_latency = 20 * 1000;
 > -			printk_once(KERN_INFO "Capping off P-state tranision"
 > -				    " latency at 20 uS\n");
 > +		DO_ONCE(printk(KERN_INFO
 > +			       "P-state transition latency capped at 20 uS\n"));
 
ewww. This looks pretty ugly to me. Anyone else?

	Dave

Re: [PATCH 1/3] kernel.h: Add DO_ONCE statement expression macro

[Randy Dunlap]

Joe Perches wrote:
> Add a DO_ONCE statement expression analogous to printk_once
> that executes any arbitrary statement exactly once.
> 
> This will take the place of printk_once so that
> DO_ONCE(pr_<foo>) or any other statement performed
> a single time may be easily written.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  include/linux/kernel.h |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 883cd44..a5520c9 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -637,6 +637,18 @@ static inline void ftrace_dump(void) { }
>  #define swap(a, b) \
>  	do { typeof(a) __tmp = (a); (a) = (b); (b) = __tmp; } while (0)
>  
> +/*
> + * Do something once (analogous to WARN_ONCE() et al):
> + */
> +#define DO_ONCE(x...) ({			\
> +	static bool __done = false;		\
> +						\
> +	if (!__done) {				\
> +		x;				\
> +		__done = true;			\

Hi Joe,

Since x is of arbitrary size & time duration, I think that x; should follow
		__done = true;
instead of being before it, as was done in printk_once().  Otherwise there
could be a sizable window there x could be done more than once.


> +	}					\
> +})
> +
>  /**
>   * container_of - cast a member of a structure out to the containing structure
>   * @ptr:	the pointer to the member.


-- 
~Randy
LPC 2009, Sept. 23-25, Portland, Oregon
http://linuxplumbersconf.org/2009/

Re: [PATCH 1/3] kernel.h: Add DO_ONCE statement expression macro

[Joe Perches]

On Thu, 2009-05-21 at 16:27 -0700, Randy Dunlap wrote:
> Joe Perches wrote:
> > Add a DO_ONCE statement expression analogous to printk_once
> > that executes any arbitrary statement exactly once.
> Since x is of arbitrary size & time duration, I think that x; should follow
> 		__done = true;
> instead of being before it, as was done in printk_once().  Otherwise there
> could be a sizable window there x could be done more than once.

Makes sense to me.

Add a DO_ONCE statement expression analogous to printk_once
that executes any arbitrary statement exactly once.

This will take the place of printk_once so that
DO_ONCE(pr_<foo>) or any other statement performed
a single time may be easily written.

Signed-off-by: Joe Perches <joe@perches.com>
---
 include/linux/kernel.h |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 883cd44..a5520c9 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -637,7 +637,18 @@ static inline void ftrace_dump(void) { }
 #define swap(a, b) \
 	do { typeof(a) __tmp = (a); (a) = (b); (b) = __tmp; } while (0)
 
+/*
+ * Do something once (analogous to WARN_ONCE() et al):
+ */
+#define DO_ONCE(x...) ({			\
+	static bool __done = false;		\
+						\
+	if (!__done) {				\
+		__done = true;			\
+		x;				\
+	}					\
+})
+
 /**
  * container_of - cast a member of a structure out to the containing structure
  * @ptr:	the pointer to the member.
-- 
1.6.3.1.10.g659a0.dirty

Re: [PATCH 2/3] arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c: Use DO_ONCE & spelling fix

[Joe Perches]

On Thu, 2009-05-21 at 19:26 -0400, Dave Jones wrote:
> On Thu, May 21, 2009 at 04:00:23PM -0700, Joe Perches wrote:
>  > diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
>  > index 208ecf6..f569cff 100644
>  > --- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
>  > +++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
>  > @@ -693,8 +693,8 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
>  >  	if (perf->control_register.space_id == ACPI_ADR_SPACE_FIXED_HARDWARE &&
>  >  	    policy->cpuinfo.transition_latency > 20 * 1000) {
>  >  		policy->cpuinfo.transition_latency = 20 * 1000;
>  > -			printk_once(KERN_INFO "Capping off P-state tranision"
>  > -				    " latency at 20 uS\n");
>  > +		DO_ONCE(printk(KERN_INFO
>  > +			       "P-state transition latency capped at 20 uS\n"));
>  
> ewww. This looks pretty ugly to me. Anyone else?

What look ugly?

The macro use or the newline between KERN_INFO and "P-"
or the reformatting of the quoted string?

Re: [PATCH 1/3] kernel.h: Add DO_ONCE statement expression macro

[H. Peter Anvin]

Joe Perches wrote:
> 
> Add a DO_ONCE statement expression analogous to printk_once
> that executes any arbitrary statement exactly once.
> 

If you're truly going to make that guarantee you should use an atomic
test and set.

	-hpa

Re: [PATCH 1/3] kernel.h: Add DO_ONCE statement expression macro

[Alan Cox]

> Add a DO_ONCE statement expression analogous to printk_once
> that executes any arbitrary statement exactly once.

Your implementation is racy. Its also hiding stuff in macro magic which
is asking for accidents to happen.

The natural kernel way to write this is

	if (!test_and_set_bit(....))

which is a lot more obvious than a magic macro, doesn't race and can use
other bits in an existing set of flags and be used as a
"do-once-for-this-object-instance" - which is almost always want you want.

Alan

Re: [PATCH 2/3] arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c: Use DO_ONCE & spelling fix

[Dave Jones]

On Thu, May 21, 2009 at 04:34:01PM -0700, Joe Perches wrote:
 > On Thu, 2009-05-21 at 19:26 -0400, Dave Jones wrote:
 > > On Thu, May 21, 2009 at 04:00:23PM -0700, Joe Perches wrote:
 > >  > diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
 > >  > index 208ecf6..f569cff 100644
 > >  > --- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
 > >  > +++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
 > >  > @@ -693,8 +693,8 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
 > >  >  	if (perf->control_register.space_id == ACPI_ADR_SPACE_FIXED_HARDWARE &&
 > >  >  	    policy->cpuinfo.transition_latency > 20 * 1000) {
 > >  >  		policy->cpuinfo.transition_latency = 20 * 1000;
 > >  > -			printk_once(KERN_INFO "Capping off P-state tranision"
 > >  > -				    " latency at 20 uS\n");
 > >  > +		DO_ONCE(printk(KERN_INFO
 > >  > +			       "P-state transition latency capped at 20 uS\n"));
 > >  
 > > ewww. This looks pretty ugly to me. Anyone else?
 > 
 > What look ugly?
 > 
 > The macro use or the newline between KERN_INFO and "P-"
 > or the reformatting of the quoted string?

I just think it's a less readable variant of the same thing.
The shouty macro, the extra level of brackets, the whole thing 
just seems to be ugly to me with no redeeming feature.

What does doing this change really bring us?

	Dave

Re: [PATCH 2/3] arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c: Use DO_ONCE & spelling fix

[Joe Perches]

On Thu, 2009-05-21 at 19:47 -0400, Dave Jones wrote:
> I just think it's a less readable variant of the same thing.
> The shouty macro, the extra level of brackets, the whole thing 
> just seems to be ugly to me with no redeeming feature.
> 
> What does doing this change really bring us?

The ability to use pr_<foo> types rather than printk_once(KERN_<foo>
and have the pr_fmt macro function correctly.

DO_ONCE(pr_info(

vs

printk_once(KERN_INFO

Re: [PATCH 1/3] kernel.h: Add DO_ONCE statement expression macro

[Al Viro]

On Thu, May 21, 2009 at 04:00:22PM -0700, Joe Perches wrote:
> Add a DO_ONCE statement expression analogous to printk_once
> that executes any arbitrary statement exactly once.
> 
> This will take the place of printk_once so that
> DO_ONCE(pr_<foo>) or any other statement performed
> a single time may be easily written.

Interesting, how telling somebody that they need to learn C is considered
an unacceptable thing to do.  Hostile to newbies, or some such.  Introducing
more magic that has to be learnt if one wants to read the kernel source, OTOH,
is just fine...

Re: [PATCH 1/3] kernel.h: Add DO_ONCE statement expression macro

[Joe Perches]

On Fri, 2009-05-22 at 01:27 +0100, Al Viro wrote:
> On Thu, May 21, 2009 at 04:00:22PM -0700, Joe Perches wrote:
> > Add a DO_ONCE statement expression analogous to printk_once
> > that executes any arbitrary statement exactly once.
> > 
> > This will take the place of printk_once so that
> > DO_ONCE(pr_<foo>) or any other statement performed
> > a single time may be easily written.
> 
> Interesting, how telling somebody that they need to learn C is considered
> an unacceptable thing to do.  Hostile to newbies, or some such.  Introducing
> more magic that has to be learnt if one wants to read the kernel source, OTOH,
> is just fine...

I'd be fine if the printk_once macro helper went away.
It was added in commit f036be96dd9ce442ffb9ab33e3c165f5178815c0

If the helper exists though, I think it should work with
all of the pr_<foo> variants.

Re: [PATCH 1/3] kernel.h: Add DO_ONCE statement expression macro

[Rusty Russell]

On Fri, 22 May 2009 08:30:22 am Joe Perches wrote:
> Add a DO_ONCE statement expression analogous to printk_once
> that executes any arbitrary statement exactly once.
>
> This will take the place of printk_once so that
> DO_ONCE(pr_<foo>) or any other statement performed
> a single time may be easily written.

If you're going to use a statement expression, was the intent to make
it usable for a test?  If so, you want something vaguely like:

#define DO_ONCE(x...) ({			\
	static bool __done = false;		\
	bool did = !__done;			\
	if (!__done) {				\
		x;				\
		__done = true;			\
	}					\
	did;					\
})

Otherwise, why not just use a normal do {} while (0) ?

Anyway, statements in macros leave me uncomfortable. I prefer explicit
ONCE() macro which can be tested.

Cheers,
Rusty.

Re: [PATCH 1/3] kernel.h: Add DO_ONCE statement expression macro

[Andrew Morton]

On Thu, 21 May 2009 16:00:22 -0700 Joe Perches <joe@perches.com> wrote:

> +/*
> + * Do something once (analogous to WARN_ONCE() et al):
> + */
> +#define DO_ONCE(x...) ({			\
> +	static bool __done = false;		\
> +						\
> +	if (!__done) {				\
> +		x;				\
> +		__done = true;			\
> +	}					\
> +})

Every single call site for this macro will be mind-bogglingly ugly and
complex callers won't look like C at all.

It would be much better to replace

	DO_ONCE(code-sequence);

with
	if (ONCE()) {
		code-sequence;
	}

I think that's fairly natural and clear and will allow us to clean up a
large number of callsites many of which do the same thing in different
ways, some of them buggily.

And yeah, if this is to be core kernel infrastructure then the default
implementation shouldn't be racy on SMP/preempt.