Re: [PATCH] arm64: Increase the max granular size

Re: [PATCH] arm64: Increase the max granular size

[Catalin Marinas]

(+ linux-mm)

On Tue, Nov 03, 2015 at 05:33:25PM -0600, Christoph Lameter wrote:
> On Tue, 3 Nov 2015, Catalin Marinas wrote:
> > (cc'ing Jonsoo and Christoph; summary: slab failure with L1_CACHE_BYTES
> > of 128 and sizeof(kmem_cache_node) of 152)
> 
> Hmmm... Yes that would mean use the 196 sized kmalloc array which is not a
> power of two slab. But the code looks fine to me.

I'm not entirely sure that gets used (or even created).
kmalloc_index(152) returns 8 (INDEX_NODE==8) since KMALLOC_MIN_SIZE==128
and the "kmalloc-node" cache size is 256.

> > If I revert commit 8fc9cf420b36 ("slab: make more slab management
> > structure off the slab") it works but I still need to figure out how
> > slab indices are calculated. The size_index[] array is overridden so
> > that 0..15 are 7 and 16..23 are 8. But the kmalloc_caches[7] has never
> > been populated, hence the BUG_ON. Another option may be to change
> > kmalloc_size and kmalloc_index to cope with KMALLOC_MIN_SIZE of 128.
> >
> > I'll do some more investigation tomorrow.
> 
> The commit allows off slab management for PAGE_SIZE >> 5 that is 128.

This means that the first kmalloc cache to be created, "kmalloc-128", is
off slab.

> After that commit kmem_cache_create would try to allocate an off slab
> management structure which is not available during early boot.
> But the slab_early_init is set which should prevent the use of an off slab
> management infrastructure in kmem_cache_create().
> 
> However, the failure in line 2283 shows that the OFF SLAB flag was
> mistakenly set anyways!!!! Something must havee cleared slab_early_init?

slab_early_init is cleared after "kmem_cache" and "kmalloc-node" caches
are successfully created. Following this, the minimum kmalloc allocation
goes off-slab when KMALLOC_MIN_SIZE == 128.

When trying to create "kmalloc-128" (via create_kmalloc_caches(),
slab_early_init is already 0), __kmem_cache_create() requires an
allocation of 32 bytes (freelist_size) which has index 7, hence exactly
the kmalloc_caches[7] we are trying to create.

The simplest option would be to make sure that off slab isn't allowed
for caches of KMALLOC_MIN_SIZE or smaller, with the drawback that not
only "kmalloc-128" but any other such caches will be on slab.

I think a better option would be to first check that there is a
kmalloc_caches[] entry for freelist_size before deciding to go off-slab.
See below:

-----8<------------------------------

Re: [PATCH] arm64: Increase the max granular size

[Christoph Lameter]

On Wed, 4 Nov 2015, Catalin Marinas wrote:

> The simplest option would be to make sure that off slab isn't allowed
> for caches of KMALLOC_MIN_SIZE or smaller, with the drawback that not
> only "kmalloc-128" but any other such caches will be on slab.

The reason for an off slab configuration is denser object packing.

> I think a better option would be to first check that there is a
> kmalloc_caches[] entry for freelist_size before deciding to go off-slab.

Hmmm.. Yes seems to be an option.

Maybe we simply revert commit 8fc9cf420b36 instead? That does not seem to
make too much sense to me and the goal of the commit cannot be
accomplished on ARM. Your patch essentially reverts the effect anyways.

Smaller slabs really do not need off slab management anyways since they
will only loose a few objects per slab page.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] arm64: Increase the max granular size

[Catalin Marinas]

