Re: [PATCH] slab: fix slab flags for archs use alignment larger 64-bit

Re: [PATCH] slab: fix slab flags for archs use alignment larger 64-bit

[Paul Mundt]

On Thu, Feb 12, 2009 at 06:51:13PM +0100, Giuseppe CAVALLARO wrote:
> I think, this fix is necessary for all the architectures want to
> perform DMA into kmalloc caches and need a guaranteed alignment
> larger than the alignment of a 64-bit integer.
> An example is sh architecture where ARCH_KMALLOC_MINALIGN is L1_CACHE_BYTES.
> 
> As side effect, these kind of objects cannot be visible
> within the /proc/slab_allocators file.
> 
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>  mm/slab.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index ddc41f3..031d785 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2262,7 +2262,7 @@ kmem_cache_create (const char *name, size_t size, size_t align,
>  		ralign = align;
>  	}
>  	/* disable debug if necessary */
> -	if (ralign > __alignof__(unsigned long long))
> +	if (ralign > ARCH_KMALLOC_MINALIGN)
>  		flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
>  	/*
>  	 * 4) Store it.

No, this change in itself is not sufficient. The redzone marker placement
as well as that of the user store need to know about the minalign as well
before slab debug can work correctly.

I last looked at this when introducing ARCH_SLAB_MINALIGN:

http://article.gmane.org/gmane.linux.kernel/262528

But it would need some rework for the current slab code.

Note that the ARCH_KMALLOC_MINALIGN value has no meaning here, as this
relates to slab caches in general, of which kmalloc just happens to have
a few. This is also why the rest of the kmem_cache_create() code
references ARCH_SLAB_MINALIGN in the first place. But that in itself is
irrelevant since for the kmalloc slab caches, ARCH_KMALLOC_MINALIGN is
already passed in as the align value for kmem_cache_create(), so ralign
is already set to L1_CACHE_BYTES immediately before that check.

What exactly are you having problems with that made you come up with this
patch? It would be helpful to know precisely what your issues are, as
this change in itself is only related to slab debug, and not general
operation.

Re: [PATCH] slab: fix slab flags for archs use alignment larger 64-bit

[Giuseppe CAVALLARO]

Paul Mundt wrote:
> No, this change in itself is not sufficient. The redzone marker placement
> as well as that of the user store need to know about the minalign as well
> before slab debug can work correctly.
>
> I last looked at this when introducing ARCH_SLAB_MINALIGN:
>
> http://article.gmane.org/gmane.linux.kernel/262528
>
> But it would need some rework for the current slab code.
>
> Note that the ARCH_KMALLOC_MINALIGN value has no meaning here, as this
> relates to slab caches in general, of which kmalloc just happens to have
> a few. This is also why the rest of the kmem_cache_create() code
> references ARCH_SLAB_MINALIGN in the first place. But that in itself is
> irrelevant since for the kmalloc slab caches, ARCH_KMALLOC_MINALIGN is
> already passed in as the align value for kmem_cache_create(), so ralign
> is already set to L1_CACHE_BYTES immediately before that check.
>
> What exactly are you having problems with that made you come up with this
> patch? It would be helpful to know precisely what your issues are, as
> this change in itself is only related to slab debug, and not general
> operation
Thanks for your feedback.

This patch is only to fix the debug information reported in
/proc/slab_allocators .

On SH, I've noticed that /proc/slab_allocators has no size-X entries.
I guess, we should find these fields, shouldn't we?

IIUC, and as you explained above, ralign is already set to the cache
line size by the following code:
...
/* 3) caller mandated alignment */
if (ralign < align)
ralign = align;

Then, there is following check:
...
/* disable debug if necessary */
if (ralign > _alignof__(unsigned long long))
flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);

