[patch] slab: always follow arch requested alignments

[patch] slab: always follow arch requested alignments

[Heiko Carstens]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

In kmem_cache_create(): always check if BYTES_PER_WORD is less than
ARCH_SLAB_MINALIGN and disable debug options that would set the
alignment to BYTES_PER_WORD.
This will make sure that all slab caches will have at least an
ARCH_SLAB_MINALIGN alignment.

In addition make sure that a caller mandated align which is greater
than BYTES_PER_WORD also disables the same debug options.
This makes sure that ARCH_KMALLOC_MINALIGN also has an effect if
CONFIG_DEBUG_SLAB is set.

Cc: Christoph Lameter <clameter@sgi.com>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---

 mm/slab.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/mm/slab.c b/mm/slab.c
index 0f20843..1f6fc04 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2103,12 +2103,18 @@ #endif
 		if (ralign > BYTES_PER_WORD)
 			flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
 	}
+	if (BYTES_PER_WORD < ARCH_SLAB_MINALIGN)
+		flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
+
 	/* 3) caller mandated alignment: disables debug if necessary */
 	if (ralign < align) {
 		ralign = align;
 		if (ralign > BYTES_PER_WORD)
 			flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
 	}
+	if (align > BYTES_PER_WORD)
+		flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
+
 	/*
 	 * 4) Store it. Note that the debug code below can reduce
 	 *    the alignment to BYTES_PER_WORD.

Re: [patch] slab: always follow arch requested alignments

[Pekka Enberg]

On 7/22/06, Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> In kmem_cache_create(): always check if BYTES_PER_WORD is less than
> ARCH_SLAB_MINALIGN and disable debug options that would set the
> alignment to BYTES_PER_WORD.
> This will make sure that all slab caches will have at least an
> ARCH_SLAB_MINALIGN alignment.
>
> In addition make sure that a caller mandated align which is greater
> than BYTES_PER_WORD also disables the same debug options.
> This makes sure that ARCH_KMALLOC_MINALIGN also has an effect if
> CONFIG_DEBUG_SLAB is set.

Ok, but why?

                                                          Pekka

Re: [patch] slab: always follow arch requested alignments

[Christoph Lameter]

On Sat, 22 Jul 2006, Heiko Carstens wrote:

> In kmem_cache_create(): always check if BYTES_PER_WORD is less than
> ARCH_SLAB_MINALIGN and disable debug options that would set the
> alignment to BYTES_PER_WORD.

Why disable debug options?

> This will make sure that all slab caches will have at least an
> ARCH_SLAB_MINALIGN alignment.

You can specify alignment at cache creation. Why do we need all slabs to 
be aligned?

> In addition make sure that a caller mandated align which is greater
> than BYTES_PER_WORD also disables the same debug options.
> This makes sure that ARCH_KMALLOC_MINALIGN also has an effect if
> CONFIG_DEBUG_SLAB is set.

Is there a particular problem you are trying to address?

Re: [patch] slab: always follow arch requested alignments

[Heiko Carstens]

On Sat, Jul 22, 2006 at 07:50:00AM -0700, Christoph Lameter wrote:
> On Sat, 22 Jul 2006, Heiko Carstens wrote:
> 
> > In kmem_cache_create(): always check if BYTES_PER_WORD is less than
> > ARCH_SLAB_MINALIGN and disable debug options that would set the
> > alignment to BYTES_PER_WORD.
> 
> Why disable debug options?

Because if they are still enabled we would end up with an BYTES_PER_WORD
alignment (which is bad, see below).

> > This will make sure that all slab caches will have at least an
> > ARCH_SLAB_MINALIGN alignment.
> 
> You can specify alignment at cache creation. Why do we need all slabs to 
> be aligned?

Uhm, that's the meaning of ARCH_SLAB_MINALIGN, isn't it?

from mm/slab.c :

#ifndef ARCH_SLAB_MINALIGN
/*
 * Enforce a minimum alignment for all caches.
 * Intended for archs that get misalignment faults even for BYTES_PER_WORD
 * aligned buffers. Includes ARCH_KMALLOC_MINALIGN.
 * If possible: Do not enable this flag for CONFIG_DEBUG_SLAB, it disables
 * some debug features.
 */
#define ARCH_SLAB_MINALIGN 0
#endif

> > In addition make sure that a caller mandated align which is greater
> > than BYTES_PER_WORD also disables the same debug options.
> > This makes sure that ARCH_KMALLOC_MINALIGN also has an effect if
> > CONFIG_DEBUG_SLAB is set.
> 
> Is there a particular problem you are trying to address?

Sorry, I should have mentioned it: on s390 (32 bit) we set
#define ARCH_KMALLOC_MINALIGN 8.
This is needed since our common I/O layer allocates data structures that need
to have an eight byte alignment. Now, if I turn on DEBUG_SLAB, nothing works
anymore, simply because the slab cache code ignores ARCH_KMALLOC_MINALIGN and
uses an BYTES_PER_WORD alignment instead, which it shouldn't:

also from mm/slab.c :

#ifndef ARCH_KMALLOC_MINALIGN
/*
 * Enforce a minimum alignment for the kmalloc caches.
 * Usually, the kmalloc caches are cache_line_size() aligned, except when
 * DEBUG and FORCED_DEBUG are enabled, then they are BYTES_PER_WORD aligned.
 * Some archs want to perform DMA into kmalloc caches and need a guaranteed
 * alignment larger than BYTES_PER_WORD. ARCH_KMALLOC_MINALIGN allows that.
 * Note that this flag disables some debug features.
 */
#define ARCH_KMALLOC_MINALIGN 0
#endif

Since that didn't work I thought why not set ARCH_SLAB_MINALIGN to 8, since
that would (according to the description) guarantee that _all_ caches would
have an 8 byte alignment. But that didn't work too.

So the result is this patch, which makes DEBUG_SLAB work on s390. And actually
guarantees what the above descriptions imply (unless the patch is broken).

Re: [patch] slab: always follow arch requested alignments

[Christoph Lameter]

On Sat, 22 Jul 2006, Heiko Carstens wrote:

> Sorry, I should have mentioned it: on s390 (32 bit) we set
> #define ARCH_KMALLOC_MINALIGN 8.
> This is needed since our common I/O layer allocates data structures that need
> to have an eight byte alignment. Now, if I turn on DEBUG_SLAB, nothing works
> anymore, simply because the slab cache code ignores ARCH_KMALLOC_MINALIGN and
> uses an BYTES_PER_WORD alignment instead, which it shouldn't:
> 
> also from mm/slab.c :
> 
> #ifndef ARCH_KMALLOC_MINALIGN
> /*
>  * Enforce a minimum alignment for the kmalloc caches.
>  * Usually, the kmalloc caches are cache_line_size() aligned, except when
>  * DEBUG and FORCED_DEBUG are enabled, then they are BYTES_PER_WORD aligned.
>  * Some archs want to perform DMA into kmalloc caches and need a guaranteed
>  * alignment larger than BYTES_PER_WORD. ARCH_KMALLOC_MINALIGN allows that.
>  * Note that this flag disables some debug features.
>  */
> #define ARCH_KMALLOC_MINALIGN 0
> #endif
> 
> Since that didn't work I thought why not set ARCH_SLAB_MINALIGN to 8, since
> that would (according to the description) guarantee that _all_ caches would
> have an 8 byte alignment. But that didn't work too.