On Wed, Nov 04, 2015 at 07:53:50AM -0600, Christoph Lameter wrote:
> On Wed, 4 Nov 2015, Catalin Marinas wrote:
> 
> > The simplest option would be to make sure that off slab isn't allowed
> > for caches of KMALLOC_MIN_SIZE or smaller, with the drawback that not
> > only "kmalloc-128" but any other such caches will be on slab.
> 
> The reason for an off slab configuration is denser object packing.
> 
> > I think a better option would be to first check that there is a
> > kmalloc_caches[] entry for freelist_size before deciding to go off-slab.
> 
> Hmmm.. Yes seems to be an option.
> 
> Maybe we simply revert commit 8fc9cf420b36 instead?

I'm fine with this. Also note that the arm64 commit changing
L1_CACHE_BYTES to 128 hasn't been pushed yet (it's queued for 4.4).

> That does not seem to make too much sense to me and the goal of the
> commit cannot be accomplished on ARM. Your patch essentially reverts
> the effect anyways.

In theory it only reverts the effect for the first kmalloc_cache
("kmalloc-128" in the arm64 case). Any other bigger cache which would
not be mergeable with an existing one still has the potential of
off-slab management.

> Smaller slabs really do not need off slab management anyways since they
> will only loose a few objects per slab page.

IIUC, starting with 128 slab size for a 4KB page, you have 32 objects
per page. The freelist takes 32 bytes (or 31), therefore you waste a
single slab object. However, only 1/4 of it is used for freelist and the
waste gets bigger with 256 slab size, hence the original commit.

BTW, assuming L1_CACHE_BYTES is 512 (I don't ever see this happening but
just in theory), we potentially have the same issue. What would save us
is that INDEX_NODE would match the first "kmalloc-512" cache, so we have
it pre-populated.

-- 
Catalin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] arm64: Increase the max granular size

[Christoph Lameter]

On Wed, 4 Nov 2015, Catalin Marinas wrote:

> BTW, assuming L1_CACHE_BYTES is 512 (I don't ever see this happening but
> just in theory), we potentially have the same issue. What would save us
> is that INDEX_NODE would match the first "kmalloc-512" cache, so we have
> it pre-populated.

Ok maybe add some BUILD_BUG_ONs to ensure that builds fail until we have
addressed that.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] arm64: Increase the max granular size

[Catalin Marinas]

On Wed, Nov 04, 2015 at 09:28:34AM -0600, Christoph Lameter wrote:
> On Wed, 4 Nov 2015, Catalin Marinas wrote:
> 
> > BTW, assuming L1_CACHE_BYTES is 512 (I don't ever see this happening but
> > just in theory), we potentially have the same issue. What would save us
> > is that INDEX_NODE would match the first "kmalloc-512" cache, so we have
> > it pre-populated.
> 
> Ok maybe add some BUILD_BUG_ONs to ensure that builds fail until we have
> addressed that.

A BUILD_BUG_ON should be fine.

Thinking some more, I think if KMALLOC_MIN_SIZE is 128, there is no gain
with off-slab management since the freelist allocation would still be
128 bytes. An alternative to reverting while still having a little
benefit of off-slab for 256 bytes objects (rather than 512 as we would
get with the revert):

diff --git a/mm/slab.c b/mm/slab.c
index 4fcc5dd8d5a6..ac32b4a0f2ec 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2212,8 +2212,8 @@ __kmem_cache_create (struct kmem_cache *cachep, unsigned long flags)
 	 * it too early on. Always use on-slab management when
 	 * SLAB_NOLEAKTRACE to avoid recursive calls into kmemleak)
 	 */
