d451564 breakage

d451564 breakage

[Russell King]

Change:

highmem: Fix debug_kmap_atomic() to also handle KM_IRQ_PTE, KM_NMI, and KM_NMI_PTE

Appears to break ARM:

mm/highmem.c: In function ‘debug_kmap_atomic’:
mm/highmem.c:436: error: ‘KM_NMI’ undeclared (first use in this function)
mm/highmem.c:436: error: (Each undeclared identifier is reported only once
mm/highmem.c:436: error: for each function it appears in.)
mm/highmem.c:436: error: ‘KM_NMI_PTE’ undeclared (first use in this function)
mm/highmem.c:443: error: ‘KM_IRQ_PTE’ undeclared (first use in this function)
make[2]: *** [mm/highmem.o] Error 1
make[1]: *** [mm] Error 2

I'd prefer not to have to add these definitions just so that highmem
debugging can work for two reasons:

1. either we allocate mappings for these which will never be used, which
   unnecessarily wastes precious virtual memory space.

2. we define them to be some other KM_* value and hope that they never
   get used.  If they do get used, we'll never know by way of compiler
   error but could result in silent data corruption.

The only sane alternative I can see would be to define these as KM_TYPE_NR
and either ensure that kmap_atomic() always fails for out-of-bounds kmap
types (more preferable to catch problems but has a performance impact) or
we have the kmap debugging detect this as well.

Any preferences?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: d451564 breakage

[Andrew Morton]

On Fri, 13 Nov 2009 15:11:19 +0000
Russell King <rmk@arm.linux.org.uk> wrote:

> Change:
> 
> highmem: Fix debug_kmap_atomic() to also handle KM_IRQ_PTE, KM_NMI, and KM_NMI_PTE
> 
> Appears to break ARM:
> 
> mm/highmem.c: In function ___debug_kmap_atomic___:
> mm/highmem.c:436: error: ___KM_NMI___ undeclared (first use in this function)
> mm/highmem.c:436: error: (Each undeclared identifier is reported only once
> mm/highmem.c:436: error: for each function it appears in.)
> mm/highmem.c:436: error: ___KM_NMI_PTE___ undeclared (first use in this function)
> mm/highmem.c:443: error: ___KM_IRQ_PTE___ undeclared (first use in this function)
> make[2]: *** [mm/highmem.o] Error 1
> make[1]: *** [mm] Error 2
> 
> I'd prefer not to have to add these definitions just so that highmem
> debugging can work for two reasons:
> 
> 1. either we allocate mappings for these which will never be used, which
>    unnecessarily wastes precious virtual memory space.
> 
> 2. we define them to be some other KM_* value and hope that they never
>    get used.  If they do get used, we'll never know by way of compiler
>    error but could result in silent data corruption.
> 
> The only sane alternative I can see would be to define these as KM_TYPE_NR
> and either ensure that kmap_atomic() always fails for out-of-bounds kmap
> types (more preferable to catch problems but has a performance impact) or
> we have the kmap debugging detect this as well.
> 
> Any preferences?

Could we do something nasty with ifdefs?

In arch header:
#define KM_NMI KM_NMI


In generic code:
#ifdef KM_NMI
	<stuff which references KM_NMI>
#endif

or similar?

