[PATCH 1/2] SLUB: Fix merged slab cache names

[PATCH 1/2] SLUB: Fix merged slab cache names

[Pekka Enberg]

As explained by Linus "I'm Proud to be an American" Torvalds:

  Looking at the merging code, I actually think it's totally
  buggy. If you have something like this:

   - load module A: create slab cache A

   - load module B: create slab cache B that can merge with A

   - unload module A

   - "cat /proc/slabinfo": BOOM. Oops.

  exactly because the name is not handled correctly, and you'll have
  module B holding open a slab cache that has a name pointer that points
  to module A that no longer exists.

This patch fixes the problem by introducing a SLAB_DYNAMIC_NAME flag and using
kstrdup() to allocate memory when reference count is bumped up.

Cc: Christoph Lameter <cl@linux.com>
Cc: David Rientjes <rientjes@google.com>
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Pekka Enberg <penberg@kernel.org>
---
 include/linux/slab.h |    2 ++
 mm/slub.c            |   15 +++++++++++++++
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 59260e2..df201cf 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -76,6 +76,8 @@
 # define SLAB_FAILSLAB		0x00000000UL
 #endif
 
+#define SLAB_DYNAMIC_NAME	0x04000000UL	/* s->name is kmalloc()'d */
+
 /* The following flags affect the page allocator grouping pages by mobility */
 #define SLAB_RECLAIM_ACCOUNT	0x00020000UL		/* Objects are reclaimable */
 #define SLAB_TEMPORARY		SLAB_RECLAIM_ACCOUNT	/* Objects are short-lived */
diff --git a/mm/slub.c b/mm/slub.c
index 13fffe1..0704288 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -210,6 +210,9 @@ static inline int sysfs_slab_alias(struct kmem_cache *s, const char *p)
 							{ return 0; }
 static inline void sysfs_slab_remove(struct kmem_cache *s)
 {
+	if (s->flags & SLAB_DYNAMIC_NAME)
+		kfree(s->name);
+
 	kfree(s);
 }
 
@@ -3218,6 +3221,18 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size,
 	down_write(&slub_lock);
 	s = find_mergeable(size, align, flags, name, ctor);
 	if (s) {
+		if (!(s->flags & SLAB_DYNAMIC_NAME)) {
+			const char *new_name;
+
+			new_name = kstrdup(s->name, GFP_KERNEL);
+			if (!new_name)
+				goto err;
+
+			s->name = new_name;
+
+			s->flags |= SLAB_DYNAMIC_NAME;
+		}
+
 		s->refcount++;
 		/*
 		 * Adjust the object sizes so that we clear
-- 
1.6.3.3

[PATCH 2/2] SLUB: Mark merged slab caches in /proc/slabinfo

[Pekka Enberg]

SLUB uses the name of the first slab cache for all merged slab caches. To make
the output of /proc/slabinfo more obvious, append the name of each merged slab
cache to s->name.

An example output looks like this:

  bip-16,ip_dst_cache,kioctx     42     42    [snip]

Cc: Christoph Lameter <cl@linux.com>
Cc: David Rientjes <rientjes@google.com>
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Pekka Enberg <penberg@kernel.org>
---
 mm/slub.c |   41 ++++++++++++++++++++++++++++++-----------
 1 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 0704288..6b08256 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3149,6 +3149,34 @@ void __init kmem_cache_init_late(void)
 {
 }
 
+static int kmem_merge_names(struct kmem_cache *s, const char *name)
+{
+	size_t new_size;
+	char *new_name;
+
+	/* Don't append name to merged name more than once */
+	if (strstr(s->name, name))
+		return 0;
+
+	/* Comma separated and NULL terminated. */
+	new_size = strlen(s->name) + strlen(name) + 2;
+
+	new_name = kmalloc(new_size, GFP_KERNEL);
+	if (!new_name)
+		return -ENOMEM;
+
+	snprintf(new_name, new_size, "%s,%s", s->name, name);
+
+	if (s->flags & SLAB_DYNAMIC_NAME)
+		kfree(s->name);
+
+	s->name = new_name;
+
+	s->flags |= SLAB_DYNAMIC_NAME;
+
+	return 0;
+}
+
 /*
  * Find a mergeable slab cache
  */