-	if ((size >= (PAGE_SIZE >> 5)) && !slab_early_init &&
-	    !(flags & SLAB_NOLEAKTRACE))
+	if ((size >= (PAGE_SIZE >> 5)) && (size > KMALLOC_MIN_SIZE) &&
+		!slab_early_init && !(flags & SLAB_NOLEAKTRACE))
 		/*
 		 * Size is large, assume best to place the slab management obj
 		 * off-slab (should allow better packing of objs).

Whichever you prefer.

-- 
Catalin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] arm64: Increase the max granular size

[Joonsoo Kim]

On Wed, Nov 04, 2015 at 03:39:10PM +0000, Catalin Marinas wrote:
> On Wed, Nov 04, 2015 at 09:28:34AM -0600, Christoph Lameter wrote:
> > On Wed, 4 Nov 2015, Catalin Marinas wrote:
> > 
> > > BTW, assuming L1_CACHE_BYTES is 512 (I don't ever see this happening but
> > > just in theory), we potentially have the same issue. What would save us
> > > is that INDEX_NODE would match the first "kmalloc-512" cache, so we have
> > > it pre-populated.
> > 
> > Ok maybe add some BUILD_BUG_ONs to ensure that builds fail until we have
> > addressed that.
> 
> A BUILD_BUG_ON should be fine.
> 
> Thinking some more, I think if KMALLOC_MIN_SIZE is 128, there is no gain
> with off-slab management since the freelist allocation would still be
> 128 bytes. An alternative to reverting while still having a little
> benefit of off-slab for 256 bytes objects (rather than 512 as we would
> get with the revert):
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index 4fcc5dd8d5a6..ac32b4a0f2ec 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2212,8 +2212,8 @@ __kmem_cache_create (struct kmem_cache *cachep, unsigned long flags)
>  	 * it too early on. Always use on-slab management when
>  	 * SLAB_NOLEAKTRACE to avoid recursive calls into kmemleak)
>  	 */
> -	if ((size >= (PAGE_SIZE >> 5)) && !slab_early_init &&
> -	    !(flags & SLAB_NOLEAKTRACE))
> +	if ((size >= (PAGE_SIZE >> 5)) && (size > KMALLOC_MIN_SIZE) &&
> +		!slab_early_init && !(flags & SLAB_NOLEAKTRACE))
>  		/*
>  		 * Size is large, assume best to place the slab management obj
>  		 * off-slab (should allow better packing of objs).
> 
> Whichever you prefer.

Hello,

I prefer this simple way. It looks like that it can solve the issue on
any arbitrary configuration.

Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] mm: slab: Only move management objects off-slab for sizes larger than KMALLOC_MIN_SIZE

[Catalin Marinas]

Commit 8fc9cf420b36 ("slab: make more slab management structure off the
slab") enables off-slab management objects for sizes starting with
PAGE_SIZE >> 5. This means 128 bytes for a 4KB page configuration.
However, on systems with a KMALLOC_MIN_SIZE of 128 (arm64 in 4.4), such
optimisation does not make sense since the slab management allocation
would take 128 bytes anyway (even though freelist_size is 32) with the
additional overhead of another allocation.

This patch introduces an OFF_SLAB_MIN_SIZE macro which takes
KMALLOC_MIN_SIZE into account. It also solves a slab bug on arm64 where
the first kmalloc_cache to be initialised after slab_early_init = 0,
"kmalloc-128", fails to allocate off-slab management objects from the
same "kmalloc-128" cache.

Fixes: 8fc9cf420b36 ("slab: make more slab management structure off the slab")
Cc: <stable@vger.kernel.org> # 3.15+
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Christoph Lameter <cl@linux.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 mm/slab.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 4fcc5dd8d5a6..419b649b395e 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -282,6 +282,7 @@ static void kmem_cache_node_init(struct kmem_cache_node *parent)
 
 #define CFLGS_OFF_SLAB		(0x80000000UL)
 #define	OFF_SLAB(x)	((x)->flags & CFLGS_OFF_SLAB)
+#define OFF_SLAB_MIN_SIZE	(max_t(size_t, PAGE_SIZE >> 5, KMALLOC_MIN_SIZE + 1))
 
 #define BATCHREFILL_LIMIT	16
 /*
@@ -2212,7 +2213,7 @@ __kmem_cache_create (struct kmem_cache *cachep, unsigned long flags)
 	 * it too early on. Always use on-slab management when
 	 * SLAB_NOLEAKTRACE to avoid recursive calls into kmemleak)
 	 */
