PREEMPT_ACTIVE too low error with all asm-generic headers for some arches

PREEMPT_ACTIVE too low error with all asm-generic headers for some arches

[Mike Frysinger]

after pulling the latest mainline code, Blackfin started hitting a
build failure like so:
  CC      arch/blackfin/kernel/asm-offsets.s
In file included from include/linux/interrupt.h:12,
                 from include/linux/kernel_stat.h:8,
                 from arch/blackfin/kernel/asm-offsets.c:32:
include/linux/hardirq.h:66:2: error: #error PREEMPT_ACTIVE is too low!
make[1]: *** [arch/blackfin/kernel/asm-offsets.s] Error 1

this is because we've converted to asm-generic for most of our headers
(including hardirq.h).  originally we were defining HARDIRQ_BITS
ourselves to 8, but then we dropped that in favor of the
asm-generic/hardirq.h which setup a default of 8.  but then they
dropped it in favor of the linux/hardirq.h default handling ... but it
sets it to MAX_HARDIRQ_BITS by default which is 10.  which pushes
Blackfin over the edge and into this build error.

if we look at linux/hardirq.h, it makes this claim:
 * - bit 28 is the PREEMPT_ACTIVE flag
if that's true, then why are we letting any arch set this define ?  a
quick survey shows that half the arches (11) are using 0x10000000 (bit
28) while the other half (10) are using 0x4000000 (bit 26).  and then
there is the ia64 oddity which uses bit 30.  the exact value here
shouldnt really matter across arches though should it ?

how about adding this to linux/thread_info.h:
#ifndef PREEMPT_ACTIVE
# ifndef PREEMPT_ACTIVE_BIT
#  define PREEMPT_ACTIVE_BIT 28
# endif
# define PREEMPT_ACTIVE (1 << PREEMPT_ACTIVE_BIT)
#endif
-mike

Re: PREEMPT_ACTIVE too low error with all asm-generic headers for some arches

[Ingo Molnar]

* Mike Frysinger <vapier.adi@gmail.com> wrote:

> after pulling the latest mainline code, Blackfin started hitting a
> build failure like so:
>   CC      arch/blackfin/kernel/asm-offsets.s
> In file included from include/linux/interrupt.h:12,
>                  from include/linux/kernel_stat.h:8,
>                  from arch/blackfin/kernel/asm-offsets.c:32:
> include/linux/hardirq.h:66:2: error: #error PREEMPT_ACTIVE is too low!
> make[1]: *** [arch/blackfin/kernel/asm-offsets.s] Error 1
> 
> this is because we've converted to asm-generic for most of our headers
> (including hardirq.h).  originally we were defining HARDIRQ_BITS
> ourselves to 8, but then we dropped that in favor of the
> asm-generic/hardirq.h which setup a default of 8.  but then they
> dropped it in favor of the linux/hardirq.h default handling ... but it
> sets it to MAX_HARDIRQ_BITS by default which is 10.  which pushes
> Blackfin over the edge and into this build error.

hm, you wrote this mail to me but i havent touched asm-generic nor 
blackfin in this cycle. The breakage appears to have been caused by 
or at around this commit:

>From 804387a1af87f66a4b93eee230ba98f8b906b088 Mon Sep 17 00:00:00 2001
From: Arnd Bergmann <arnd@arndb.de>
Date: Sun, 14 Jun 2009 22:38:11 +0200
Subject: [PATCH] asm-generic: drop HARDIRQ_BITS definition from hardirq.h

[...]

Reported-by: Mike Frysinger <vapier@gentoo.org>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

So i've Cc:-ed those folks too.

> if we look at linux/hardirq.h, it makes this claim:
>  * - bit 28 is the PREEMPT_ACTIVE flag
> if that's true, then why are we letting any arch set this define ?  a
> quick survey shows that half the arches (11) are using 0x10000000 (bit
> 28) while the other half (10) are using 0x4000000 (bit 26).  and then
> there is the ia64 oddity which uses bit 30.  the exact value here
> shouldnt really matter across arches though should it ?