In my point of view, just this check appears "incoherent" (please, note
I'm not familiar with the slab internals).
It always makes sense in case of x86 where ARCH_KMALLOC_MINALIGN is
defined as: __alignof__(unsigned long long) as well.
In case of sh, we always disable debug for 32 aligned objects. As side
effect, within the leaks_show function we immediately exit for them.
Indeed, after applying the patch, I attached, I was able to find size-X
fields within the slab_allocators proc entry.

Re: [PATCH] slab: fix slab flags for archs use alignment larger

[Pekka Enberg]

Hi Giuseppe,

On Fri, Feb 13, 2009 at 12:00 PM, Giuseppe CAVALLARO
<peppe.cavallaro@st.com> wrote:
> IIUC, and as you explained above, ralign is already set to the cache
> line size by the following code:
> ...
> /* 3) caller mandated alignment */
> if (ralign < align)
> ralign = align;
>
> Then, there is following check:
> ...
> /* disable debug if necessary */
> if (ralign > _alignof__(unsigned long long))
> flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
>
> In my point of view, just this check appears "incoherent" (please, note
> I'm not familiar with the slab internals).
> It always makes sense in case of x86 where ARCH_KMALLOC_MINALIGN is
> defined as: __alignof__(unsigned long long) as well.

The code is indeed quite cryptic but I don't think the above statement
is correct. Both SLAB_RED_ZONE and SLAB_STORE_USER add padding in
front of an object and the maximum size of that padding is 2 *
sizeof(unsigned long long). That's why we check for
__alignof__(unsigned long) there to make sure the object can begin
immediately after the padding added for red-zone and user pointer.

On Fri, Feb 13, 2009 at 12:00 PM, Giuseppe CAVALLARO
<peppe.cavallaro@st.com> wrote:
> In case of sh, we always disable debug for 32 aligned objects. As side
> effect, within the leaks_show function we immediately exit for them.
> Indeed, after applying the patch, I attached, I was able to find size-X
> fields within the slab_allocators proc entry.

That sounds unfortunate. Can you post

  cat /proc/meminfo | grep Slab

results on sh without and with your patch? Bumping the limit up to
ARCH_KMALLOC_MINALIGN does make sense but we'd need to know what kind
of problems it might cause.

                                Pekka

Re: [PATCH] slab: fix slab flags for archs use alignment larger

[Pekka Enberg]

On Fri, Feb 13, 2009 at 11:22 AM, Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> That sounds unfortunate. Can you post
>
>  cat /proc/meminfo | grep Slab
>
> results on sh without and with your patch? Bumping the limit up to
> ARCH_KMALLOC_MINALIGN does make sense but we'd need to know what kind
> of problems it might cause.

You probably want to fix up the issues Paul raised, though.

Re: [PATCH] slab: fix slab flags for archs use alignment larger

[Giuseppe CAVALLARO]

Hi Pekka,
thanks for your prompt feedback.
Feel free to ask again if you need some other information.

Peppe

Kernel 2.6.23.17
======================1) LOG with my patch:
root@linux:~# cat /proc/meminfo | grep Slab
Slab:             2612 kB

root@linux:~# cat /proc/slab_allocators
rpc_buffers: 8 mempool_alloc_slab+0x10/0x1c
rpc_tasks: 11 mempool_alloc_slab+0x10/0x1c
rpc_inode_cache: 10 rpc_alloc_inode+0x10/0x2c
...
anon_vma: 62 anon_vma_prepare+0x38/0xd4
pid: 20 alloc_pid+0x1e/0x2cc
size-2048: 1 tty_write+0xac/0x1d8
size-2048: 1 alloc_netdev_mq+0x40/0xbc
...

======================2) LOG without my patch:
root@linux:~# cat /proc/meminfo | grep Slab
Slab:             2504 kB

root@linux:~# cat /proc/slab_allocators
rpc_buffers: 8 mempool_alloc_slab+0x10/0x1c
rpc_tasks: 11 mempool_alloc_slab+0x10/0x1c
rpc_inode_cache: 10 rpc_alloc_inode+0x10/0x2c
...
pid: 20 alloc_pid+0x1e/0x2cc

Pekka Enberg wrote:
> Hi Giuseppe,
>
> On Fri, Feb 13, 2009 at 12:00 PM, Giuseppe CAVALLARO
> <peppe.cavallaro@st.com> wrote:
>   
>> IIUC, and as you explained above, ralign is already set to the cache
>> line size by the following code:
>> ...
>> /* 3) caller mandated alignment */
>> if (ralign < align)
>> ralign = align;
>>
>> Then, there is following check:
>> ...
>> /* disable debug if necessary */
>> if (ralign > _alignof__(unsigned long long))
>> flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
>>
>> In my point of view, just this check appears "incoherent" (please, note
>> I'm not familiar with the slab internals).
>> It always makes sense in case of x86 where ARCH_KMALLOC_MINALIGN is
>> defined as: __alignof__(unsigned long long) as well.
>>     
>
> The code is indeed quite cryptic but I don't think the above statement
> is correct. Both SLAB_RED_ZONE and SLAB_STORE_USER add padding in
> front of an object and the maximum size of that padding is 2 *
> sizeof(unsigned long long). That's why we check for
> __alignof__(unsigned long) there to make sure the object can begin
> immediately after the padding added for red-zone and user pointer.
>
> On Fri, Feb 13, 2009 at 12:00 PM, Giuseppe CAVALLARO
> <peppe.cavallaro@st.com> wrote:
>   
>> In case of sh, we always disable debug for 32 aligned objects. As side
>> effect, within the leaks_show function we immediately exit for them.
>> Indeed, after applying the patch, I attached, I was able to find size-X
>> fields within the slab_allocators proc entry.
>>     
>
> That sounds unfortunate. Can you post
>
>   cat /proc/meminfo | grep Slab
>
> results on sh without and with your patch? Bumping the limit up to
> ARCH_KMALLOC_MINALIGN does make sense but we'd need to know what kind
> of problems it might cause.
>
>                                 Pekka
>
>   

Re: [PATCH] slab: fix slab flags for archs use alignment larger

[Pekka Enberg]

Hi Giuseppe,

On Fri, Feb 13, 2009 at 12:46 PM, Giuseppe CAVALLARO
<peppe.cavallaro@st.com> wrote:
> 1) LOG with my patch:
> root@linux:~# cat /proc/meminfo | grep Slab
> Slab:             2612 kB
>
> 2) LOG without my patch:
> root@linux:~# cat /proc/meminfo | grep Slab
> Slab:             2504 kB