Why did that not work

See kmem_cache_create():
      /* 2) arch mandated alignment: disables debug if necessary */
        if (ralign < ARCH_SLAB_MINALIGN) {
                ralign = ARCH_SLAB_MINALIGN;
                if (ralign > BYTES_PER_WORD)
                        flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
        }

Re: [patch] slab: always follow arch requested alignments

[Heiko Carstens]

On Sat, Jul 22, 2006 at 12:42:32PM -0700, Christoph Lameter wrote:
> On Sat, 22 Jul 2006, Heiko Carstens wrote:
> > Since that didn't work I thought why not set ARCH_SLAB_MINALIGN to 8, since
> > that would (according to the description) guarantee that _all_ caches would
> > have an 8 byte alignment. But that didn't work too.
> 
> Why did that not work
> 
> See kmem_cache_create():
>       /* 2) arch mandated alignment: disables debug if necessary */
>         if (ralign < ARCH_SLAB_MINALIGN) {
>                 ralign = ARCH_SLAB_MINALIGN;
>                 if (ralign > BYTES_PER_WORD)
>                         flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
>         }

That is because if kmem_cache_create gets called with SLAB_HWCACHE_ALIGN set
in flags then ralign will be greater or equal to ARCH_SLAB_MINALIGN:

        /* 1) arch recommendation: can be overridden for debug */ 
        if (flags & SLAB_HWCACHE_ALIGN) { 
	        [...]
                ralign = cache_line_size(); 
	        [...]

Therefore the test above will be passed and SLAB_RED_ZONE and SLAB_STORE_USER
will stay in flags.
cache_line_size() will return 256 on s390.

Re: [patch] slab: always follow arch requested alignments

[Christoph Lameter]

On Sun, 23 Jul 2006, Heiko Carstens wrote:

> > 
> > See kmem_cache_create():
> >       /* 2) arch mandated alignment: disables debug if necessary */
> >         if (ralign < ARCH_SLAB_MINALIGN) {
> >                 ralign = ARCH_SLAB_MINALIGN;
> >                 if (ralign > BYTES_PER_WORD)
> >                         flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
> >         }
> 
> That is because if kmem_cache_create gets called with SLAB_HWCACHE_ALIGN set
> in flags then ralign will be greater or equal to ARCH_SLAB_MINALIGN:
> 
>         /* 1) arch recommendation: can be overridden for debug */ 
>         if (flags & SLAB_HWCACHE_ALIGN) { 
> 	        [...]
>                 ralign = cache_line_size(); 
> 	        [...]

Ok. Then you do not have a problem because ralign is greater than
ARCH_SLAB_MINALIGN.
 
> Therefore the test above will be passed and SLAB_RED_ZONE and SLAB_STORE_USER
> will stay in flags.
> cache_line_size() will return 256 on s390.

Looks as if you would have the correct alignment then. I still do not 
understand where the problem is since you want to align on an 8 byte 
boundary.

Re: [patch] slab: always follow arch requested alignments

[Heiko Carstens]

On Sun, Jul 23, 2006 at 06:03:09AM -0700, Christoph Lameter wrote:
> On Sun, 23 Jul 2006, Heiko Carstens wrote:
> > > See kmem_cache_create():
> > >       /* 2) arch mandated alignment: disables debug if necessary */
> > >         if (ralign < ARCH_SLAB_MINALIGN) {
> > >                 ralign = ARCH_SLAB_MINALIGN;
> > >                 if (ralign > BYTES_PER_WORD)
> > >                         flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
> > >         }
> > 
> > That is because if kmem_cache_create gets called with SLAB_HWCACHE_ALIGN set
> > in flags then ralign will be greater or equal to ARCH_SLAB_MINALIGN:
> > 
> >         /* 1) arch recommendation: can be overridden for debug */ 
> >         if (flags & SLAB_HWCACHE_ALIGN) { 
> > 	        [...]
> >                 ralign = cache_line_size(); 
> > 	        [...]
> 
> Ok. Then you do not have a problem because ralign is greater than
> ARCH_SLAB_MINALIGN.
>  
> > Therefore the test above will be passed and SLAB_RED_ZONE and SLAB_STORE_USER
> > will stay in flags.
> > cache_line_size() will return 256 on s390.
> 
> Looks as if you would have the correct alignment then. I still do not 
> understand where the problem is since you want to align on an 8 byte 
> boundary.

CONFIG_DEBUG_SLAB is on. In step 4) we have align = ralign.
Still ok.
Next thing:

	if (flags & SLAB_RED_ZONE) {
		/* redzoning only works with word aligned caches */
		align = BYTES_PER_WORD;

Result: align is less than ARCH_SLAB_MINALIGN -> busted.
Same is true if SLAB_STORE_USER is set.
Therefore I masked them both out in my patch.

Re: [patch] slab: always follow arch requested alignments

[Heiko Carstens]

On Sun, Jul 23, 2006 at 06:24:27PM +0200, Heiko Carstens wrote:
> On Sun, Jul 23, 2006 at 06:03:09AM -0700, Christoph Lameter wrote:
> > On Sun, 23 Jul 2006, Heiko Carstens wrote:
> > > > See kmem_cache_create():
> > > >       /* 2) arch mandated alignment: disables debug if necessary */
> > > >         if (ralign < ARCH_SLAB_MINALIGN) {
> > > >                 ralign = ARCH_SLAB_MINALIGN;
> > > >                 if (ralign > BYTES_PER_WORD)
> > > >                         flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
> > > >         }
> > > 
> > > That is because if kmem_cache_create gets called with SLAB_HWCACHE_ALIGN set
> > > in flags then ralign will be greater or equal to ARCH_SLAB_MINALIGN:
> > > 
> > >         /* 1) arch recommendation: can be overridden for debug */ 
> > >         if (flags & SLAB_HWCACHE_ALIGN) { 
> > > 	        [...]
> > >                 ralign = cache_line_size(); 
> > > 	        [...]
> > 
> > Ok. Then you do not have a problem because ralign is greater than
> > ARCH_SLAB_MINALIGN.
> >  
> > > Therefore the test above will be passed and SLAB_RED_ZONE and SLAB_STORE_USER
> > > will stay in flags.
> > > cache_line_size() will return 256 on s390.
> > 
> > Looks as if you would have the correct alignment then. I still do not 
> > understand where the problem is since you want to align on an 8 byte 
> > boundary.
> 
> CONFIG_DEBUG_SLAB is on. In step 4) we have align = ralign.
> Still ok.
> Next thing:
> 
> 	if (flags & SLAB_RED_ZONE) {
> 		/* redzoning only works with word aligned caches */
> 		align = BYTES_PER_WORD;
> 
> Result: align is less than ARCH_SLAB_MINALIGN -> busted.
> Same is true if SLAB_STORE_USER is set.
> Therefore I masked them both out in my patch.

Since I didn't get an ACK or NACK, I split this patch into two very small
ones and repost them. Hopefully with a better description than the first
time.

[patch 1/2] slab: always consider caller mandated alignment

[Heiko Carstens]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

In case of CONFIG_DEBUG_SLAB kmem_cache_create() creates caches with an
alignment lesser than ARCH_KMALLOC_MINALIGN. This breaks s390 (32bit),
since it needs an eight byte alignment. Also it doesn't behave like it's
decribed in mm/slab.c :

 * Enforce a minimum alignment for the kmalloc caches.
 * Usually, the kmalloc caches are cache_line_size() aligned, except when
 * DEBUG and FORCED_DEBUG are enabled, then they are BYTES_PER_WORD aligned.
 * Some archs want to perform DMA into kmalloc caches and need a guaranteed
 * alignment larger than BYTES_PER_WORD. ARCH_KMALLOC_MINALIGN allows that.
 * Note that this flag disables some debug features.

For example the following might happen if kmem_cache_create() gets called
with -- size: 64; align: 8; flags with SLAB_HWCACHE_ALIGN, SLAB_RED_ZONE and
SLAB_STORE_USER set.
These are the steps as numbered in kmem_cache_create() where 5) is after the
"if (flags & SLAB_RED_ZONE)" statement.

1) align: 8 ralign 64 
2) align: 8 ralign 64 
3) align: 8 ralign 64 
4) align: 64 ralign 64
5) align: 4 ralign 64 