Correct - what matters is to have no collision between the fields.

> how about adding this to linux/thread_info.h:
> #ifndef PREEMPT_ACTIVE
> # ifndef PREEMPT_ACTIVE_BIT
> #  define PREEMPT_ACTIVE_BIT 28
> # endif
> # define PREEMPT_ACTIVE (1 << PREEMPT_ACTIVE_BIT)
> #endif

Makes sense i guess - but do we really need that level of
#ifdef nesting? PREEMPT_ACTIVE_BIT should be the main control - with
a default to 28 if it's not set. PREEMPT_ACTIVE is then derived off 
that, without any #ifdefs.

Anyway ... no objections from me in this area (and your build is 
broken so i suspect you want a fix quickly), just please make the 
override clean. Btw., why cannot blackfin use the defaults?

	Ingo

Re: PREEMPT_ACTIVE too low error with all asm-generic headers for some arches

[Mike Frysinger]

On Wed, Jun 24, 2009 at 09:13, Ingo Molnar wrote:
> * Mike Frysinger <vapier.adi@gmail.com> wrote:
>> after pulling the latest mainline code, Blackfin started hitting a
>> build failure like so:
>>   CC      arch/blackfin/kernel/asm-offsets.s
>> In file included from include/linux/interrupt.h:12,
>>                  from include/linux/kernel_stat.h:8,
>>                  from arch/blackfin/kernel/asm-offsets.c:32:
>> include/linux/hardirq.h:66:2: error: #error PREEMPT_ACTIVE is too low!
>> make[1]: *** [arch/blackfin/kernel/asm-offsets.s] Error 1
>>
>> this is because we've converted to asm-generic for most of our headers
>> (including hardirq.h).  originally we were defining HARDIRQ_BITS
>> ourselves to 8, but then we dropped that in favor of the
>> asm-generic/hardirq.h which setup a default of 8.  but then they
>> dropped it in favor of the linux/hardirq.h default handling ... but it
>> sets it to MAX_HARDIRQ_BITS by default which is 10.  which pushes
>> Blackfin over the edge and into this build error.
>
> hm, you wrote this mail to me but i havent touched asm-generic nor
> blackfin in this cycle.

i didnt say you did.  you seemed to be the guy who would know about
sane values in hardirq/preempt, i was merely giving background on what
lead me here -- those changes arent wrong in any way.

>> if we look at linux/hardirq.h, it makes this claim:
>>  * - bit 28 is the PREEMPT_ACTIVE flag
>> if that's true, then why are we letting any arch set this define ?  a
>> quick survey shows that half the arches (11) are using 0x10000000 (bit
>> 28) while the other half (10) are using 0x4000000 (bit 26).  and then
>> there is the ia64 oddity which uses bit 30.  the exact value here
>> shouldnt really matter across arches though should it ?
>
> Correct - what matters is to have no collision between the fields.
>
>> how about adding this to linux/thread_info.h:
>> #ifndef PREEMPT_ACTIVE
>> # ifndef PREEMPT_ACTIVE_BIT
>> #  define PREEMPT_ACTIVE_BIT 28
>> # endif
>> # define PREEMPT_ACTIVE (1 << PREEMPT_ACTIVE_BIT)
>> #endif
>
> Makes sense i guess - but do we really need that level of
> #ifdef nesting? PREEMPT_ACTIVE_BIT should be the main control - with
> a default to 28 if it's not set. PREEMPT_ACTIVE is then derived off
> that, without any #ifdefs.

well, i didnt want to write it like so:
#ifndef PREEMPT_ACTIVE_BIT
# define PREEMPT_ACTIVE_BIT 28
#endif
#ifndef PREEMPT_ACTIVE
# define PREEMPT_ACTIVE (1 << PREEMPT_ACTIVE_BIT)
#endif