Re: d451564 breakage

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Fri, 13 Nov 2009 15:11:19 +0000
> Russell King <rmk@arm.linux.org.uk> wrote:
> 
> > Change:
> > 
> > highmem: Fix debug_kmap_atomic() to also handle KM_IRQ_PTE, KM_NMI, and KM_NMI_PTE
> > 
> > Appears to break ARM:
> > 
> > mm/highmem.c: In function ___debug_kmap_atomic___:
> > mm/highmem.c:436: error: ___KM_NMI___ undeclared (first use in this function)
> > mm/highmem.c:436: error: (Each undeclared identifier is reported only once
> > mm/highmem.c:436: error: for each function it appears in.)
> > mm/highmem.c:436: error: ___KM_NMI_PTE___ undeclared (first use in this function)
> > mm/highmem.c:443: error: ___KM_IRQ_PTE___ undeclared (first use in this function)
> > make[2]: *** [mm/highmem.o] Error 1
> > make[1]: *** [mm] Error 2
> > 
> > I'd prefer not to have to add these definitions just so that highmem
> > debugging can work for two reasons:
> > 
> > 1. either we allocate mappings for these which will never be used, which
> >    unnecessarily wastes precious virtual memory space.
> > 
> > 2. we define them to be some other KM_* value and hope that they never
> >    get used.  If they do get used, we'll never know by way of compiler
> >    error but could result in silent data corruption.
> > 
> > The only sane alternative I can see would be to define these as KM_TYPE_NR
> > and either ensure that kmap_atomic() always fails for out-of-bounds kmap
> > types (more preferable to catch problems but has a performance impact) or
> > we have the kmap debugging detect this as well.
> > 
> > Any preferences?
> 
> Could we do something nasty with ifdefs?
> 
> In arch header:
> #define KM_NMI KM_NMI
> 
> 
> In generic code:
> #ifdef KM_NMI
> 	<stuff which references KM_NMI>
> #endif
> 
> or similar?

Yeah, that would be the simplest - and all of this will go away with 
Peter's stacked kmaps.

	Ingo

Re: d451564 breakage

[Ingo Molnar]

* Russell King <rmk@arm.linux.org.uk> wrote:

> Change:
> 
> highmem: Fix debug_kmap_atomic() to also handle KM_IRQ_PTE, KM_NMI, and KM_NMI_PTE
> 
> Appears to break ARM:
> 
> mm/highmem.c: In function ???debug_kmap_atomic???:
> mm/highmem.c:436: error: ???KM_NMI??? undeclared (first use in this function)

indeed - sorry.

Note that debug_kmap_atomic() will be removed in v2.6.33 so i'd suggest 
to just do the easy solution and add #ifndef dummy definitions to 
mm/highmem.c to cover ARM - we'll remove the whole cruft for good.

Btw., testing sidenote: i test the ARM defconfig and it didnt break 
there. Perhaps highmem is off in the ARM defconfig? It would be helpful 
if the ARM defconfig enabled highmem.

Thanks,

	Ingo

Re: d451564 breakage

[Russell King]

On Fri, Nov 13, 2009 at 11:48:20PM +0100, Ingo Molnar wrote:
> 
> * Russell King <rmk@arm.linux.org.uk> wrote:
> 
> > Change:
> > 
> > highmem: Fix debug_kmap_atomic() to also handle KM_IRQ_PTE, KM_NMI, and KM_NMI_PTE
> > 
> > Appears to break ARM:
> > 
> > mm/highmem.c: In function ???debug_kmap_atomic???:
> > mm/highmem.c:436: error: ???KM_NMI??? undeclared (first use in this function)
> 
> indeed - sorry.
> 
> Note that debug_kmap_atomic() will be removed in v2.6.33 so i'd suggest 
> to just do the easy solution and add #ifndef dummy definitions to 
> mm/highmem.c to cover ARM - we'll remove the whole cruft for good.

Actually, the following patch is probably the simplest solution.

> Btw., testing sidenote: i test the ARM defconfig and it didnt break 
> there. Perhaps highmem is off in the ARM defconfig? It would be helpful 
> if the ARM defconfig enabled highmem.

Given that highmem on ARM is experimental, I'd rather not have it enabled
in too many machine defconfigs as standard just yet.

However, enabling highmem on itself is not enough to show this breakage,
you also need highmem debugging enabled.

 arch/arm/include/asm/kmap_types.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/kmap_types.h b/arch/arm/include/asm/kmap_types.h
index d16ec97..c019949 100644
--- a/arch/arm/include/asm/kmap_types.h
+++ b/arch/arm/include/asm/kmap_types.h
@@ -22,4 +22,10 @@ enum km_type {
 	KM_TYPE_NR
 };
 