That's not too bad. I assume it's L1_CACHE_BYTES set to 32 bytes? One
big problem with your patch is that on some MIPS configurations
ARCH_KMALLOC_MINALIGN is as big as 128. So if you're going to do this,
you can't use ARCH_KMALLOC_MINALIGN directly but add a some
SLAB_MAX_DEBUG_ALIGN which can be overridden by architecture code.

One obvious question, though, is whether all this is worth the added
complexity. I mean, we've managed "just fine" without it for years.
Paul, thoughts?

                                        Pekka

Re: [PATCH] slab: fix slab flags for archs use alignment larger

[Giuseppe CAVALLARO]

Pekka Enberg wrote:
> Hi Giuseppe,
>
> On Fri, Feb 13, 2009 at 12:46 PM, Giuseppe CAVALLARO
> <peppe.cavallaro@st.com> wrote:
>   
>> 1) LOG with my patch:
>> root@linux:~# cat /proc/meminfo | grep Slab
>> Slab:             2612 kB
>>
>> 2) LOG without my patch:
>> root@linux:~# cat /proc/meminfo | grep Slab
>> Slab:             2504 kB
>>     
>
> That's not too bad. I assume it's L1_CACHE_BYTES set to 32 bytes? 
you are perfectly right.
> One big problem with your patch is that on some MIPS configurations
> ARCH_KMALLOC_MINALIGN is as big as 128.
Agree again and problem understood... thanks!
> So if you're going to do this, you can't use ARCH_KMALLOC_MINALIGN directly but add a some
> SLAB_MAX_DEBUG_ALIGN which can be overridden by architecture code.
>   
If you like, I can prepare a patch in any case.
Then we can decide if it actually adds complexity and discard it.
Otherwise we could maintain it.
> One obvious question, though, is whether all this is worth the added
> complexity. I mean, we've managed "just fine" without it for years.
> Paul, thoughts?
>
>                                         Pekka
>
>   

Re: [PATCH] slab: fix slab flags for archs use alignment larger

[Giuseppe CAVALLARO]

[-- Attachment #1: Type: text/plain, Size: 1541 bytes --]

Giuseppe CAVALLARO wrote:
> Pekka Enberg wrote:
>   
>> Hi Giuseppe,
>>
>> On Fri, Feb 13, 2009 at 12:46 PM, Giuseppe CAVALLARO
>> <peppe.cavallaro@st.com> wrote:
>>   
>>     
>>> 1) LOG with my patch:
>>> root@linux:~# cat /proc/meminfo | grep Slab
>>> Slab:             2612 kB
>>>
>>> 2) LOG without my patch:
>>> root@linux:~# cat /proc/meminfo | grep Slab
>>> Slab:             2504 kB
>>>     
>>>       
>> That's not too bad. I assume it's L1_CACHE_BYTES set to 32 bytes? 
>>     
> you are perfectly right.
>   
>> One big problem with your patch is that on some MIPS configurations
>> ARCH_KMALLOC_MINALIGN is as big as 128.
>>     
> Agree again and problem understood... thanks!
>   
>> So if you're going to do this, you can't use ARCH_KMALLOC_MINALIGN directly but add a some
>> SLAB_MAX_DEBUG_ALIGN which can be overridden by architecture code.
>>   
>>     
> If you like, I can prepare a patch in any case.
> Then we can decide if it actually adds complexity and discard it.
> Otherwise we could maintain it.
>   
>> One obvious question, though, is whether all this is worth the added
>> complexity. I mean, we've managed "just fine" without it for years.
>> Paul, thoughts?
>>
>>                                         Pekka
>>   
>>     
Hi!
as promised, patches attached.
At any rate, (this is just my feeling) although we add more complexity
this kind of 'fixes' can actually help users to debug.
So, complexity is welcome especially in these scenarios (debug on, users
in panic mode :-)).
Let me know.
Cheers,
Peppe