because if the arch has defined PREEMPT_ACTIVE but not
PREEMPT_ACTIVE_BIT, then things could go bad.  since the only consumer
of PREEMPT_ACTIVE_BIT that i can see is one ia64 assembly file, we can
just avoid the indirection.  i wanted to make it clear that this is
indeed defaulting to bit 28 like the comments in hardirq.h say.  i
also wanted to avoid having to change any arch files other than my own
(i.e. allow people to be grandfathered in).

i guess we can reformat it as:
#ifndef PREEMPT_ACTIVE_BIT
# define PREEMPT_ACTIVE_BIT 28
#endif
#define PREEMPT_ACTIVE (1 << PREEMPT_ACTIVE_BIT)
which makes me do the footwork of converting everyone over to PREEMPT_ACTIVE_BIT

> Anyway ... no objections from me in this area (and your build is
> broken so i suspect you want a fix quickly), just please make the
> override clean. Btw., why cannot blackfin use the defaults?

Blackfin is using the defaults.  the issue i pointed out is that the
defaults set up 10 hardirq bits which ultimately conflict with any
arch (and there are 10 of them) that is using bit 26 for
PREEMPT_ACTIVE.  there is no default value for PREEMPT_ACTIVE (yet).
-mike

Re: PREEMPT_ACTIVE too low error with all asm-generic headers for some arches

[Arnd Bergmann]

On Wednesday 24 June 2009, Ingo Molnar wrote:
> * Mike Frysinger <vapier.adi@gmail.com> wrote:
> 
> > if we look at linux/hardirq.h, it makes this claim:
> >  * - bit 28 is the PREEMPT_ACTIVE flag
> > if that's true, then why are we letting any arch set this define ?  a
> > quick survey shows that half the arches (11) are using 0x10000000 (bit
> > 28) while the other half (10) are using 0x4000000 (bit 26).  and then
> > there is the ia64 oddity which uses bit 30.  the exact value here
> > shouldnt really matter across arches though should it ?

actually alpha, arm and avr32 also use bit 30 (0x40000000), there are only
five (or eight, depending on how you count) architectures (blackfin, h8300,
m68k, s390 and sparc) using bit 26.

> Correct - what matters is to have no collision between the fields.
> 
> > how about adding this to linux/thread_info.h:
> > #ifndef PREEMPT_ACTIVE
> > # ifndef PREEMPT_ACTIVE_BIT
> > #  define PREEMPT_ACTIVE_BIT 28
> > # endif
> > # define PREEMPT_ACTIVE (1 << PREEMPT_ACTIVE_BIT)
> > #endif
> 
> Makes sense i guess - but do we really need that level of
> #ifdef nesting? PREEMPT_ACTIVE_BIT should be the main control - with
> a default to 28 if it's not set. PREEMPT_ACTIVE is then derived off 
> that, without any #ifdefs.

I think it would fit better into linux/hardirq.h instead of
linux/thread_info.h, because that is where the other bits of
the preempt count are defined.

How would this one work out?

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

--- a/include/linux/hardirq.h
+++ b/include/linux/hardirq.h
@@ -62,6 +62,12 @@
 #define HARDIRQ_OFFSET	(1UL << HARDIRQ_SHIFT)
 #define NMI_OFFSET	(1UL << NMI_SHIFT)
 
+#ifndef PREEMPT_ACTIVE
+#define PREEMPT_ACTIVE_BITS	1
+#define PREEMPT_ACTIVE_SHIFT	(NMI_SHIFT + NMI_BITS)
+#define PREEMPT_ACTIVE	(__IRQ_MASK(PREEMPT_ACTIVE_BITS) << PREEMPT_SHIFT)
+#endif
+
 #if PREEMPT_ACTIVE < (1 << (NMI_SHIFT + NMI_BITS))
 #error PREEMPT_ACTIVE is too low!
 #endif

Re: PREEMPT_ACTIVE too low error with all asm-generic headers for some arches

[Mike Frysinger]