+#ifdef CONFIG_DEBUG_HIGHMEM
+#define KM_NMI		(-1)
+#define KM_NMI_PTE	(-1)
+#define KM_IRQ_PTE	(-1)
+#endif
+
 #endif


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: d451564 breakage

[Ingo Molnar]

* Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> On Fri, Nov 13, 2009 at 11:48:20PM +0100, Ingo Molnar wrote:
> > 
> > * Russell King <rmk@arm.linux.org.uk> wrote:
> > 
> > > Change:
> > > 
> > > highmem: Fix debug_kmap_atomic() to also handle KM_IRQ_PTE, KM_NMI, and KM_NMI_PTE
> > > 
> > > Appears to break ARM:
> > > 
> > > mm/highmem.c: In function ???debug_kmap_atomic???:
> > > mm/highmem.c:436: error: ???KM_NMI??? undeclared (first use in this function)
> > 
> > indeed - sorry.
> > 
> > Note that debug_kmap_atomic() will be removed in v2.6.33 so i'd suggest 
> > to just do the easy solution and add #ifndef dummy definitions to 
> > mm/highmem.c to cover ARM - we'll remove the whole cruft for good.
> 
> Actually, the following patch is probably the simplest solution.
> 
> > Btw., testing sidenote: i test the ARM defconfig and it didnt break 
> > there. Perhaps highmem is off in the ARM defconfig? It would be helpful 
> > if the ARM defconfig enabled highmem.
> 
> Given that highmem on ARM is experimental, I'd rather not have it enabled
> in too many machine defconfigs as standard just yet.
> 
> However, enabling highmem on itself is not enough to show this breakage,
> you also need highmem debugging enabled.
> 
>  arch/arm/include/asm/kmap_types.h |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kmap_types.h b/arch/arm/include/asm/kmap_types.h
> index d16ec97..c019949 100644
> --- a/arch/arm/include/asm/kmap_types.h
> +++ b/arch/arm/include/asm/kmap_types.h
> @@ -22,4 +22,10 @@ enum km_type {
>  	KM_TYPE_NR
>  };
>  
> +#ifdef CONFIG_DEBUG_HIGHMEM
> +#define KM_NMI		(-1)
> +#define KM_NMI_PTE	(-1)
> +#define KM_IRQ_PTE	(-1)
> +#endif

Please solve this in mm/highmem.c as Andrew suggested it - other 
architectures could be affected as well beyond ARM.

	Ingo

Re: d451564 breakage

[Russell King]

On Sat, Nov 14, 2009 at 12:17:14AM +0100, Ingo Molnar wrote:
> 
> * Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> 
> > On Fri, Nov 13, 2009 at 11:48:20PM +0100, Ingo Molnar wrote:
> > > 
> > > * Russell King <rmk@arm.linux.org.uk> wrote:
> > > 
> > > > Change:
> > > > 
> > > > highmem: Fix debug_kmap_atomic() to also handle KM_IRQ_PTE, KM_NMI, and KM_NMI_PTE
> > > > 
> > > > Appears to break ARM:
> > > > 
> > > > mm/highmem.c: In function ???debug_kmap_atomic???:
> > > > mm/highmem.c:436: error: ???KM_NMI??? undeclared (first use in this function)
> > > 
> > > indeed - sorry.
> > > 
> > > Note that debug_kmap_atomic() will be removed in v2.6.33 so i'd suggest 
> > > to just do the easy solution and add #ifndef dummy definitions to 
> > > mm/highmem.c to cover ARM - we'll remove the whole cruft for good.
> > 
> > Actually, the following patch is probably the simplest solution.
> > 
> > > Btw., testing sidenote: i test the ARM defconfig and it didnt break 
> > > there. Perhaps highmem is off in the ARM defconfig? It would be helpful 
> > > if the ARM defconfig enabled highmem.
> > 
> > Given that highmem on ARM is experimental, I'd rather not have it enabled
> > in too many machine defconfigs as standard just yet.
> > 
> > However, enabling highmem on itself is not enough to show this breakage,
> > you also need highmem debugging enabled.
> > 
> >  arch/arm/include/asm/kmap_types.h |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/kmap_types.h b/arch/arm/include/asm/kmap_types.h
> > index d16ec97..c019949 100644
> > --- a/arch/arm/include/asm/kmap_types.h
> > +++ b/arch/arm/include/asm/kmap_types.h
> > @@ -22,4 +22,10 @@ enum km_type {
> >  	KM_TYPE_NR
> >  };
> >  
> > +#ifdef CONFIG_DEBUG_HIGHMEM
> > +#define KM_NMI		(-1)
> > +#define KM_NMI_PTE	(-1)
> > +#define KM_IRQ_PTE	(-1)
> > +#endif
> 
> Please solve this in mm/highmem.c as Andrew suggested it - other 
> architectures could be affected as well beyond ARM.