@@ -3221,17 +3249,8 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size,
 	down_write(&slub_lock);
 	s = find_mergeable(size, align, flags, name, ctor);
 	if (s) {
-		if (!(s->flags & SLAB_DYNAMIC_NAME)) {
-			const char *new_name;
-
-			new_name = kstrdup(s->name, GFP_KERNEL);
-			if (!new_name)
-				goto err;
-
-			s->name = new_name;
-
-			s->flags |= SLAB_DYNAMIC_NAME;
-		}
+		if (kmem_merge_names(s, name))
+			goto err;
 
 		s->refcount++;
 		/*
-- 
1.6.3.3

Re: [PATCH 2/2] SLUB: Mark merged slab caches in /proc/slabinfo

[Pekka Enberg]

On Tue, Sep 14, 2010 at 8:06 PM, Pekka Enberg <penberg@kernel.org> wrote:
> SLUB uses the name of the first slab cache for all merged slab caches. To make
> the output of /proc/slabinfo more obvious, append the name of each merged slab
> cache to s->name.
>
> An example output looks like this:
>
>  bip-16,ip_dst_cache,kioctx     42     42    [snip]
>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: David Rientjes <rientjes@google.com>
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Pekka Enberg <penberg@kernel.org>

Unfortunately this breaks "slabtop". I guess we could try some other delimiter?

> ---
>  mm/slub.c |   41 ++++++++++++++++++++++++++++++-----------
>  1 files changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 0704288..6b08256 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3149,6 +3149,34 @@ void __init kmem_cache_init_late(void)
>  {
>  }
>
> +static int kmem_merge_names(struct kmem_cache *s, const char *name)
> +{
> +       size_t new_size;
> +       char *new_name;
> +
> +       /* Don't append name to merged name more than once */
> +       if (strstr(s->name, name))
> +               return 0;
> +
> +       /* Comma separated and NULL terminated. */
> +       new_size = strlen(s->name) + strlen(name) + 2;
> +
> +       new_name = kmalloc(new_size, GFP_KERNEL);
> +       if (!new_name)
> +               return -ENOMEM;
> +
> +       snprintf(new_name, new_size, "%s,%s", s->name, name);
> +
> +       if (s->flags & SLAB_DYNAMIC_NAME)
> +               kfree(s->name);
> +
> +       s->name = new_name;
> +
> +       s->flags |= SLAB_DYNAMIC_NAME;
> +
> +       return 0;
> +}
> +
>  /*
>  * Find a mergeable slab cache
>  */
> @@ -3221,17 +3249,8 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size,
>        down_write(&slub_lock);
>        s = find_mergeable(size, align, flags, name, ctor);
>        if (s) {
> -               if (!(s->flags & SLAB_DYNAMIC_NAME)) {
> -                       const char *new_name;
> -
> -                       new_name = kstrdup(s->name, GFP_KERNEL);
> -                       if (!new_name)
> -                               goto err;
> -
> -                       s->name = new_name;
> -
> -                       s->flags |= SLAB_DYNAMIC_NAME;
> -               }
> +               if (kmem_merge_names(s, name))
> +                       goto err;
>
>                s->refcount++;
>                /*
> --
> 1.6.3.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 2/2] SLUB: Mark merged slab caches in /proc/slabinfo

[Christoph Lameter]

On Tue, 14 Sep 2010, Pekka Enberg wrote:

> Unfortunately this breaks "slabtop". I guess we could try some other delimiter?

+ ?

Re: [PATCH 2/2] SLUB: Mark merged slab caches in /proc/slabinfo

[Pekka Enberg]

On Tue, 14 Sep 2010, Pekka Enberg wrote:
>> Unfortunately this breaks "slabtop". I guess we could try some other delimiter?

On Tue, Sep 14, 2010 at 8:28 PM, Christoph Lameter <cl@linux.com> wrote:
> + ?

Looking at procps sources, slabtop and other utilities seem to think
slab cache names are limited to 64 bytes. Linus, are you OK with
adding a "+" character prefix for merged caches like you suggested in
private?

                        Pekka

Re: [PATCH 2/2] SLUB: Mark merged slab caches in /proc/slabinfo

[Linus Torvalds]

I didn't see this before I replied to the 1/2 patch..

On Tue, Sep 14, 2010 at 10:59 AM, Pekka Enberg <penberg@kernel.org> wrote:
> On Tue, 14 Sep 2010, Pekka Enberg wrote:
>>> Unfortunately this breaks "slabtop". I guess we could try some other delimiter?
>
> On Tue, Sep 14, 2010 at 8:28 PM, Christoph Lameter <cl@linux.com> wrote:
>> + ?
>
> Looking at procps sources, slabtop and other utilities seem to think
> slab cache names are limited to 64 bytes. Linus, are you OK with
> adding a "+" character prefix for merged caches like you suggested in
> private?

I'd much rather see "x+y+z" and actually see what the slab entries are.

But we can certainly also say "limit it to 64 bytes" and truncate
things past that.

                Linus

Re: [PATCH 2/2] SLUB: Mark merged slab caches in /proc/slabinfo

[Pekka Enberg]

On Tue, Sep 14, 2010 at 9:05 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>> Looking at procps sources, slabtop and other utilities seem to think
>> slab cache names are limited to 64 bytes. Linus, are you OK with
>> adding a "+" character prefix for merged caches like you suggested in
>> private?
>
> I'd much rather see "x+y+z" and actually see what the slab entries are.
>
> But we can certainly also say "limit it to 64 bytes" and truncate
> things past that.

It's actually 63 bytes due to off-by-one error in procps. There never
has been such limit on the kernel side but I worry about breaking
other applications that are prepared to read even less characters. Oh
well, I'll send the reworked patches and let you decide.

                        Pekka

Re: [PATCH 1/2] SLUB: Fix merged slab cache names

[Christoph Lameter]

On Tue, 14 Sep 2010, Pekka Enberg wrote:

> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 59260e2..df201cf 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -76,6 +76,8 @@
>  # define SLAB_FAILSLAB		0x00000000UL
>  #endif
>
> +#define SLAB_DYNAMIC_NAME	0x04000000UL	/* s->name is kmalloc()'d */
> +

Put this into mm/slub.c as slub only flag? What is the difference from
refcount == 1?

> @@ -3218,6 +3221,18 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size,
>  	down_write(&slub_lock);
>  	s = find_mergeable(size, align, flags, name, ctor);
>  	if (s) {
> +		if (!(s->flags & SLAB_DYNAMIC_NAME)) {
> +			const char *new_name;
> +
> +			new_name = kstrdup(s->name, GFP_KERNEL);

Ok. Keeping the original name. Why dont we do strdup by default and always
do a kfree(s->name) on close?

Re: [PATCH 1/2] SLUB: Fix merged slab cache names

[Pekka Enberg]

  On 14.9.2010 20.26, Christoph Lameter wrote:
> On Tue, 14 Sep 2010, Pekka Enberg wrote:
>
>> diff --git a/include/linux/slab.h b/include/linux/slab.h
>> index 59260e2..df201cf 100644
>> --- a/include/linux/slab.h
>> +++ b/include/linux/slab.h
>> @@ -76,6 +76,8 @@
>>   # define SLAB_FAILSLAB		0x00000000UL
>>   #endif
>>
>> +#define SLAB_DYNAMIC_NAME	0x04000000UL	/* s->name is kmalloc()'d */
>> +
> Put this into mm/slub.c as slub only flag? What is the difference from
> refcount == 1?
I can put it in mm/slub.c but I was worried about someone reusing the 
bit for something else.

Do you mean refcount == 2? You don't know during kmem cache release time 
if someone was merged to the cache or not.
>> @@ -3218,6 +3221,18 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size,
>>   	down_write(&slub_lock);
>>   	s = find_mergeable(size, align, flags, name, ctor);
>>   	if (s) {
>> +		if (!(s->flags&  SLAB_DYNAMIC_NAME)) {
>> +			const char *new_name;
>> +
>> +			new_name = kstrdup(s->name, GFP_KERNEL);
> Ok. Keeping the original name. Why dont we do strdup by default and always
> do a kfree(s->name) on close?
>
I tried that. It gets very nasty during bootstrap.

Re: [PATCH 1/2] SLUB: Fix merged slab cache names

[Christoph Lameter]

On Tue, 14 Sep 2010, Pekka Enberg wrote:

> > Put this into mm/slub.c as slub only flag? What is the difference from
> > refcount == 1?
> I can put it in mm/slub.c but I was worried about someone reusing the bit for
> something else.

Allocate from the other end like __OBJECT_POISON.

> Do you mean refcount == 2? You don't know during kmem cache release time if
> someone was merged to the cache or not.

> > Ok. Keeping the original name. Why dont we do strdup by default and always
> > do a kfree(s->name) on close?
> >
> I tried that. It gets very nasty during bootstrap.

Add it only to kmem_cache_create() not to kmem_cache_open. That is not
used during bootstrap. The bootstrap caches do not matter since they are
never freed.

Re: [PATCH 1/2] SLUB: Fix merged slab cache names

[Pekka Enberg]

  On 14.9.2010 20.47, Christoph Lameter wrote:
> On Tue, 14 Sep 2010, Pekka Enberg wrote:
>
>>> Put this into mm/slub.c as slub only flag? What is the difference from
>>> refcount == 1?
>> I can put it in mm/slub.c but I was worried about someone reusing the bit for
>> something else.
> Allocate from the other end like __OBJECT_POISON.
I'll do that. Thanks!
>> Do you mean refcount == 2? You don't know during kmem cache release time if
>> someone was merged to the cache or not.
>>> Ok. Keeping the original name. Why dont we do strdup by default and always
>>> do a kfree(s->name) on close?
>>>
>> I tried that. It gets very nasty during bootstrap.
> Add it only to kmem_cache_create() not to kmem_cache_open. That is not
> used during bootstrap. The bootstrap caches do not matter since they are
> never freed
I tried that too. It doesn't work because we get merged to kmalloc 
caches and can't do kfree() on them.

             Pekka

Re: [PATCH 1/2] SLUB: Fix merged slab cache names

[Christoph Lameter]

On Tue, 14 Sep 2010, Pekka Enberg wrote:

> > > Do you mean refcount == 2? You don't know during kmem cache release time
> > > if
> > > someone was merged to the cache or not.
> > > > Ok. Keeping the original name. Why dont we do strdup by default and
> > > > always
> > > > do a kfree(s->name) on close?
> > > >
> > > I tried that. It gets very nasty during bootstrap.
> > Add it only to kmem_cache_create() not to kmem_cache_open. That is not
> > used during bootstrap. The bootstrap caches do not matter since they are
> > never freed
> I tried that too. It doesn't work because we get merged to kmalloc caches and
> can't do kfree() on them.

For most of the kmalloc caches we are already doing a kasprintf. See the
last for loop in kmem_cache_init(). There are a couple of cases where we
are not doing it right now. kmalloc-96 kmalloc-192 and
kmem_cache_node.

Re: [PATCH 1/2] SLUB: Fix merged slab cache names

[Linus Torvalds]

On Tue, Sep 14, 2010 at 10:06 AM, Pekka Enberg <penberg@kernel.org> wrote:
>
> @@ -3218,6 +3221,18 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size,
>        down_write(&slub_lock);
>        s = find_mergeable(size, align, flags, name, ctor);
>        if (s) {
> +               if (!(s->flags & SLAB_DYNAMIC_NAME)) {
> +                       const char *new_name;
> +
> +                       new_name = kstrdup(s->name, GFP_KERNEL);
> +                       if (!new_name)
> +                               goto err;
> +
> +                       s->name = new_name;

Once you do this, please actually update the name to list the first
few different names.

Since the SLAB_DYNAMIC_NAME test mean that this will only happen once,
I would suggest something like just doing

   size = strlen(s->name) + strlen(name) + sizeof(", ...");
   new_name = kmalloc(size, GFP_KERNEL);
   if (!new_name)
      return -ENOMEM;
   snprintf(new_name, "%s, %s...", s->name, name);

so that you get the two first names listed. Sure, it will be
wrong/incomplete if there's lots of merging, but it will be visually
very obvious in /proc/slabinfo and in the debug messages.

                  Linus