On Wed, Jun 24, 2009 at 10:02, Arnd Bergmann wrote:
> On Wednesday 24 June 2009, Ingo Molnar wrote:
>> * Mike Frysinger <vapier.adi@gmail.com> wrote:
>> > if we look at linux/hardirq.h, it makes this claim:
>> >  * - bit 28 is the PREEMPT_ACTIVE flag
>> > if that's true, then why are we letting any arch set this define ?  a
>> > quick survey shows that half the arches (11) are using 0x10000000 (bit
>> > 28) while the other half (10) are using 0x4000000 (bit 26).  and then
>> > there is the ia64 oddity which uses bit 30.  the exact value here
>> > shouldnt really matter across arches though should it ?
>
> actually alpha, arm and avr32 also use bit 30 (0x40000000), there are only
> five (or eight, depending on how you count) architectures (blackfin, h8300,
> m68k, s390 and sparc) using bit 26.

meh, too many zeros ;)

>> Correct - what matters is to have no collision between the fields.
>>
>> > how about adding this to linux/thread_info.h:
>> > #ifndef PREEMPT_ACTIVE
>> > # ifndef PREEMPT_ACTIVE_BIT
>> > #  define PREEMPT_ACTIVE_BIT 28
>> > # endif
>> > # define PREEMPT_ACTIVE (1 << PREEMPT_ACTIVE_BIT)
>> > #endif
>>
>> Makes sense i guess - but do we really need that level of
>> #ifdef nesting? PREEMPT_ACTIVE_BIT should be the main control - with
>> a default to 28 if it's not set. PREEMPT_ACTIVE is then derived off
>> that, without any #ifdefs.
>
> I think it would fit better into linux/hardirq.h instead of
> linux/thread_info.h, because that is where the other bits of
> the preempt count are defined.

agreed

> --- a/include/linux/hardirq.h
> +++ b/include/linux/hardirq.h
> @@ -62,6 +62,12 @@
>  #define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
>  #define NMI_OFFSET     (1UL << NMI_SHIFT)
>
> +#ifndef PREEMPT_ACTIVE
> +#define PREEMPT_ACTIVE_BITS    1
> +#define PREEMPT_ACTIVE_SHIFT   (NMI_SHIFT + NMI_BITS)
> +#define PREEMPT_ACTIVE (__IRQ_MASK(PREEMPT_ACTIVE_BITS) << PREEMPT_SHIFT)

i think you meant "<< PREEMPT_ACTIVE_SHIFT" there.  once i make that
change, it builds fine.
-mike

Re: PREEMPT_ACTIVE too low error with all asm-generic headers for some arches

[Ingo Molnar]

* Mike Frysinger <vapier.adi@gmail.com> wrote:

