a52b1752c07 broke !SMP: error: implicit declaration of function `WARN_ON'

a52b1752c07 broke !SMP: error: implicit declaration of function `WARN_ON'

[Uwe Kleine-König]

Hello,

kernel/timer.c (and some others as arch/arm/kernel/irq.c) include
<linux/smp.h>, but not <linux/kernel.h>

a52b1752c07 introduces usage of the WARN_ON macro in <linux/smp.h>, but
doesn't pull in <linux/kernel.h>.  (<asm/bug.h> is not enough, at least
for arm, because WARN_ON uses printk there.)

The obvious options are:

1) include <linux/kernel.h> in <linux/smp.h>, maybe conditioned by !SMP
2) include <linux/kernel.h> in all includers of <linux/smp.h>
3) remove the WARN_ONs introduced by a52b1752c07.

WARN_ON is used in an inline function that isn't used in every file
including <linux/smp.h>, so maybe updating the compiler might make
the effort for 2) smaller!?  (I'm using gcc 3.4.4)

Best regards
Uwe

-- 
Uwe Kleine-König

$ dc -e "5735816763073014741799356604682P"

Re: a52b1752c07 broke !SMP: error: implicit declaration of function `WARN_ON'

[Al Viro]

On Wed, Jul 18, 2007 at 01:24:46AM +0200, Uwe Kleine-K?nig wrote:
> Hello,
> 
> kernel/timer.c (and some others as arch/arm/kernel/irq.c) include
> <linux/smp.h>, but not <linux/kernel.h>
> 
> a52b1752c07 introduces usage of the WARN_ON macro in <linux/smp.h>, but
> doesn't pull in <linux/kernel.h>.  (<asm/bug.h> is not enough, at least
> for arm, because WARN_ON uses printk there.)
> 
> The obvious options are:
> 
> 1) include <linux/kernel.h> in <linux/smp.h>, maybe conditioned by !SMP
> 2) include <linux/kernel.h> in all includers of <linux/smp.h>
> 3) remove the WARN_ONs introduced by a52b1752c07.
> 
> WARN_ON is used in an inline function that isn't used in every file
> including <linux/smp.h>, so maybe updating the compiler might make
> the effort for 2) smaller!?  (I'm using gcc 3.4.4)

4) turn the sucker into macro

[PATCH] UP: smp_call_function_single() must warn on irqs_disabled()

[Satyam Sharma]

On Wed, 18 Jul 2007, Al Viro wrote:

> On Wed, Jul 18, 2007 at 01:24:46AM +0200, Uwe Kleine-Konig wrote:
> > [...]
> > a52b1752c07 introduces usage of the WARN_ON macro in <linux/smp.h>, but
> > doesn't pull in <linux/kernel.h>.  (<asm/bug.h> is not enough, at least
> > for arm, because WARN_ON uses printk there.)
> > 
> > The obvious options are:
> > 
> > 1) include <linux/kernel.h> in <linux/smp.h>, maybe conditioned by !SMP
> > 2) include <linux/kernel.h> in all includers of <linux/smp.h>
> > 3) remove the WARN_ONs introduced by a52b1752c07.
> > [...]
> 
> 4) turn the sucker into macro


I think that warning is incorrect anyway ... and I'm to blame
for that. What happened is that I had earlier (month or two back,
_before_ the recent change in smp_call_function_single() semantics)
submitted a patch that put an unconditional WARN_ON(1) in this
function for UP -- in those days, both the smp_call_function*
variants were illegal on the current CPU itself.
( http://lkml.org/lkml/2007/6/7/262 )

Then recently, Andi / Avi proposed the new semantics for
smp_call_function_single() to allow it to just execute the given
function on current CPU as well -- smp_call_function() semantics
were left unchanged, so as not to break stuff like smp_send_stop().

I think I got confused and recommended the (cpuid != 0) warning,
but I don't really think it is necessary with new semantics --
sorry about this, Avi.

OTOH, the function _should_ include a WARN_ON, but for a different
condition -- irqs_disabled(), as explained in changelog below.

I'd be grateful if someone could apply this, Cc:'ing Andi too.

Satyam

---

[PATCH] UP: smp_call_function_single() must warn on irqs_disabled()

Because:
(1) smp_call_function_single() semantics dictate that.
(2) That makes UP behaviour similar to SMP case (implementations
    of smp_call_function_single() in all the archs do this).
(3) We use the unconditional non-save/restore-flags versions of
    local_irq_disable/enable just below this, so it's a bug to
    call this function with IRQs disabled anyway.

Also remove the cpuid != 0 warning that I had erroneously
suggested to Avi earlier (sorry!)

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Andi Kleen <ak@suse.de>
Cc: Avi Kivity <avi@qumranet.com>

---

 include/linux/smp.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/smp.h b/include/linux/smp.h
index 259a13c..016dab5 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -101,7 +101,7 @@ static inline void smp_send_reschedule(int cpu) { }
 #define smp_prepare_boot_cpu()			do {} while (0)
 #define smp_call_function_single(cpuid, func, info, retry, wait) \
 ({ \
-	WARN_ON(cpuid != 0);	\
+	WARN_ON(irqs_disabled()); \
 	local_irq_disable();	\
 	(func)(info);		\
 	local_irq_enable();	\

Re: [PATCH] UP: smp_call_function_single() must warn on irqs_disabled()

[Satyam Sharma]

On Wed, 18 Jul 2007, Satyam Sharma wrote:

> On Wed, 18 Jul 2007, Al Viro wrote:
> 
> > On Wed, Jul 18, 2007 at 01:24:46AM +0200, Uwe Kleine-Konig wrote:
> > > [...]
> > > a52b1752c07 introduces usage of the WARN_ON macro in <linux/smp.h>, but
> > > doesn't pull in <linux/kernel.h>.  (<asm/bug.h> is not enough, at least
> > > for arm, because WARN_ON uses printk there.)
> > > 
> > > The obvious options are:
> > > 
> > > 1) include <linux/kernel.h> in <linux/smp.h>, maybe conditioned by !SMP
> > > 2) include <linux/kernel.h> in all includers of <linux/smp.h>
> > > 3) remove the WARN_ONs introduced by a52b1752c07.
> > > [...]
> > 
> > 4) turn the sucker into macro
> 
> 
> I think that warning is incorrect anyway ... and I'm to blame
> for that. What happened is that I had earlier (month or two back,
> _before_ the recent change in smp_call_function_single() semantics)
> submitted a patch that put an unconditional WARN_ON(1) in this
> function for UP -- in those days, both the smp_call_function*
> variants were illegal on the current CPU itself.
> ( http://lkml.org/lkml/2007/6/7/262 )
> 
> Then recently, Andi / Avi proposed the new semantics for
> smp_call_function_single() to allow it to just execute the given
> function on current CPU as well -- smp_call_function() semantics
> were left unchanged, so as not to break stuff like smp_send_stop().
> 
> I think I got confused and recommended the (cpuid != 0) warning,
> but I don't really think it is necessary with new semantics --
> sorry about this, Avi.
> 
> OTOH, the function _should_ include a WARN_ON, but for a different
> condition -- irqs_disabled(), as explained in changelog below.

But on third thoughts, we shouldn't be executing that function locally
for UP if cpuid != 0 anyway, so my earlier suggestion wasn't really
erroneous ... perhaps *both* those warnings are required (well, the
irqs_disabled() one definitely is, at least).