Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Linux: Kernel

[PATCH v2] slab+slob: dup name string

 

 

Linux kernel RSS feed   Index | Next | Previous | View Threaded


glommer at parallels

May 22, 2012, 2:51 AM

Post #1 of 8 (117 views)
Permalink
[PATCH v2] slab+slob: dup name string

The slub allocator creates a copy of the name string, and
frees it later. I would like all caches to behave the same,
whether it is the slab+slob starting to create a copy of it itself,
or the slub ceasing to.

This patch creates copies of the name string for slob and slab,
adopting slub behavior for them all.

For the slab, we can't really do it before the kmalloc caches are
up. So we manually do it before the end of EARLY phase, and conditionally
do it for the caches created afterwards.

[. v2: Also dup string for early caches, requested by David Rientjes ]

Signed-off-by: Glauber Costa <glommer [at] parallels>
CC: Christoph Lameter <cl [at] linux>
CC: Pekka Enberg <penberg [at] cs>
CC: David Rientjes <rientjes [at] google>
---
mm/slab.c | 37 +++++++++++++++++++++++++++++++++++--
mm/slob.c | 12 ++++++++++--
2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index e901a36..fe05f8bf 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1676,6 +1676,33 @@ void __init kmem_cache_init(void)
}
}

+ /*
+ * create a copy of all the name strings for early caches. This is
+ * so deleting those caches will work in a consistent way. We don't
+ * expect allocation failures this early in the process, just make sure
+ * they didn't happen.
+ */
+ sizes = malloc_sizes;
+
+ while (sizes->cs_size != ULONG_MAX) {
+ struct kmem_cache *cachep;
+
+ cachep = sizes->cs_cachep;
+ if (cachep) {
+ cachep->name = kstrdup(cachep->name, GFP_NOWAIT);
+ BUG_ON(!cachep->name);
+ }
+
+ cachep = sizes->cs_dmacachep;
+ if (cachep) {
+ cachep->name = kstrdup(cachep->name, GFP_NOWAIT);
+ BUG_ON(!cachep->name);
+ }
+ sizes++;
+ }
+
+ cache_cache.name = kstrdup(cache_cache.name, GFP_NOWAIT);
+ BUG_ON(!cache_cache.name);
g_cpucache_up = EARLY;
}

@@ -2118,6 +2145,7 @@ static void __kmem_cache_destroy(struct kmem_cache *cachep)
kfree(l3);
}
}
+ kfree(cachep->name);
kmem_cache_free(&cache_cache, cachep);
}

@@ -2526,9 +2554,14 @@ kmem_cache_create (const char *name, size_t size, size_t align,
BUG_ON(ZERO_OR_NULL_PTR(cachep->slabp_cache));
}
cachep->ctor = ctor;
- cachep->name = name;

- if (setup_cpu_cache(cachep, gfp)) {
+ /* Can't do strdup while kmalloc is not up */
+ if (slab_is_available())
+ cachep->name = kstrdup(name, GFP_KERNEL);
+ else
+ cachep->name = name;
+
+ if (!cachep->name || setup_cpu_cache(cachep, gfp)) {
__kmem_cache_destroy(cachep);
cachep = NULL;
goto oops;
diff --git a/mm/slob.c b/mm/slob.c
index 8105be4..8f10d36 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -575,7 +575,12 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size,
GFP_KERNEL, ARCH_KMALLOC_MINALIGN, -1);

if (c) {
- c->name = name;
+ c->name = kstrdup(name, GFP_KERNEL);
+ if (!c->name) {
+ slob_free(c, sizeof(struct kmem_cache));
+ c = NULL;
+ goto out;
+ }
c->size = size;
if (flags & SLAB_DESTROY_BY_RCU) {
/* leave room for rcu footer at the end of object */
@@ -589,7 +594,9 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size,
c->align = ARCH_SLAB_MINALIGN;
if (c->align < align)
c->align = align;
- } else if (flags & SLAB_PANIC)
+ }
+out:
+ if (!c && (flags & SLAB_PANIC))
panic("Cannot create slab cache %s\n", name);

kmemleak_alloc(c, sizeof(struct kmem_cache), 1, GFP_KERNEL);
@@ -602,6 +609,7 @@ void kmem_cache_destroy(struct kmem_cache *c)
kmemleak_free(c);
if (c->flags & SLAB_DESTROY_BY_RCU)
rcu_barrier();
+ kfree(c->name);
slob_free(c, sizeof(struct kmem_cache));
}
EXPORT_SYMBOL(kmem_cache_destroy);
--
1.7.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


cl at linux

May 22, 2012, 6:58 AM

Post #2 of 8 (118 views)
Permalink
Re: [PATCH v2] slab+slob: dup name string [In reply to]

On Tue, 22 May 2012, Glauber Costa wrote:

> [. v2: Also dup string for early caches, requested by David Rientjes ]

kstrdups that early could cause additional issues. Its better to leave
things as they were.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


glommer at parallels

May 22, 2012, 8:27 AM

Post #3 of 8 (114 views)
Permalink
Re: [PATCH v2] slab+slob: dup name string [In reply to]

On 05/22/2012 05:58 PM, Christoph Lameter wrote:
> On Tue, 22 May 2012, Glauber Costa wrote:
>
>> [. v2: Also dup string for early caches, requested by David Rientjes ]
>
> kstrdups that early could cause additional issues. Its better to leave
> things as they were.
>