> On Wed, Jun 24, 2009 at 10:02, Arnd Bergmann wrote:
> > On Wednesday 24 June 2009, Ingo Molnar wrote:
> >> * Mike Frysinger <vapier.adi@gmail.com> wrote:
> >> > if we look at linux/hardirq.h, it makes this claim:
> >> >  * - bit 28 is the PREEMPT_ACTIVE flag
> >> > if that's true, then why are we letting any arch set this define ?  a
> >> > quick survey shows that half the arches (11) are using 0x10000000 (bit
> >> > 28) while the other half (10) are using 0x4000000 (bit 26).  and then
> >> > there is the ia64 oddity which uses bit 30.  the exact value here
> >> > shouldnt really matter across arches though should it ?
> >
> > actually alpha, arm and avr32 also use bit 30 (0x40000000), there are only
> > five (or eight, depending on how you count) architectures (blackfin, h8300,
> > m68k, s390 and sparc) using bit 26.
> 
> meh, too many zeros ;)
> 
> >> Correct - what matters is to have no collision between the fields.
> >>
> >> > how about adding this to linux/thread_info.h:
> >> > #ifndef PREEMPT_ACTIVE
> >> > # ifndef PREEMPT_ACTIVE_BIT
> >> > #  define PREEMPT_ACTIVE_BIT 28
> >> > # endif
> >> > # define PREEMPT_ACTIVE (1 << PREEMPT_ACTIVE_BIT)
> >> > #endif
> >>
> >> Makes sense i guess - but do we really need that level of
> >> #ifdef nesting? PREEMPT_ACTIVE_BIT should be the main control - with
> >> a default to 28 if it's not set. PREEMPT_ACTIVE is then derived off
> >> that, without any #ifdefs.
> >
> > I think it would fit better into linux/hardirq.h instead of
> > linux/thread_info.h, because that is where the other bits of
> > the preempt count are defined.
> 
> agreed
> 
> > --- a/include/linux/hardirq.h
> > +++ b/include/linux/hardirq.h
> > @@ -62,6 +62,12 @@
> >  #define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
> >  #define NMI_OFFSET     (1UL << NMI_SHIFT)
> >
> > +#ifndef PREEMPT_ACTIVE
> > +#define PREEMPT_ACTIVE_BITS    1
> > +#define PREEMPT_ACTIVE_SHIFT   (NMI_SHIFT + NMI_BITS)
> > +#define PREEMPT_ACTIVE (__IRQ_MASK(PREEMPT_ACTIVE_BITS) << PREEMPT_SHIFT)
> 
> i think you meant "<< PREEMPT_ACTIVE_SHIFT" there.  once i make 
> that change, it builds fine.

With that fix:

 Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

Re: PREEMPT_ACTIVE too low error with all asm-generic headers for some arches

[Mike Frysinger]

On Wed, Jun 24, 2009 at 11:02, Mike Frysinger wrote:
> On Wed, Jun 24, 2009 at 10:02, Arnd Bergmann wrote:
>> --- a/include/linux/hardirq.h
>> +++ b/include/linux/hardirq.h
>> @@ -62,6 +62,12 @@
>>  #define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
>>  #define NMI_OFFSET     (1UL << NMI_SHIFT)
>>
>> +#ifndef PREEMPT_ACTIVE
>> +#define PREEMPT_ACTIVE_BITS    1
>> +#define PREEMPT_ACTIVE_SHIFT   (NMI_SHIFT + NMI_BITS)
>> +#define PREEMPT_ACTIVE (__IRQ_MASK(PREEMPT_ACTIVE_BITS) << PREEMPT_SHIFT)
>
> i think you meant "<< PREEMPT_ACTIVE_SHIFT" there.  once i make that
> change, it builds fine.

and like Ingo, with that fix, add my Acked-by

thanks !
-mike

Re: PREEMPT_ACTIVE too low error with all asm-generic headers for some arches

[Mike Frysinger]

On Wed, Jun 24, 2009 at 18:21, Mike Frysinger wrote:
> On Wed, Jun 24, 2009 at 11:02, Mike Frysinger wrote:
>> On Wed, Jun 24, 2009 at 10:02, Arnd Bergmann wrote:
>>> --- a/include/linux/hardirq.h
>>> +++ b/include/linux/hardirq.h
>>> @@ -62,6 +62,12 @@
>>>  #define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
>>>  #define NMI_OFFSET     (1UL << NMI_SHIFT)
>>>
>>> +#ifndef PREEMPT_ACTIVE
>>> +#define PREEMPT_ACTIVE_BITS    1
>>> +#define PREEMPT_ACTIVE_SHIFT   (NMI_SHIFT + NMI_BITS)
>>> +#define PREEMPT_ACTIVE (__IRQ_MASK(PREEMPT_ACTIVE_BITS) << PREEMPT_SHIFT)
>>
>> i think you meant "<< PREEMPT_ACTIVE_SHIFT" there.  once i make that
>> change, it builds fine.
>
> and like Ingo, with that fix, add my Acked-by
>
> thanks !

could you push this to Linus ?  or should i throw something into the
Blackfin headers in the meantime ?
-mike