-	if ((size >= (PAGE_SIZE >> 5)) && !slab_early_init &&
+	if (size >= OFF_SLAB_MIN_SIZE && !slab_early_init &&
 	    !(flags & SLAB_NOLEAKTRACE))
 		/*
 		 * Size is large, assume best to place the slab management obj
@@ -2276,7 +2277,7 @@ __kmem_cache_create (struct kmem_cache *cachep, unsigned long flags)
 		/*
 		 * This is a possibility for one of the kmalloc_{dma,}_caches.
 		 * But since we go off slab only for object size greater than
-		 * PAGE_SIZE/8, and kmalloc_{dma,}_caches get created
+		 * OFF_SLAB_MIN_SIZE, and kmalloc_{dma,}_caches get created
 		 * in ascending order,this should not happen at all.
 		 * But leave a BUG_ON for some lucky dude.
 		 */

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: slab: Only move management objects off-slab for sizes larger than KMALLOC_MIN_SIZE

[Andrew Morton]

On Thu,  5 Nov 2015 11:50:35 +0000 Catalin Marinas <catalin.marinas@arm.com> wrote:

> Commit 8fc9cf420b36 ("slab: make more slab management structure off the
> slab") enables off-slab management objects for sizes starting with
> PAGE_SIZE >> 5. This means 128 bytes for a 4KB page configuration.
> However, on systems with a KMALLOC_MIN_SIZE of 128 (arm64 in 4.4), such
> optimisation does not make sense since the slab management allocation
> would take 128 bytes anyway (even though freelist_size is 32) with the
> additional overhead of another allocation.
> 
> This patch introduces an OFF_SLAB_MIN_SIZE macro which takes
> KMALLOC_MIN_SIZE into account. It also solves a slab bug on arm64 where
> the first kmalloc_cache to be initialised after slab_early_init = 0,
> "kmalloc-128", fails to allocate off-slab management objects from the
> same "kmalloc-128" cache.

That all seems to be quite minor stuff.

> Fixes: 8fc9cf420b36 ("slab: make more slab management structure off the slab")
> Cc: <stable@vger.kernel.org> # 3.15+

Yet you believe the fix should be backported.

So, the usual refrain: when fixing a bug, please describe the end-user
visible effects of that bug.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: slab: Only move management objects off-slab for sizes larger than KMALLOC_MIN_SIZE

[Catalin Marinas]

On Thu, Nov 05, 2015 at 05:31:39AM -0800, Andrew Morton wrote:
> On Thu,  5 Nov 2015 11:50:35 +0000 Catalin Marinas <catalin.marinas@arm.com> wrote:
> 
> > Commit 8fc9cf420b36 ("slab: make more slab management structure off the
> > slab") enables off-slab management objects for sizes starting with
> > PAGE_SIZE >> 5. This means 128 bytes for a 4KB page configuration.
> > However, on systems with a KMALLOC_MIN_SIZE of 128 (arm64 in 4.4), such
> > optimisation does not make sense since the slab management allocation
> > would take 128 bytes anyway (even though freelist_size is 32) with the
> > additional overhead of another allocation.
> > 
> > This patch introduces an OFF_SLAB_MIN_SIZE macro which takes
> > KMALLOC_MIN_SIZE into account. It also solves a slab bug on arm64 where
> > the first kmalloc_cache to be initialised after slab_early_init = 0,
> > "kmalloc-128", fails to allocate off-slab management objects from the
> > same "kmalloc-128" cache.
> 
> That all seems to be quite minor stuff.

Apart from "it also solves a bug on arm64...". But I agree, the initial
commit log doesn't give any justification for cc stable.

> > Fixes: 8fc9cf420b36 ("slab: make more slab management structure off the slab")
> > Cc: <stable@vger.kernel.org> # 3.15+
> 
> Yet you believe the fix should be backported.
> 
> So, the usual refrain: when fixing a bug, please describe the end-user
> visible effects of that bug.

What about (unless you prefer this slightly more intrusive fix:
http://article.gmane.org/gmane.linux.ports.sh.devel/50303):

------------------8<--------------------------

Re: [PATCH] mm: slab: Only move management objects off-slab for sizes larger than KMALLOC_MIN_SIZE

[Geert Uytterhoeven]

On Thu, Nov 5, 2015 at 5:08 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> From fda8f306b6941f4ddbefcbcfaa59fedef4a679a3 Mon Sep 17 00:00:00 2001
> From: Catalin Marinas <catalin.marinas@arm.com>
> Date: Thu, 5 Nov 2015 11:14:48 +0000
> Subject: [PATCH] mm: slab: Only move management objects off-slab for sizes
>  larger than KMALLOC_MIN_SIZE
>
> On systems with a KMALLOC_MIN_SIZE of 128 (arm64, some mips and powerpc
> configurations defining ARCH_DMA_MINALIGN to 128), the first
> kmalloc_caches[] entry to be initialised after slab_early_init = 0 is
> "kmalloc-128" with index 7. Depending on the debug kernel configuration,
> sizeof(struct kmem_cache) can be larger than 128 resulting in an
> INDEX_NODE of 8.
>
> Commit 8fc9cf420b36 ("slab: make more slab management structure off the
> slab") enables off-slab management objects for sizes starting with
> PAGE_SIZE >> 5 (128 bytes for a 4KB page configuration) and the creation
> of the "kmalloc-128" cache would try to place the management objects
> off-slab. However, since KMALLOC_MIN_SIZE is already 128 and
> freelist_size == 32 in __kmem_cache_create(),
> kmalloc_slab(freelist_size) returns NULL (kmalloc_caches[7] not
> populated yet). This triggers the following bug on arm64:
>
> [    0.000000] kernel BUG at /work/Linux/linux-2.6-aarch64/mm/slab.c:2283!
> [    0.000000] Internal error: Oops - BUG: 0 [#1] SMP
> [    0.000000] Modules linked in:
> [    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.3.0-rc4+ #540
> [    0.000000] Hardware name: Juno (DT)
> [    0.000000] task: ffffffc0006962b0 ti: ffffffc00068c000 task.ti: ffffffc00068c000
> [    0.000000] PC is at __kmem_cache_create+0x21c/0x280
> [    0.000000] LR is at __kmem_cache_create+0x210/0x280
> [...]
> [    0.000000] Call trace:
> [    0.000000] [<ffffffc000154948>] __kmem_cache_create+0x21c/0x280
> [    0.000000] [<ffffffc000652da4>] create_boot_cache+0x48/0x80
> [    0.000000] [<ffffffc000652e2c>] create_kmalloc_cache+0x50/0x88
> [    0.000000] [<ffffffc000652f14>] create_kmalloc_caches+0x4c/0xf4
> [    0.000000] [<ffffffc000654a9c>] kmem_cache_init+0x100/0x118
> [    0.000000] [<ffffffc0006447d4>] start_kernel+0x214/0x33c
>
> This patch introduces an OFF_SLAB_MIN_SIZE definition to avoid off-slab
> management objects for sizes equal to or smaller than KMALLOC_MIN_SIZE.
>
>
> Fixes: 8fc9cf420b36 ("slab: make more slab management structure off the slab")
> Cc: <stable@vger.kernel.org> # 3.15+
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks a lot!

For the record (the fix is already upstream):

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: slab: Only move management objects off-slab for sizes larger than KMALLOC_MIN_SIZE

[Christoph Lameter]

On Thu, 5 Nov 2015, Catalin Marinas wrote:

> This patch introduces an OFF_SLAB_MIN_SIZE macro which takes
> KMALLOC_MIN_SIZE into account. It also solves a slab bug on arm64 where
> the first kmalloc_cache to be initialised after slab_early_init = 0,
> "kmalloc-128", fails to allocate off-slab management objects from the
> same "kmalloc-128" cache.

Acked-by: Christoph Lameter <cl@linux.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>