For me is really all the same. But note that before those kstrdups, we
do a bunch of kmallocs as well already. (ex:

/* 4) Replace the bootstrap head arrays */
{
struct array_cache *ptr;

ptr = kmalloc(sizeof(struct arraycache_init), GFP_NOWAIT);

Which other point of issues do you see besides the memory allocation
done by strdup?

I agree with your comment that we shouldn't worry about those caches,
because only init code uses it.

Weather or not David's concern of wanting to delete those caches some
day is valid, I'll leave up to you guys to decide

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


cl at linux

May 22, 2012, 10:18 AM

Post #4 of 8 (114 views)
Permalink
Re: [PATCH v2] slab+slob: dup name string [In reply to]

On Tue, 22 May 2012, Glauber Costa wrote:

> On 05/22/2012 05:58 PM, Christoph Lameter wrote:
> > On Tue, 22 May 2012, Glauber Costa wrote:
> >
> > > [. v2: Also dup string for early caches, requested by David Rientjes ]
> >
> > kstrdups that early could cause additional issues. Its better to leave
> > things as they were.
> >
>
> For me is really all the same. But note that before those kstrdups, we do a
> bunch of kmallocs as well already. (ex:

We check carefully and make sure those caches are present before doing
these kmallocs. See the slab bootup code. A kstrdup may take a variously
sized string and explode because the required kmalloc cache is not
present.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


rientjes at google

May 22, 2012, 8:55 PM

Post #5 of 8 (114 views)
Permalink
Re: [PATCH v2] slab+slob: dup name string [In reply to]

On Tue, 22 May 2012, Christoph Lameter wrote:

> > [. v2: Also dup string for early caches, requested by David Rientjes ]
>
> kstrdups that early could cause additional issues. Its better to leave
> things as they were.
>

No, it's not, there's no reason to prevent caches created before
g_cpucache_up <= EARLY to be destroyed because it makes a patch easier to
implement and then leave that little gotcha as an undocumented treasure
for someone to find when they try it later on.

I hate consistency patches like this because it could potentially fail a
kmem_cache_create() from a sufficiently long cache name when it wouldn't
have before, but I'm not really concerned since kmem_cache_create() will
naturally be followed by kmem_cache_alloc() which is more likely to cause
the oom anyway. But it's just another waste of memory for consistency
sake.

This is much easier to do, just statically allocate the const char *'s
needed for the boot caches and then set their ->name's manually in
kmem_cache_init() and then avoid the kfree() in kmem_cache_destroy() if
the name is between &boot_cache_name[0] and &boot_cache_name[n].
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


glommer at parallels

May 23, 2012, 2:25 AM

Post #6 of 8 (111 views)
Permalink
Re: [PATCH v2] slab+slob: dup name string [In reply to]

On 05/23/2012 07:55 AM, David Rientjes wrote:
> I hate consistency patches like this because it could potentially fail a
> kmem_cache_create() from a sufficiently long cache name when it wouldn't
> have before, but I'm not really concerned since kmem_cache_create() will
> naturally be followed by kmem_cache_alloc() which is more likely to cause
> the oom anyway. But it's just another waste of memory for consistency
> sake.
>
> This is much easier to do, just statically allocate the const char *'s
> needed for the boot caches and then set their ->name's manually in
> kmem_cache_init() and then avoid the kfree() in kmem_cache_destroy() if
> the name is between&boot_cache_name[0] and&boot_cache_name[n].

That can be done.

I'll also revisit my memcg patches to see if I can rework it so it
doesn't care about this particular behavior. We're having a surprisingly
difficult time reaching consensus on this, so maybe it would be better
left untouched (if there is a way that makes sense to)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


cl at linux

May 23, 2012, 6:53 AM

Post #7 of 8 (107 views)
Permalink
Re: [PATCH v2] slab+slob: dup name string [In reply to]

On Tue, 22 May 2012, David Rientjes wrote:

> On Tue, 22 May 2012, Christoph Lameter wrote:
>
> > > [. v2: Also dup string for early caches, requested by David Rientjes ]
> >
> > kstrdups that early could cause additional issues. Its better to leave
> > things as they were.
> >
>
> No, it's not, there's no reason to prevent caches created before
> g_cpucache_up <= EARLY to be destroyed because it makes a patch easier to
> implement and then leave that little gotcha as an undocumented treasure
> for someone to find when they try it later on.

g_cpucache_up <= EARLY is slab bootstrap code and the system is in a
pretty fragile state. Plus the the kmalloc logic *depends* on these
caches being present. Removing those is not a good idea. The other caches
that are created at that point are needed to create more caches.

There is no reason to remove these caches.

> This is much easier to do, just statically allocate the const char *'s
> needed for the boot caches and then set their ->name's manually in
> kmem_cache_init() and then avoid the kfree() in kmem_cache_destroy() if
> the name is between &boot_cache_name[0] and &boot_cache_name[n].

Yeah that is already occurring for some of the boot caches.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


rientjes at google

May 23, 2012, 6:01 PM

Post #8 of 8 (108 views)
Permalink
Re: [PATCH v2] slab+slob: dup name string [In reply to]

On Wed, 23 May 2012, Christoph Lameter wrote:

> > No, it's not, there's no reason to prevent caches created before
> > g_cpucache_up <= EARLY to be destroyed because it makes a patch easier to
> > implement and then leave that little gotcha as an undocumented treasure
> > for someone to find when they try it later on.
>
> g_cpucache_up <= EARLY is slab bootstrap code and the system is in a
> pretty fragile state. Plus the the kmalloc logic *depends* on these
> caches being present. Removing those is not a good idea. The other caches
> that are created at that point are needed to create more caches.
>
> There is no reason to remove these caches.
>

Yes, we know that we don't want to remove the caches that are currently
created in kmem_cache_init(), it would be a pretty stupid thing to do.
I'm talking about the possibility of creating additional caches while
g_cpucache_up <= EARLY in the future and then finding that you can't
destroy them because of this string allocation. I don't think it's too
difficult to statically allocate space for these names and then test for
it before doing kfree() in kmem_cache_destroy(), it's not performance
critical.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Linux kernel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.