Re: PREEMPT_ACTIVE too low error with all asm-generic headers for some arches

[Robin Getz]

On Sat 4 Jul 2009 18:43, Mike Frysinger pondered:
> On Wed, Jun 24, 2009 at 18:21, Mike Frysinger wrote:
> > On Wed, Jun 24, 2009 at 11:02, Mike Frysinger wrote:
> >> On Wed, Jun 24, 2009 at 10:02, Arnd Bergmann wrote:
> >>> --- a/include/linux/hardirq.h
> >>> +++ b/include/linux/hardirq.h
> >>> @@ -62,6 +62,12 @@
> >>>  #define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
> >>>  #define NMI_OFFSET     (1UL << NMI_SHIFT)
> >>>
> >>> +#ifndef PREEMPT_ACTIVE
> >>> +#define PREEMPT_ACTIVE_BITS    1
> >>> +#define PREEMPT_ACTIVE_SHIFT   (NMI_SHIFT + NMI_BITS)
> >>> +#define PREEMPT_ACTIVE (__IRQ_MASK(PREEMPT_ACTIVE_BITS) << PREEMPT_SHIFT)
> >>
> >> i think you meant "<< PREEMPT_ACTIVE_SHIFT" there.  once i make that
> >> change, it builds fine.
> >
> > and like Ingo, with that fix, add my Acked-by
> >
> > thanks !
> 
> could you push this to Linus ?  or should i throw something into the
> Blackfin headers in the meantime ?

Yeah, this is causing build failures on Linux 2.6.31-rc2 for Blackfin.

In file included from include/linux/interrupt.h:12,
                 from include/linux/kernel_stat.h:8,
                 from arch/blackfin/kernel/asm-offsets.c:32:
include/linux/hardirq.h:66:2: error: #error PREEMPT_ACTIVE is too low!
make[1]: *** [arch/blackfin/kernel/asm-offsets.s] Error 1
make: *** [prepare0] Error 2

Which the above patch makes go away...

[PATCH] PREEMPT_ACTIVE: add default defines

[Mike Frysinger]

From: Arnd Bergmann <arnd@arndb.de>

The PREEMPT_ACTIVE setting doesn't actually need to be arch-specific, so
set up a sane default for all arches to (hopefully) migrate to.

> if we look at linux/hardirq.h, it makes this claim:
>  * - bit 28 is the PREEMPT_ACTIVE flag
> if that's true, then why are we letting any arch set this define ?  a
> quick survey shows that half the arches (11) are using 0x10000000 (bit
> 28) while the other half (10) are using 0x4000000 (bit 26).  and then
> there is the ia64 oddity which uses bit 30.  the exact value here
> shouldnt really matter across arches though should it ?

actually alpha, arm and avr32 also use bit 30 (0x40000000), there are only
five (or eight, depending on how you count) architectures (blackfin, h8300,
m68k, s390 and sparc) using bit 26.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
Acked-by: Ingo Molnar <mingo@elte.hu>
---
 include/linux/hardirq.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
index 4525747..41cfbe1 100644
--- a/include/linux/hardirq.h
+++ b/include/linux/hardirq.h
@@ -62,6 +62,12 @@
 #define HARDIRQ_OFFSET	(1UL << HARDIRQ_SHIFT)
 #define NMI_OFFSET	(1UL << NMI_SHIFT)
 
+#ifndef PREEMPT_ACTIVE
+#define PREEMPT_ACTIVE_BITS	1
+#define PREEMPT_ACTIVE_SHIFT	(NMI_SHIFT + NMI_BITS)
+#define PREEMPT_ACTIVE	(__IRQ_MASK(PREEMPT_ACTIVE_BITS) << PREEMPT_ACTIVE_SHIFT)
+#endif
+
 #if PREEMPT_ACTIVE < (1 << (NMI_SHIFT + NMI_BITS))
 #error PREEMPT_ACTIVE is too low!
 #endif
-- 
1.6.3.3