Note that in this case in step 3) the flags SLAB_RED_ZONE and SLAB_STORE_USER
don't get masked out and that this causes an BYTES_PER_WORD alignment in
step 5) which breaks s390.

Cc: Christoph Lameter <clameter@sgi.com>
Cc: Pekka Enberg <penberg@cs.helsinki.fi>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---

 mm/slab.c |    3 +++
 1 files changed, 3 insertions(+)

Index: linux-2.6/mm/slab.c
===================================================================
--- linux-2.6.orig/mm/slab.c	2006-07-24 09:41:36.000000000 +0200
+++ linux-2.6/mm/slab.c	2006-07-26 09:55:54.000000000 +0200
@@ -2109,6 +2109,9 @@
 		if (ralign > BYTES_PER_WORD)
 			flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
 	}
+	if (align > BYTES_PER_WORD)
+		flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
+
 	/*
 	 * 4) Store it. Note that the debug code below can reduce
 	 *    the alignment to BYTES_PER_WORD.

[patch 2/2] slab: always consider arch mandated alignment

[Heiko Carstens]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

Since ARCH_KMALLOC_MINALIGN didn't work on s390 I tried ARCH_SLAB_MINALIGN
instead, just to find out that it didn't work too.
In case of CONFIG_DEBUG_SLAB kmem_cache_create() creates caches with an
alignment lesser than ARCH_SLAB_MINALIGN, which it shouldn't according to
this comment in mm/slab.c :

 * Enforce a minimum alignment for all caches.
 * Intended for archs that get misalignment faults even for BYTES_PER_WORD
 * aligned buffers. Includes ARCH_KMALLOC_MINALIGN.
 * If possible: Do not enable this flag for CONFIG_DEBUG_SLAB, it disables
 * some debug features.

For example the following might happen if kmem_cache_create() gets called
with -- size: 64; align: 0; flags with SLAB_HWCACHE_ALIGN, SLAB_RED_ZONE and
SLAB_STORE_USER set. ARCH_SLAB_MINALIGN is 8.
These are the steps as numbered in kmem_cache_create() where 5) is after the
"if (flags & SLAB_RED_ZONE)" statement.

1) align: 0 ralign 64
2) align: 0 ralign 64
3) align: 0 ralign 64
4) align: 64 ralign 64
5) align: 4 ralign 64

Note that in this case in step 2) the flags SLAB_RED_ZONE and SLAB_STORE_USER
don't get masked out and that this causes an BYTES_PER_WORD alignment in
step 5) which is lesser than ARCH_SLAB_MINALIGN.

Cc: Christoph Lameter <clameter@sgi.com>
Cc: Pekka Enberg <penberg@cs.helsinki.fi>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---

 mm/slab.c |    3 +++
 1 files changed, 3 insertions(+)

Index: linux-2.6/mm/slab.c
===================================================================
--- linux-2.6.orig/mm/slab.c	2006-07-26 09:55:54.000000000 +0200
+++ linux-2.6/mm/slab.c	2006-07-26 09:57:07.000000000 +0200
@@ -2103,6 +2103,9 @@
 		if (ralign > BYTES_PER_WORD)
 			flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
 	}
+	if (BYTES_PER_WORD < ARCH_SLAB_MINALIGN)
+		flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
+
 	/* 3) caller mandated alignment: disables debug if necessary */
 	if (ralign < align) {
 		ralign = align;

Re: [patch] slab: always follow arch requested alignments

[Pekka J Enberg]

Hi Heiko,

On Wed, 26 Jul 2006, Heiko Carstens wrote:
> Since I didn't get an ACK or NACK, I split this patch into two very small
> ones and repost them. Hopefully with a better description than the first
> time.

I don't see ARCH_SLAB_MINALIGN defined for s390, what's up with that? I 
agree that we should always respect ARCH_SLAB_MINALIGN no matter what. 
Does the included patch fix that for you?

				Pekka

[PATCH] slab: respect mandated minimum architecture alignment

The slab debugging code for SLAB_RED_ZONE and SLAB_STORE_USER require 
BYTES_PER_WORD alignment for the caches. However, for some architectures, 
the mandated minimum alignment might be bigger than that, so disable 
debugging for those cases instead of crashing the machine.

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---

diff --git a/mm/slab.c b/mm/slab.c
index 0f20843..0346311 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2123,6 +2123,13 @@ #endif
 #if DEBUG
 	cachep->obj_size = size;
 
+	/*
+	 * Debugging requires BYTES_PER_WORD alignment but we must always
+	 * respect ARCH_SLAB_MINALIGN so disable debugging if necessary.
+	 */
+	if (ARCH_SLAB_MINALIGN > BYTES_PER_WORD)
+		flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
+
 	if (flags & SLAB_RED_ZONE) {
 		/* redzoning only works with word aligned caches */
 		align = BYTES_PER_WORD;

Re: [patch 2/2] slab: always consider arch mandated alignment

[Pekka J Enberg]

On Wed, 26 Jul 2006, Heiko Carstens wrote:
> Since ARCH_KMALLOC_MINALIGN didn't work on s390 I tried ARCH_SLAB_MINALIGN
> instead, just to find out that it didn't work too.
> In case of CONFIG_DEBUG_SLAB kmem_cache_create() creates caches with an
> alignment lesser than ARCH_SLAB_MINALIGN, which it shouldn't according to
> this comment in mm/slab.c :

[snip]

> Index: linux-2.6/mm/slab.c
> ===================================================================
> --- linux-2.6.orig/mm/slab.c	2006-07-26 09:55:54.000000000 +0200
> +++ linux-2.6/mm/slab.c	2006-07-26 09:57:07.000000000 +0200
> @@ -2103,6 +2103,9 @@
>  		if (ralign > BYTES_PER_WORD)
>  			flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
>  	}
> +	if (BYTES_PER_WORD < ARCH_SLAB_MINALIGN)
> +		flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
> +
>  	/* 3) caller mandated alignment: disables debug if necessary */
>  	if (ralign < align) {
>  		ralign = align;

This is similar to my patch and should be enough to fix the problem. The 
first patch seems bogus and I don't really understand why you would need 
it.

					Pekka

Re: [patch 2/2] slab: always consider arch mandated alignment

[Heiko Carstens]

On Wed, Jul 26, 2006 at 01:05:43PM +0300, Pekka J Enberg wrote:
> On Wed, 26 Jul 2006, Heiko Carstens wrote:
> > Since ARCH_KMALLOC_MINALIGN didn't work on s390 I tried ARCH_SLAB_MINALIGN
> > instead, just to find out that it didn't work too.
> > In case of CONFIG_DEBUG_SLAB kmem_cache_create() creates caches with an
> > alignment lesser than ARCH_SLAB_MINALIGN, which it shouldn't according to
> > this comment in mm/slab.c :
> 
> [snip]
> 
> > Index: linux-2.6/mm/slab.c
> > ===================================================================
> > --- linux-2.6.orig/mm/slab.c	2006-07-26 09:55:54.000000000 +0200
> > +++ linux-2.6/mm/slab.c	2006-07-26 09:57:07.000000000 +0200
> > @@ -2103,6 +2103,9 @@
> >  		if (ralign > BYTES_PER_WORD)
> >  			flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
> >  	}
> > +	if (BYTES_PER_WORD < ARCH_SLAB_MINALIGN)
> > +		flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
> > +
> >  	/* 3) caller mandated alignment: disables debug if necessary */
> >  	if (ralign < align) {
> >  		ralign = align;
> 
> This is similar to my patch and should be enough to fix the problem. The 
> first patch seems bogus and I don't really understand why you would need 
> it.

It's enough to fix the ARCH_SLAB_MINALIGN problem. But it does _not_ fix the
ARCH_KMALLOC_MINALIGN problem. s390 currently only uses ARCH_KMALLOC_MINALIGN
since that should be good enough and it doesn't disable as much debugging
as ARCH_SLAB_MINALIGN does.
What exactly isn't clear from the description of the first patch? Or why do
you consider it bogus?

Re: [patch 2/2] slab: always consider arch mandated alignment

[Pekka J Enberg]

On Wed, 26 Jul 2006, Heiko Carstens wrote:
> It's enough to fix the ARCH_SLAB_MINALIGN problem. But it does _not_ fix the
> ARCH_KMALLOC_MINALIGN problem. s390 currently only uses ARCH_KMALLOC_MINALIGN
> since that should be good enough and it doesn't disable as much debugging
> as ARCH_SLAB_MINALIGN does.
> What exactly isn't clear from the description of the first patch? Or why do
> you consider it bogus?

Now I am confused. What do you mean by "doesn't disable as much debugging 
as ARCH_SLAB_MINALIGN does"? AFAICT, the SLAB_RED_ZONE and SLAB_STORE_USER 
options _require_ BYTES_PER_WORD alignment, so if s390 requires 8 
byte alignment, you can't have them debugging anyhow...

				Pekka

Re: [patch 2/2] slab: always consider arch mandated alignment

[Heiko Carstens]

On Wed, Jul 26, 2006 at 01:37:42PM +0300, Pekka J Enberg wrote:
> On Wed, 26 Jul 2006, Heiko Carstens wrote:
> > It's enough to fix the ARCH_SLAB_MINALIGN problem. But it does _not_ fix the
> > ARCH_KMALLOC_MINALIGN problem. s390 currently only uses ARCH_KMALLOC_MINALIGN
> > since that should be good enough and it doesn't disable as much debugging
> > as ARCH_SLAB_MINALIGN does.
> > What exactly isn't clear from the description of the first patch? Or why do
> > you consider it bogus?
> 
> Now I am confused. What do you mean by "doesn't disable as much debugging 
> as ARCH_SLAB_MINALIGN does"? AFAICT, the SLAB_RED_ZONE and SLAB_STORE_USER 
> options _require_ BYTES_PER_WORD alignment, so if s390 requires 8 
> byte alignment, you can't have them debugging anyhow...

We only specify ARCH_KMALLOC_MINALIGN, since that aligns only the kmalloc
caches, but it doesn't disable debugging on other caches that are created
via kmem_cache_create() where an alignment of e.g. 0 is specified.

The point of the first patch is: why should the slab cache be allowed to chose
an aligment that is less than what the caller specified? This does very likely
break things.

Re: [patch 2/2] slab: always consider arch mandated alignment

[Pekka J Enberg]

On Wed, 26 Jul 2006, Heiko Carstens wrote:
> We only specify ARCH_KMALLOC_MINALIGN, since that aligns only the kmalloc
> caches, but it doesn't disable debugging on other caches that are created
> via kmem_cache_create() where an alignment of e.g. 0 is specified.
> 
> The point of the first patch is: why should the slab cache be allowed to chose
> an aligment that is less than what the caller specified? This does very likely
> break things.

Ah, yes, you are absolutely right. We need to respect caller mandated 
alignment too. How about this?

			Pekka

[PATCH] slab: respect architecture and caller mandated alignment

Ensure cache alignment is always at minimum what the architecture or 
caller mandates even if slab debugging is enabled.

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---

diff --git a/mm/slab.c b/mm/slab.c
index 0f20843..3767460 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2097,6 +2097,15 @@ #endif
 	} else {
 		ralign = BYTES_PER_WORD;
 	}
+
+	/*
+	 * Redzoning and user store require word alignment. Note this will be
+	 * overridden by architecture or caller mandated alignment if either
+	 * is greater than BYTES_PER_WORD.
+	 */
+	if (flags & SLAB_RED_ZONE || flags & SLAB_STORE_USER)
+		ralign = BYTES_PER_WORD;
+
 	/* 2) arch mandated alignment: disables debug if necessary */
 	if (ralign < ARCH_SLAB_MINALIGN) {
 		ralign = ARCH_SLAB_MINALIGN;
@@ -2123,20 +2132,19 @@ #endif
 #if DEBUG
 	cachep->obj_size = size;
 
+	/*
+	 * Both debugging options require word-alignment which is calculated
+	 * into align above.
+	 */
 	if (flags & SLAB_RED_ZONE) {
-		/* redzoning only works with word aligned caches */
-		align = BYTES_PER_WORD;
-
 		/* add space for red zone words */
 		cachep->obj_offset += BYTES_PER_WORD;
 		size += 2 * BYTES_PER_WORD;
 	}
 	if (flags & SLAB_STORE_USER) {
-		/* user store requires word alignment and
-		 * one word storage behind the end of the real
-		 * object.
+		/* user store requires one word storage behind the end of
+		 * the real object.
 		 */
-		align = BYTES_PER_WORD;
 		size += BYTES_PER_WORD;
 	}
 #if FORCED_DEBUG && defined(CONFIG_DEBUG_PAGEALLOC)

Re: [patch] slab: always follow arch requested alignments

[Pekka Enberg]

Hi Heiko,

On 7/22/06, Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> Sorry, I should have mentioned it: on s390 (32 bit) we set
> #define ARCH_KMALLOC_MINALIGN 8.
> This is needed since our common I/O layer allocates data structures that need
> to have an eight byte alignment. Now, if I turn on DEBUG_SLAB, nothing works
> anymore, simply because the slab cache code ignores ARCH_KMALLOC_MINALIGN and
> uses an BYTES_PER_WORD alignment instead, which it shouldn't:

This is the bit I missed, sorry. I thought that the s390 hardware
mandates 8 byte alignment, but it really doesn't. So you're absolutely
right, you don't need to set ARCH_SLAB_MINALIGN and the alignment
calculation in slab is indeed broken for both, architecture and caller
mandated alignments.

                                     Pekka

Re: [patch 2/2] slab: always consider arch mandated alignment

[Heiko Carstens]

On Wed, Jul 26, 2006 at 02:16:06PM +0300, Pekka J Enberg wrote:
> On Wed, 26 Jul 2006, Heiko Carstens wrote:
> > We only specify ARCH_KMALLOC_MINALIGN, since that aligns only the kmalloc
> > caches, but it doesn't disable debugging on other caches that are created
> > via kmem_cache_create() where an alignment of e.g. 0 is specified.
> > 
> > The point of the first patch is: why should the slab cache be allowed to chose
> > an aligment that is less than what the caller specified? This does very likely
> > break things.
> 
> Ah, yes, you are absolutely right. We need to respect caller mandated 
> alignment too. How about this?

Works fine and looks much better than my two patches. Thanks!

Re: [patch] slab: always follow arch requested alignments

[Christoph Lameter]

On Wed, 26 Jul 2006, Pekka Enberg wrote:

> Hi Heiko,
> 
> On 7/22/06, Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> > Sorry, I should have mentioned it: on s390 (32 bit) we set
> > #define ARCH_KMALLOC_MINALIGN 8.
> > This is needed since our common I/O layer allocates data structures that
> > need
> > to have an eight byte alignment. Now, if I turn on DEBUG_SLAB, nothing works
> > anymore, simply because the slab cache code ignores ARCH_KMALLOC_MINALIGN
> > and
> > uses an BYTES_PER_WORD alignment instead, which it shouldn't:
> 
> This is the bit I missed, sorry. I thought that the s390 hardware
> mandates 8 byte alignment, but it really doesn't. So you're absolutely
> right, you don't need to set ARCH_SLAB_MINALIGN and the alignment
> calculation in slab is indeed broken for both, architecture and caller
> mandated alignments.

Well that is a bit far reaching. What is broken is that SLAB_RED_ZONE and
SLAB_STORE_USER ignore any given alignment. If you want to fix that then 
you need to modify how both debugging methods work.

Re: [patch] slab: always follow arch requested alignments

[Pekka J Enberg]

On Wed, 26 Jul 2006, Pekka Enberg wrote:
> > This is the bit I missed, sorry. I thought that the s390 hardware
> > mandates 8 byte alignment, but it really doesn't. So you're absolutely
> > right, you don't need to set ARCH_SLAB_MINALIGN and the alignment
> > calculation in slab is indeed broken for both, architecture and caller
> > mandated alignments.

On Wed, 26 Jul 2006, Christoph Lameter wrote:
> Well that is a bit far reaching. What is broken is that SLAB_RED_ZONE and
> SLAB_STORE_USER ignore any given alignment. If you want to fix that then 
> you need to modify how both debugging methods work.

Not sure I understand what you mean. Isn't it enough that we disable 
debugging if architecture or caller mandated alignment is greater than 
BYTES_PER_WORD?

				Pekka 

Re: [patch] slab: always follow arch requested alignments

[Christoph Lameter]

On Wed, 26 Jul 2006, Pekka J Enberg wrote:

> On Wed, 26 Jul 2006, Christoph Lameter wrote:
> > Well that is a bit far reaching. What is broken is that SLAB_RED_ZONE and
> > SLAB_STORE_USER ignore any given alignment. If you want to fix that then 
> > you need to modify how both debugging methods work.
> 
> Not sure I understand what you mean. Isn't it enough that we disable 
> debugging if architecture or caller mandated alignment is greater than 
> BYTES_PER_WORD?

If you disable them then we are fine. I think the main "bug" is that 
we create the caches with ARCH_KMALLOC_MINALIGN in kmem_cache_init but 
allow debug options on them. It seemss that we need to be able to disable 
debugging from kmem_cache_init.

Re: [patch] slab: always follow arch requested alignments

[Pekka J Enberg]

On Wed, 26 Jul 2006, Christoph Lameter wrote:
> If you disable them then we are fine. I think the main "bug" is that 
> we create the caches with ARCH_KMALLOC_MINALIGN in kmem_cache_init but 
> allow debug options on them. It seemss that we need to be able to disable 
> debugging from kmem_cache_init.

No, as Heiko explained, but bug is that we fail to respect architecture 
and caller mandated alignment when CONFIG_SLAB_DEBUG is enabled. With this 
patch (or Heiko's), we should be okay: http://lkml.org/lkml/2006/7/26/93

Note that this will fix the kmem_cache_init() case too. If 
ARCH_KMALLOC_MINALIGN is greater than BYTES_PER_WORD, we'll disable 
debugging for those caches. It's obviously ok to have debugging for 
kmem_cache_init caches too if ARCH_KMALLOC_MINALIGN is greater than or 
equal to BYTES_PER_WORD.

					Pekka

Re: [patch] slab: always follow arch requested alignments

[Pekka J Enberg]

On Wed, 26 Jul 2006, Pekka J Enberg wrote:
> Note that this will fix the kmem_cache_init() case too. If 
> ARCH_KMALLOC_MINALIGN is greater than BYTES_PER_WORD, we'll disable 
> debugging for those caches. It's obviously ok to have debugging for 
> kmem_cache_init caches too if ARCH_KMALLOC_MINALIGN is greater than or 
> equal to BYTES_PER_WORD.

Eh, meant "if ARCH_KMALLOC_MINALIGN is less than or equal to 
BYTES_PER_WORD" obviously.

Re: [patch] slab: always follow arch requested alignments

[Christoph Lameter]

On Wed, 26 Jul 2006, Pekka J Enberg wrote:

> On Wed, 26 Jul 2006, Pekka J Enberg wrote:
> > Note that this will fix the kmem_cache_init() case too. If 
> > ARCH_KMALLOC_MINALIGN is greater than BYTES_PER_WORD, we'll disable 
> > debugging for those caches. It's obviously ok to have debugging for 
> > kmem_cache_init caches too if ARCH_KMALLOC_MINALIGN is greater than or 
> > equal to BYTES_PER_WORD.
> 
> Eh, meant "if ARCH_KMALLOC_MINALIGN is less than or equal to 
> BYTES_PER_WORD" obviously.

Your patch only deals with ARCH_SLAB_MINALIGN. kmem_cache_create() never 
uses ARCH_KMALLOC_MINALIGN only kmem_cache_init() does by passing it to 
kmem_cache_create.  ARCH_KMALLOC_MINALIGN will still be ignored.

Re: [patch] slab: always follow arch requested alignments

[Pekka Enberg]

On 7/26/06, Christoph Lameter <clameter@sgi.com> wrote:
> Your patch only deals with ARCH_SLAB_MINALIGN. kmem_cache_create() never
> uses ARCH_KMALLOC_MINALIGN only kmem_cache_init() does by passing it to
> kmem_cache_create.  ARCH_KMALLOC_MINALIGN will still be ignored.

Yes, in which case the caller mandated align will be, well,
ARCH_KMALLOC_MINALIGN. The patch changes kmem_cache_create to respect
caller mandated alignment too.

                                        Pekka

Re: [patch] slab: always follow arch requested alignments

[Christoph Lameter]

On Wed, 26 Jul 2006, Pekka Enberg wrote:

> On 7/26/06, Christoph Lameter <clameter@sgi.com> wrote:
> > Your patch only deals with ARCH_SLAB_MINALIGN. kmem_cache_create() never
> > uses ARCH_KMALLOC_MINALIGN only kmem_cache_init() does by passing it to
> > kmem_cache_create.  ARCH_KMALLOC_MINALIGN will still be ignored.
> 
> Yes, in which case the caller mandated align will be, well,
> ARCH_KMALLOC_MINALIGN. The patch changes kmem_cache_create to respect
> caller mandated alignment too.

As far as I understood Heiko s390 does not set ARCH_SLAB_MINALIGN
because they do not want alignent for all caches.

The following patch adds an option SLAB_DEBUG_OVERRIDE to switch off
debugging if its on by default. S390 would have to set ARCH_KMALLOC_FLAGS
to SLAB_DEBUG_OVERRIDE. The flag will then be passed in 
kmem_cache_init to kmem_cache_create(). This approach also preserves the 
existing slab behavior for all other archs.

Index: linux-2.6/include/linux/slab.h
===================================================================
--- linux-2.6.orig/include/linux/slab.h	2006-07-26 05:14:33.000000000 -0700
+++ linux-2.6/include/linux/slab.h	2006-07-26 05:15:08.000000000 -0700
@@ -46,6 +46,7 @@ typedef struct kmem_cache kmem_cache_t;
 #define SLAB_PANIC		0x00040000UL	/* panic if kmem_cache_create() fails */
 #define SLAB_DESTROY_BY_RCU	0x00080000UL	/* defer freeing pages to RCU */
 #define SLAB_MEM_SPREAD		0x00100000UL	/* Spread some memory over cpuset */
+#define SLAB_DEBUG_OVERRIDE	0x00200000UL	/* Do not debug this slab */
 
 /* flags passed to a constructor func */
 #define	SLAB_CTOR_CONSTRUCTOR	0x001UL		/* if not set, then deconstructor */
Index: linux-2.6/mm/slab.c
===================================================================
--- linux-2.6.orig/mm/slab.c	2006-07-26 05:13:59.000000000 -0700
+++ linux-2.6/mm/slab.c	2006-07-26 05:17:59.000000000 -0700
@@ -2055,8 +2055,11 @@ kmem_cache_create (const char *name, siz
 	 * above the next power of two: caches with object sizes just above a
 	 * power of two have a significant amount of internal fragmentation.
 	 */
-	if (size < 4096 || fls(size - 1) == fls(size-1 + 3 * BYTES_PER_WORD))
-		flags |= SLAB_RED_ZONE | SLAB_STORE_USER;
+
+	if (!(flags & DEBUG_OVERRIDE) &&
+		size < 4096 ||
+		(fls(size - 1) == fls(size-1 + 3 * BYTES_PER_WORD)))
+			flags |= SLAB_RED_ZONE | SLAB_STORE_USER;
 	if (!(flags & SLAB_DESTROY_BY_RCU))
 		flags |= SLAB_POISON;
 #endif

Re: [patch] slab: always follow arch requested alignments

[Pekka J Enberg]

On Wed, 26 Jul 2006, Christoph Lameter wrote:
> The following patch adds an option SLAB_DEBUG_OVERRIDE to switch off
> debugging if its on by default. S390 would have to set ARCH_KMALLOC_FLAGS
> to SLAB_DEBUG_OVERRIDE. The flag will then be passed in 
> kmem_cache_init to kmem_cache_create(). This approach also preserves the 
> existing slab behavior for all other archs.

Please read my patch again. The rules are simple: we must disable 
debugging if architecture OR caller mandated alignment is greater than 
BYTES_PER_WORD. Note: for kmem_cache_init() the caller mandated alignment 
_is_ ARCH_KMALLOC_MINALIGN.

My patch takes care of _both_ ARCH_KMALLOC_MINALIGN and 
ARCH_SLAB_MINALIGN.

				Pekka

Re: [patch] slab: always follow arch requested alignments

[Pekka J Enberg]

On 7/26/06, Christoph Lameter <clameter@sgi.com> wrote:
> > > Your patch only deals with ARCH_SLAB_MINALIGN. kmem_cache_create() never
> > > uses ARCH_KMALLOC_MINALIGN only kmem_cache_init() does by passing it to
> > > kmem_cache_create.  ARCH_KMALLOC_MINALIGN will still be ignored.
 
On Wed, 26 Jul 2006, Pekka Enberg wrote:
> > Yes, in which case the caller mandated align will be, well,
> > ARCH_KMALLOC_MINALIGN. The patch changes kmem_cache_create to respect
> > caller mandated alignment too.

On Wed, 26 Jul 2006, Christoph Lameter wrote:
> As far as I understood Heiko s390 does not set ARCH_SLAB_MINALIGN
> because they do not want alignent for all caches.

Correct. Heiko sets ARCH_KMALLOC_MINALIGN which will be passed as the 
'align' parameter to kmem_cache_create -- also known as 'caller mandated 
alignment.'

My patch changes the code so that, if either architecture or 
caller mandated alignment is greater than BYTES_PER_WORD, 
kmem_cache_create will disable debugging. Do you now see why my patch is 
in fact _not_ ignoring ARCH_KMALLOC_MINALIGN, but instead respecting that.

			Pekka

Re: [patch] slab: always follow arch requested alignments

[Pekka J Enberg]

On Wed, 26 Jul 2006, Pekka J Enberg wrote:
> My patch changes the code so that, if either architecture or 
> caller mandated alignment is greater than BYTES_PER_WORD, 
> kmem_cache_create will disable debugging. Do you now see why my patch is 
> in fact _not_ ignoring ARCH_KMALLOC_MINALIGN, but instead respecting that.

And btw, I am referring to this patch: http://lkml.org/lkml/2006/7/26/93. 
Not the one I posted initially.

				Pekka

Re: [patch] slab: always follow arch requested alignments

[Christoph Lameter]

On Wed, 26 Jul 2006, Pekka J Enberg wrote:

> On Wed, 26 Jul 2006, Christoph Lameter wrote:
> > The following patch adds an option SLAB_DEBUG_OVERRIDE to switch off
> > debugging if its on by default. S390 would have to set ARCH_KMALLOC_FLAGS
> > to SLAB_DEBUG_OVERRIDE. The flag will then be passed in 
> > kmem_cache_init to kmem_cache_create(). This approach also preserves the 
> > existing slab behavior for all other archs.
> 
> Please read my patch again. The rules are simple: we must disable 
> debugging if architecture OR caller mandated alignment is greater than 
> BYTES_PER_WORD. Note: for kmem_cache_init() the caller mandated alignment 
> _is_ ARCH_KMALLOC_MINALIGN.

We intentionally discard the caller mandated alignment for debugging 
purposes. 
 
> My patch takes care of _both_ ARCH_KMALLOC_MINALIGN and 
> ARCH_SLAB_MINALIGN.

And it changes the basic way that slab debugging works. 

Re: Re: [patch] slab: always follow arch requested alignments

[Pekka Enberg]

Hi Christoph,

On 7/26/06, Christoph Lameter <clameter@sgi.com> wrote:
> We intentionally discard the caller mandated alignment for debugging
> purposes.

Disagreed. The caller mandated alignment is not a hint. It is the
required minimum alignment for objects.

On 7/26/06, Christoph Lameter <clameter@sgi.com> wrote:
> And it changes the basic way that slab debugging works.

Look at kmem_cache_create, we turn off debugging for both caller and
architecture mandated alignments already and the only reason we are
not doing it for Heiko is because the architecture recommended default
alignment is so large.

                                      Pekka

Re: Re: [patch] slab: always follow arch requested alignments

[Christoph Lameter]

On Wed, 26 Jul 2006, Pekka Enberg wrote:

> On 7/26/06, Christoph Lameter <clameter@sgi.com> wrote:
> > We intentionally discard the caller mandated alignment for debugging
> > purposes.
> 
> Disagreed. The caller mandated alignment is not a hint. It is the
> required minimum alignment for objects.

This has been with the slab for a long time. Lots of alignments are now
ignored for the debugging case without a problem. Manfred intentionally
put that in. Alignments passed to kmem_cache_create are there for
performance reasons and not because unaligned object break the arch code.
 
> On 7/26/06, Christoph Lameter <clameter@sgi.com> wrote:
> > And it changes the basic way that slab debugging works.
> 
> Look at kmem_cache_create, we turn off debugging for both caller and
> architecture mandated alignments already and the only reason we are
> not doing it for Heiko is because the architecture recommended default
> alignment is so large.

We discard alignment for FORCED_DEBUG unless

1. object size > 4096

2. If the object size would increase unreasonably.

I simply added another case. That preseves the discarding of alignment for 
the FORCED_DEBUG case but allows an override if operations would be 
impossible for not correctly aligned objects for certain caches (like in 
the S/390 case).

Re: [patch 2/2] slab: always consider arch mandated alignment

[Manfred Spraul]

Pekka J Enberg wrote:

>On Wed, 26 Jul 2006, Heiko Carstens wrote:
>  
>
>>We only specify ARCH_KMALLOC_MINALIGN, since that aligns only the kmalloc
>>caches, but it doesn't disable debugging on other caches that are created
>>via kmem_cache_create() where an alignment of e.g. 0 is specified.
>>
>>The point of the first patch is: why should the slab cache be allowed to chose
>>an aligment that is less than what the caller specified? This does very likely
>>break things.
>>    
>>
>
>Ah, yes, you are absolutely right. We need to respect caller mandated 
>alignment too. How about this?
>
>  
>
Good catch - I obviously never tested the code for an HWCACHE_ALIGN cache...


>			Pekka
>
>[PATCH] slab: respect architecture and caller mandated alignment
>
>Ensure cache alignment is always at minimum what the architecture or 
>caller mandates even if slab debugging is enabled.
>
>Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
>  
>
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>

--
    Manfred

Re: [patch 2/2] slab: always consider arch mandated alignment

[Christoph Lameter]

On Wed, 26 Jul 2006, Manfred Spraul wrote:

> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>

Good bye to all those cacheline contentions that helped us find so many 
race conditions in the past if we switched on SLAB_DEBUG. I thought this 
was intentional?

Re: [patch] slab: always follow arch requested alignments

[Manfred Spraul]

Pekka Enberg wrote:

> Hi Christoph,
>
> On 7/26/06, Christoph Lameter <clameter@sgi.com> wrote:
>
>> We intentionally discard the caller mandated alignment for debugging
>> purposes.
>
>
There are two different types of alignment:
- SLAB_HWCACHE_ALIGN: it's a recommendation, it's regularly ignored.
- the align parameter, or ARCH_SLAB_MINALIGN: It's mandatory. For 
example the pgd structures must be 4 kB aligned, it's required by the 
hardware. And I think there was (is?) a structure where ptr & ~(size-1) 
was used to find the start of the structure.

Thus the patch is correct, it's a bug in the slab allocator. If 
HWCACHE_ALIGN is set, then the allocator ignores align or 
ARCH_SLAB_MINALIGN.

--
    Manfred

Re: [patch] slab: always follow arch requested alignments

[Christoph Lameter]

On Wed, 26 Jul 2006, Manfred Spraul wrote:

> There are two different types of alignment:
> - SLAB_HWCACHE_ALIGN: it's a recommendation, it's regularly ignored.
> - the align parameter, or ARCH_SLAB_MINALIGN: It's mandatory. For example the
> pgd structures must be 4 kB aligned, it's required by the hardware. And I
> think there was (is?) a structure where ptr & ~(size-1) was used to find the
> start of the structure.

I agree with the above if there is an issue there then lets fix it.

> Thus the patch is correct, it's a bug in the slab allocator. If HWCACHE_ALIGN
> is set, then the allocator ignores align or ARCH_SLAB_MINALIGN.

But then Heiko does not want to set ARCH_SLAB_MINALIGN at all. This is not 
the issue we are discussing. In the DEBUG case he wants 
ARCH_KMALLOC_MINALIGN to be enforced even if ARCH_SLAB_MINALIGN is not 
set.

Re: [patch 2/2] slab: always consider arch mandated alignment

[Manfred Spraul]

Christoph Lameter wrote:

>On Wed, 26 Jul 2006, Manfred Spraul wrote:
>
>  
>
>>Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
>>    
>>
>
>Good bye to all those cacheline contentions that helped us find so many 
>race conditions in the past if we switched on SLAB_DEBUG. I thought this 
>was intentional?
>
>  
>
Relax, align is nearly never set.

- kmalloc uses align==0, except if the architecture requests it 
(ARCH_KMALLOC_MINALIGN not 0)
- on my i386 system, the following users explicitely use align:
* the pmd structure (4096: hardware requirement)
* the pgd structure (32 bytes: hardware requirement)
* the task structure (16 byte. fxsave)
* sigqueue, pid: both request 4 byte alignment (based on __alignof__()). 
Doesn't affect debugging.

 From the other mail:

>Thus the patch is correct, it's a bug in the slab allocator. If HWCACHE_ALIGN
>> is set, then the allocator ignores align or ARCH_SLAB_MINALIGN.
>  
>
>
>But then Heiko does not want to set ARCH_SLAB_MINALIGN at all. This is not 
>the issue we are discussing. In the DEBUG case he wants 
>ARCH_KMALLOC_MINALIGN to be enforced even if ARCH_SLAB_MINALIGN is not 
>set.
>  
>
The kmalloc caches are allocated with 
HWCACHE_ALIGN+ARCH_KMALLOC_MINALIGN. The logic in kmem_cache_create 
didn't handle that case correctly.
On most architectures, ARCH_KMALLOC_MINALIGN is 0. Thus SLAB_DEBUG 
redzones everything.
On s390, ARCH_KMALLOC_MINALIGN is 8. This disables redzoning.

Ok?

--
    Manfred

Re: [patch 2/2] slab: always consider arch mandated alignment

[Christoph Lameter]

On Wed, 26 Jul 2006, Manfred Spraul wrote:

> > Thus the patch is correct, it's a bug in the slab allocator. If
> > HWCACHE_ALIGN
> > > is set, then the allocator ignores align or ARCH_SLAB_MINALIGN.
> >  
> > 
> > But then Heiko does not want to set ARCH_SLAB_MINALIGN at all. This is not
> > the issue we are discussing. In the DEBUG case he wants
> > ARCH_KMALLOC_MINALIGN to be enforced even if ARCH_SLAB_MINALIGN is not set.
> >  
> The kmalloc caches are allocated with HWCACHE_ALIGN+ARCH_KMALLOC_MINALIGN. The
> logic in kmem_cache_create didn't handle that case correctly.
> On most architectures, ARCH_KMALLOC_MINALIGN is 0. Thus SLAB_DEBUG redzones
> everything.
> On s390, ARCH_KMALLOC_MINALIGN is 8. This disables redzoning.
> 
> Ok?

So Redzoning etc will now be diabled regardless even if  
ARCH_SLAB_MINALIGN is not set but another alignment is given to 
kmem_cache_alloc?

So we sacrifice the ability to worsen the performance of slabs by 
misaligning them for debugging purposes.

Re: [patch 2/2] slab: always consider arch mandated alignment

[Manfred Spraul]

Christoph Lameter wrote:

>So Redzoning etc will now be diabled regardless even if  
>ARCH_SLAB_MINALIGN is not set but another alignment is given to 
>kmem_cache_alloc?
>  
>
kmem_cache_create().

>So we sacrifice the ability to worsen the performance of slabs by 
>misaligning them for debugging purposes.
>  
>
If a slab user can live with misaligned objects, then he shouldn't set 
align. Thus we do not sacrifice anything.

--
    Manfred

Re: [patch 2/2] slab: always consider arch mandated alignment

[Christoph Lameter]

On Wed, 26 Jul 2006, Manfred Spraul wrote:

> > So we sacrifice the ability to worsen the performance of slabs by
> > misaligning them for debugging purposes.
> >  
> If a slab user can live with misaligned objects, then he shouldn't set align.
> Thus we do not sacrifice anything.

A slab user is setting alignment in order to optimize performance not for 
correctness. Most users that I know of can live with misalignments. If 
that would not be the case then this code would never have worked.

Re: [patch 2/2] slab: always consider arch mandated alignment

[Manfred Spraul]

Christoph Lameter wrote:

>A slab user is setting alignment in order to optimize performance not for 
>correctness. Most users that I know of can live with misalignments. If 
>that would not be the case then this code would never have worked.
>  
>

Which users do you know that set align and that can live with misalignments?
As I wrote, there are no such users in my (i386) kernel.

--
    Manfred

Re: [patch 2/2] slab: always consider arch mandated alignment

[Christoph Lameter]

On Wed, 26 Jul 2006, Manfred Spraul wrote:

> Christoph Lameter wrote:
> 
> > A slab user is setting alignment in order to optimize performance not for
> > correctness. Most users that I know of can live with misalignments. If that
> > would not be the case then this code would never have worked.
> >  
> 
> Which users do you know that set align and that can live with misalignments?
> As I wrote, there are no such users in my (i386) kernel.

The users of SLAB_HWCACHE_ALIGN can live with that.

Systems running with slab debugging on must be very buggy at this point or 
we were very lucky:

The list is a bit strange:

>* the pmd structure (4096: hardware requirement)

It is already exempted from debug since the size is 4096.

>* the pgd structure (32 bytes: hardware requirement)

We were lucky on that one in the past? This should break.

>* the task structure (16 byte. fxsave)

Would only break if floating point is used I think.

>* sigqueue, pid: both request 4 byte alignment (based on __alignof__()). 
>Doesn't affect debugging.
So also not relevant.


We now want to say that SLAB_HWCACHE_ALIGN is only a suggestion to be 
disposed of if debug is on whereas an explicitly specified alignment must be enforced?

Re: [patch 2/2] slab: always consider arch mandated alignment

[Pekka J Enberg]

On Wed, 26 Jul 2006, Christoph Lameter wrote:
> A slab user is setting alignment in order to optimize performance not for 
> correctness. Most users that I know of can live with misalignments. If 
> that would not be the case then this code would never have worked.

No, caller and architecture mandated alignment is set for correctness. The 
slab allocator can already optimize for performance on its own 
(SLAB_HWCACHE_ALIGN).

				Pekka

Re: [patch 2/2] slab: always consider arch mandated alignment

[Pekka J Enberg]

On Wed, 26 Jul 2006, Christoph Lameter wrote:
> We now want to say that SLAB_HWCACHE_ALIGN is only a suggestion to be 
> disposed of if debug is on whereas an explicitly specified alignment must be enforced?

Yes and that's what we have been saying all along. When you want 
performance, you use SLAB_HWCACHE_ALIGN and let the allocator do its job. 
I don't see much point from API point of view for the caller to explicitly 
ask for a given alignment and then in addition pass a 'yes I really meant' 
flag (SLAB_DEBUG_OVERRIDE).

So Christoph, unless you can point out a specific problem with my patch, 
I'll queue it up to Andrew. Thanks.

				Pekka

Re: [patch 2/2] slab: always consider arch mandated alignment

[Pekka J Enberg]

On Thu, 27 Jul 2006, Pekka J Enberg wrote:
> Yes and that's what we have been saying all along. When you want 
> performance, you use SLAB_HWCACHE_ALIGN and let the allocator do its job. 
> I don't see much point from API point of view for the caller to explicitly 
> ask for a given alignment and then in addition pass a 'yes I really meant' 
> flag (SLAB_DEBUG_OVERRIDE).

Btw, /proc/slabinfo for UML with defconfig reveals change for only one 
cache with my patch applied. The 'dquot' cache is created by dquot_init in 
fs/dquot.c and doesn't really seem to need the alignment for anything...

				Pekka