Have you actually put any thought into that approach?  What you're
suggesting means:

- adding preprocessor definitions to all existing places where these
  symbols are defined
- adding multiple complex ifdefs to mm/highmem.c thusly:

		if (
#ifdef KM_NMI
			type != KM_NMI
#else
			1
#endif
			&&
#ifdef KM_NMI_PTE
			type != KM_NMI_PTE
#else
			1
#endif
		) {
			WARN_ON(1);
			warn_count--;
		}

and I really don't think this is a realistic solution, even for the
interim.

The other solution is to determine if KM_NMI/KM_NMI_PTE/KM_IRQ_PTE are
x86 only (which does seem to be the case).  If that is the case, moving
the new definitions in my patch into mm/highmem.c itself and ifdef out
the ones in asm-generic/km_types.h would seem like another possible
approach.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: d451564 breakage

[Ingo Molnar]

* Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> On Sat, Nov 14, 2009 at 12:17:14AM +0100, Ingo Molnar wrote:
> > 
> > * Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> > 
> > > On Fri, Nov 13, 2009 at 11:48:20PM +0100, Ingo Molnar wrote:
> > > > 
> > > > * Russell King <rmk@arm.linux.org.uk> wrote:
> > > > 
> > > > > Change:
> > > > > 
> > > > > highmem: Fix debug_kmap_atomic() to also handle KM_IRQ_PTE, KM_NMI, and KM_NMI_PTE
> > > > > 
> > > > > Appears to break ARM:
> > > > > 
> > > > > mm/highmem.c: In function ???debug_kmap_atomic???:
> > > > > mm/highmem.c:436: error: ???KM_NMI??? undeclared (first use in this function)
> > > > 
> > > > indeed - sorry.
> > > > 
> > > > Note that debug_kmap_atomic() will be removed in v2.6.33 so i'd suggest 
> > > > to just do the easy solution and add #ifndef dummy definitions to 
> > > > mm/highmem.c to cover ARM - we'll remove the whole cruft for good.
> > > 
> > > Actually, the following patch is probably the simplest solution.
> > > 
> > > > Btw., testing sidenote: i test the ARM defconfig and it didnt break 
> > > > there. Perhaps highmem is off in the ARM defconfig? It would be helpful 
> > > > if the ARM defconfig enabled highmem.
> > > 
> > > Given that highmem on ARM is experimental, I'd rather not have it enabled
> > > in too many machine defconfigs as standard just yet.
> > > 
> > > However, enabling highmem on itself is not enough to show this breakage,
> > > you also need highmem debugging enabled.
> > > 
> > >  arch/arm/include/asm/kmap_types.h |    6 ++++++
> > >  1 files changed, 6 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/arch/arm/include/asm/kmap_types.h b/arch/arm/include/asm/kmap_types.h
> > > index d16ec97..c019949 100644
> > > --- a/arch/arm/include/asm/kmap_types.h
> > > +++ b/arch/arm/include/asm/kmap_types.h
> > > @@ -22,4 +22,10 @@ enum km_type {
> > >  	KM_TYPE_NR
> > >  };
> > >  
> > > +#ifdef CONFIG_DEBUG_HIGHMEM
> > > +#define KM_NMI		(-1)
> > > +#define KM_NMI_PTE	(-1)
> > > +#define KM_IRQ_PTE	(-1)
> > > +#endif
> > 
> > Please solve this in mm/highmem.c as Andrew suggested it - other 
> > architectures could be affected as well beyond ARM.
> 
> Have you actually put any thought into that approach? [...]

