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

[PATCH V2 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));

Uses atomic test_and_set_bit

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                     |   26 +++++++++++---------------
 2 files changed, 13 insertions(+), 17 deletions(-)

[PATCH V2 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>) can be more easily written.

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

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 883cd44..179fdac 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -637,6 +637,17 @@ 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 unsigned long __done;		\
+						\
+	if (!test_and_set_bit(0, &__done)) {	\
+		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

[PATCH V2 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 V2 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 179fdac..46234af 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 V2 0/3] Introduce and use DO_ONCE statement expression macro

[Al Viro]

On Thu, May 21, 2009 at 09:27:29PM -0700, Joe Perches wrote:
> 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));

Sigh...  Please, don't do introduce new control structures without extremely
good reasons.

printk_once() has syntax of function call; it's far more tolerable for
casual reader (e.g. somebody looking through the code while trying to
localize a bug) since it doesn't interrupt the flow - it parses as
"some function call, apparently doing some debugging output, let's see
if there are any obvious side effects in the arguments and move on".
Your DO_ONCE(....) parses as "what the fuck is that?" followed by
grepping for definition, and the cost is much higher.

Don't do that.  Staying close to normal syntax is a very good thing;
preprocessor can be used in a lot of ways that are harmful and introduction
of (effectively) new kinds of statements is a prime example.  It *does*
have its place, but it should be done very sparingly.

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

[Joe Perches]

On Fri, 2009-05-22 at 06:26 +0100, Al Viro wrote:
> Your DO_ONCE(....) parses as "what the fuck is that?" followed by
> grepping for definition, and the cost is much higher.

So what do you suggest?

#define pr_info_once(fmt, args...)	printk_once(KERN_INFO pr_fmt(fmt), ##args)
#define pr_warning_once(fmt, args...)	printk_once(KERN_WARNING pr_fmt(fmt), ##args)
etc

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

[Al Viro]

On Thu, May 21, 2009 at 10:39:16PM -0700, Joe Perches wrote:
> On Fri, 2009-05-22 at 06:26 +0100, Al Viro wrote:
> > Your DO_ONCE(....) parses as "what the fuck is that?" followed by
> > grepping for definition, and the cost is much higher.
> 
> So what do you suggest?
> 
> #define pr_info_once(fmt, args...)	printk_once(KERN_INFO pr_fmt(fmt), ##args)
> #define pr_warning_once(fmt, args...)	printk_once(KERN_WARNING pr_fmt(fmt), ##args)
> etc

That would be much saner.

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

[Ingo Molnar]

* Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Thu, May 21, 2009 at 10:39:16PM -0700, Joe Perches wrote:
> > On Fri, 2009-05-22 at 06:26 +0100, Al Viro wrote:
> > > Your DO_ONCE(....) parses as "what the fuck is that?" followed by
> > > grepping for definition, and the cost is much higher.
> > 
> > So what do you suggest?
> > 
> > #define pr_info_once(fmt, args...)	printk_once(KERN_INFO pr_fmt(fmt), ##args)
> > #define pr_warning_once(fmt, args...)	printk_once(KERN_WARNING pr_fmt(fmt), ##args)
> > etc
> 
> That would be much saner.

Agreed.

The *_once() namespace meme is intuitive and easily understood, and 
we use it in a couple of places in the kernel and extend it on an 
as-needed basis for reoccuring (and boring and distracting) 
once-flag C code spam. We apply it to 'boring to begin with' 
constructs: printing a message or a warning, etc. So if a kernel 
coder sees a _once() or _ONCE() construct it can be assumed almost 
straight away that the code there is largely uninteresting from a 
code logic POV.

DO_ONCE() on the other hand is non-intuitive as it is a control 
structure that can be applied to _any_ code construct - interesting 
and uninteresting alike. For anything truly interesting that is not 
a kernel library/facility i dont want it to be hidden and abstracted 
away in 98% of the cases, i want to see the raw C form of it.

Otherwise we might as well write the kernel in C++.

	Ingo