2006-02-24 07:52:47

by Pekka Enberg

[permalink] [raw]
Subject: [PATCH] slab: Don't scan cache_cache

From: Pekka Enberg <[email protected]>

The cache_cache object cache is used for bootstrapping, but the cache is
essentially static. It is unlikely that we ever have more than one page
allocated for it. As SLAB_NO_REAP is gone now, fix a regression by skipping
cache_cache explicitly in cache_reap().

Boot-tested with UML.

Cc: Christoph Lameter <[email protected]>
Cc: Alok Kataria <[email protected]>
Cc: Ravikiran G Thirumalai <[email protected]>
Cc: Manfred Spraul <[email protected]>
Cc: Mark Fasheh <[email protected]>
Signed-off-by: Pekka Enberg <[email protected]>
---

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

Index: 2.6-git/mm/slab.c
===================================================================
--- 2.6-git.orig/mm/slab.c
+++ 2.6-git/mm/slab.c
@@ -3483,6 +3483,10 @@ static void cache_reap(void *unused)
struct slab *slabp;

searchp = list_entry(walk, struct kmem_cache, next);
+
+ if (searchp == &cache_cache)
+ goto next;
+
check_irq_on();

l3 = searchp->nodelists[numa_node_id()];


2006-02-24 16:17:40

by Christoph Lameter

[permalink] [raw]
Subject: Re: [PATCH] slab: Don't scan cache_cache

On Fri, 24 Feb 2006, Pekka J Enberg wrote:

> From: Pekka Enberg <[email protected]>
>
> The cache_cache object cache is used for bootstrapping, but the cache is
> essentially static. It is unlikely that we ever have more than one page
> allocated for it. As SLAB_NO_REAP is gone now, fix a regression by skipping
> cache_cache explicitly in cache_reap().

There are other essentially static caches as well. Could we have something
more general?

Are you really seeing any measurable regression?

2006-02-25 21:50:19

by Pekka Enberg

[permalink] [raw]
Subject: Re: [PATCH] slab: Don't scan cache_cache

On Fri, 24 Feb 2006, Pekka J Enberg wrote:
> > From: Pekka Enberg <[email protected]>
> >
> > The cache_cache object cache is used for bootstrapping, but the cache is
> > essentially static. It is unlikely that we ever have more than one page
> > allocated for it. As SLAB_NO_REAP is gone now, fix a regression by skipping
> > cache_cache explicitly in cache_reap().

On Fri, 2006-02-24 at 08:16 -0800, Christoph Lameter wrote:
> There are other essentially static caches as well. Could we have something
> more general?
>
> Are you really seeing any measurable regression?

I haven't measured it but it seems obvious enough that especially for
low end boxes. I don't think something more general is required because
other static caches should use kmalloc() instead.

Pekka

2006-02-27 02:24:36

by Ravikiran G Thirumalai

[permalink] [raw]
Subject: Re: [PATCH] slab: Don't scan cache_cache

On Sat, Feb 25, 2006 at 11:50:13PM +0200, Pekka Enberg wrote:
> On Fri, 24 Feb 2006, Pekka J Enberg wrote:
> > > From: Pekka Enberg <[email protected]>
> >
> > Are you really seeing any measurable regression?
>
> I haven't measured it but it seems obvious enough that especially for
> low end boxes. I don't think something more general is required because
> other static caches should use kmalloc() instead.
>

I hope all of us mean "less used and not changing in usage over time" when we
refer to static caches all throughout our discussion. That said, for a
given workload, one set of caches maybe static while the other is not
(networking loads may have networking slab caches grow and shrink, while the
fs driver caches stay static etc). So I am not sure if static caches can
use kmalloc in general. If scanning cache_cache is really a regression (it is
probably 1 of 15-20 other static caches on a system), then IMHO, similar
treatment should be given to other static caches as well. We could have a
counter on cachep (per-cpu of course) which keeps tracks of cachep usage,
and we could build logic not to scan these caches as frequently.

Now, it is not like cache_cache cannot grow at all or is absolutely static.
So not scanning just cache_cache, when SLAB_NO_REAP flag is taken out
does not make it look very good IMHO. Sure, it was this way before, but
we had a flag to indicate so. And if we think this is a regression, we
should generalize this for other less used caches too.

Thanks,
Kiran