Yes.

> [...]  What you're suggesting means:
> 
> - adding preprocessor definitions to all existing places where these
>   symbols are defined
> - adding multiple complex ifdefs to mm/highmem.c thusly:
> 
> 		if (
> #ifdef KM_NMI
> 			type != KM_NMI
> #else
> 			1
> #endif
> 			&&
> #ifdef KM_NMI_PTE
> 			type != KM_NMI_PTE
> #else
> 			1
> #endif

I'd NAK such ugly code.

What i was thinking of was a really simple thing in highmem.c (or 
rather, in include/linux/highmem.h, after kmap_types.h gets included):

#ifndef KM_NMI
# define KM_NMI			-1
#endif
#ifndef KM_NMI_PTE
# define KM_NMI_PTE		-1
#endif

etc.

Thanks,

	Ingo

Re: d451564 breakage

[Russell King]

On Sat, Nov 14, 2009 at 01:21:38AM +0100, Ingo Molnar wrote:
> What i was thinking of was a really simple thing in highmem.c (or 
> rather, in include/linux/highmem.h, after kmap_types.h gets included):
> 
> #ifndef KM_NMI
> # define KM_NMI			-1
> #endif
> #ifndef KM_NMI_PTE
> # define KM_NMI_PTE		-1
> #endif
> 
> etc.

That means we need to ensure that kmap_atomic() fails when these are
used, to avoid kmap_atomic corrupting virtual mappings below the kmap
area should one of these be used.  That means additional run-time code
in all those implementations - I'd much rather leave that to someone
else to do.

Alternatively, they could be wrapped with CONFIG_DEBUG_HIGHMEM so they
aren't normally visible.

In any case, as I've already pointed out, we still need to add definitions
to all places which define these constants.  Having them exist as enum
constants does not make them visible to the preprocessor.

I don't think it's as simple as you're trying to make it.  However,
since you seem to have a solid idea which you're trying to get me to
implement, maybe it'd just be much simpler and faster if you provided
that patch.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: d451564 breakage

[Ingo Molnar]

* Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> On Sat, Nov 14, 2009 at 01:21:38AM +0100, Ingo Molnar wrote:
> > What i was thinking of was a really simple thing in highmem.c (or 
> > rather, in include/linux/highmem.h, after kmap_types.h gets included):
> > 
> > #ifndef KM_NMI
> > # define KM_NMI			-1
> > #endif
> > #ifndef KM_NMI_PTE
> > # define KM_NMI_PTE		-1
> > #endif
> > 
> > etc.
> 
> That means we need to ensure that kmap_atomic() fails when these are 
> used, to avoid kmap_atomic corrupting virtual mappings below the kmap 
> area should one of these be used.  That means additional run-time code 
> in all those implementations - I'd much rather leave that to someone 
> else to do.
> 
> Alternatively, they could be wrapped with CONFIG_DEBUG_HIGHMEM so they 
> aren't normally visible.
> 
> In any case, as I've already pointed out, we still need to add 
> definitions to all places which define these constants.  Having them 
> exist as enum constants does not make them visible to the 
> preprocessor.
> 
> I don't think it's as simple as you're trying to make it.  However, 
> since you seem to have a solid idea which you're trying to get me to 
> implement, maybe it'd just be much simpler and faster if you provided 
> that patch.

Since we are going to remove the generic code (the patch for that has 
been posted) it doesnt really matter and i'd suggest for you to commit 
the ARM patch.

	Ingo