[-- Attachment #2: 0001-slab-fix-the-debug-information-with-different-align.patch --]
[-- Type: text/plain, Size: 1339 bytes --]

From 86c7fe861b4a6ee7b533a25ce2c506134a7533a8 Mon Sep 17 00:00:00 2001
From: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Date: Fri, 13 Feb 2009 15:06:22 +0100
Subject: [PATCH] slab: fix the debug information with different alignments.

This patch is to fix the debug information reported in /proc/slab_allocators

for all the architectures by using a new define (ARCH_SLAB_MAX_DEBUG_ALIGN)
rather than the alignment of a 64-bit integer.
Currently, on sh, for example, /proc/slab_allocators has no size-X entries
as side effect.

Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 mm/slab.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 4d00855..587e0f3 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -165,6 +165,10 @@
 #define ARCH_SLAB_MINALIGN 0
 #endif
 
+#ifndef ARCH_SLAB_MAX_DEBUG_ALIGN
+#define ARCH_SLAB_MAX_DEBUG_ALIGN __alignof__(unsigned long long)
+#endif
+
 #ifndef ARCH_KMALLOC_FLAGS
 #define ARCH_KMALLOC_FLAGS SLAB_HWCACHE_ALIGN
 #endif
@@ -2262,7 +2266,7 @@ kmem_cache_create (const char *name, size_t size, size_t align,
 		ralign = align;
 	}
 	/* disable debug if necessary */
-	if (ralign > __alignof__(unsigned long long))
+	if (ralign > ARCH_SLAB_MAX_DEBUG_ALIGN)
 		flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
 	/*
 	 * 4) Store it.
-- 
1.5.6.6


[-- Attachment #3: 0002-sh-add-the-default-value-for-ARCH_SLAB_MAX_DEBUG_AL.patch --]
[-- Type: text/plain, Size: 812 bytes --]

From 3b461766df1841f03e740b91efc38132eae74f86 Mon Sep 17 00:00:00 2001
From: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Date: Fri, 13 Feb 2009 15:06:39 +0100
Subject: [PATCH] sh: add the default value for ARCH_SLAB_MAX_DEBUG_ALIGN define.

Add and enforce ARCH_SLAB_MAX_DEBUG_ALIGN to the cache line size.

Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 arch/sh/include/asm/page.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/sh/include/asm/page.h b/arch/sh/include/asm/page.h
index 5871d78..14768c8 100644
--- a/arch/sh/include/asm/page.h
+++ b/arch/sh/include/asm/page.h
@@ -182,4 +182,6 @@ typedef struct page *pgtable_t;
 #define ARCH_SLAB_MINALIGN	8
 #endif
 
+#define ARCH_SLAB_MAX_DEBUG_ALIGN L1_CACHE_BYTES
+
 #endif /* __ASM_SH_PAGE_H */
-- 
1.5.6.6

Re: [PATCH] slab: fix slab flags for archs use alignment larger

[Pekka Enberg]

Hi Giuseppe,

On Fri, Feb 13, 2009 at 4:11 PM, Giuseppe CAVALLARO
<peppe.cavallaro@st.com> wrote:
> @@ -2262,7 +2266,7 @@ kmem_cache_create (const char *name, size_t size, size_t align,
>                ralign = align;
>        }
>        /* disable debug if necessary */
> -       if (ralign > __alignof__(unsigned long long))
> +       if (ralign > ARCH_SLAB_MAX_DEBUG_ALIGN)
>                flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
>        /*
>         * 4) Store it.

This doesn't address the issues raised by Paul at all.

                                Pekka

Re: [PATCH] slab: fix slab flags for archs use alignment larger

[Giuseppe CAVALLARO]

Hi Pekka,
Pekka Enberg wrote:
> Hi Giuseppe,
>
> On Fri, Feb 13, 2009 at 4:11 PM, Giuseppe CAVALLARO
> <peppe.cavallaro@st.com> wrote:
>   
>> @@ -2262,7 +2266,7 @@ kmem_cache_create (const char *name, size_t size, size_t align,
>>                ralign = align;
>>        }
>>        /* disable debug if necessary */
>> -       if (ralign > __alignof__(unsigned long long))
>> +       if (ralign > ARCH_SLAB_MAX_DEBUG_ALIGN)
>>                flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
>>        /*
>>         * 4) Store it.
>>     
>
> This doesn't address the issues raised by Paul at all.
>
>                                 Pekka
>   
Sorry for my late reply.
You are right.
I'm going to review this but this could take a while.
I'll go back to you as soon as possible.
Regards,
Peppe
//