slab: always follow arch requested alignments

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.

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.

Quoted 3 lines · Heiko Carstens

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?

Quoted 2 lines

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?

Quoted 4 lines

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.

Quoted 3 lines · Heiko Carstens

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);
        }

Quoted 8 lines

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.

Quoted 8 lines · Heiko Carstens

Quoted 7 lines

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);
        }

Quoted 8 lines

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.

Quoted 3 lines

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.

Quoted 21 lines · Christoph Lameter

Quoted 8 lines · Heiko Carstens

Quoted 7 lines

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);
        }

Quoted 8 lines

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.

Quoted 7 lines

Quoted 3 lines

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.

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 :

Index: linux-2.6/mm/slab.c
===================================================================

Quoted 5 lines · Heiko Carstens

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]

Quoted 10 lines

Index: linux-2.6/mm/slab.c
===================================================================

mm/slab.c +2 -0

@@ -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);

mm/slab.c line 2111

Quoted 4 lines

mm/slab.c +1 -0

+
 	/* 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?

Quoted 6 lines · Heiko Carstens

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.

Quoted 7 lines · Heiko Carstens

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?

Quoted 4 lines · Heiko Carstens

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.

Quoted 5 lines

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 <[email protected]>

Signed-off-by Manfred Spraul <[email protected]>

On Wed, 26 Jul 2006, Manfred Spraul wrote:

Quoted 3 lines

Signed-off-by Manfred Spraul <[email protected]>

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?

Thus the patch is correct, it's a bug in the slab allocator. If HWCACHE_ALIGN

Quoted 1 line

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.

Quoted 9 lines

Thus the patch is correct, it's a bug in the slab allocator. If HWCACHE_ALIGN

Quoted 1 line

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.

Quoted 3 lines

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.

Quoted 4 lines · Christoph Lameter

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 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.

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).

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.

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:

Hi Heiko,

Quoted 8 lines · Heiko Carstens

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.

Quoted 5 lines

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.

Quoted 3 lines · Christoph Lameter

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.

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.

Quoted 5 lines · Pekka J Enberg

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.

Quoted 3 lines · Christoph Lameter

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.

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.

Quoted 5 lines · Christoph Lameter

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.

We intentionally discard the caller mandated alignment for debugging purposes.

And it changes the basic way that slab debugging works.

Quoted 2 lines · Christoph Lameter

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.

Quoted 1 line

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.

Hi Christoph,

Quoted 2 lines · Christoph Lameter

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.

Quoted 3 lines

Quoted 3 lines

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.

Quoted 3 lines

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.

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.