[PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path

[PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path

[Vladimir Davydov]

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 mm/memcontrol.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 19d5d4274e22..53385cd4e6f0 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3400,7 +3400,7 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
 static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
 						  struct kmem_cache *s)
 {
-	struct kmem_cache *new;
+	struct kmem_cache *new = NULL;
 	static char *tmp_name = NULL;
 	static DEFINE_MUTEX(mutex);	/* protects tmp_name */
 
@@ -3416,7 +3416,7 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
 	if (!tmp_name) {
 		tmp_name = kmalloc(PATH_MAX, GFP_KERNEL);
 		if (!tmp_name)
-			return NULL;
+			goto out;
 	}
 
 	rcu_read_lock();
@@ -3426,12 +3426,11 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
 
 	new = kmem_cache_create_memcg(memcg, tmp_name, s->object_size, s->align,
 				      (s->flags & ~SLAB_PANIC), s->ctor, s);
-
 	if (new)
 		new->allocflags |= __GFP_KMEMCG;
 	else
 		new = s;
-
+out:
 	mutex_unlock(&mutex);
 	return new;
 }
-- 
1.7.10.4

Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path

[Andrew Morton]

On Thu, 30 Jan 2014 20:01:33 +0400 Vladimir Davydov <vdavydov@parallels.com> wrote:

> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3400,7 +3400,7 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
>  static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
>  						  struct kmem_cache *s)
>  {
> -	struct kmem_cache *new;
> +	struct kmem_cache *new = NULL;
>  	static char *tmp_name = NULL;
>  	static DEFINE_MUTEX(mutex);	/* protects tmp_name */
>  
> @@ -3416,7 +3416,7 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
>  	if (!tmp_name) {
>  		tmp_name = kmalloc(PATH_MAX, GFP_KERNEL);
>  		if (!tmp_name)
> -			return NULL;
> +			goto out;
>  	}
>  
>  	rcu_read_lock();
> @@ -3426,12 +3426,11 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
>  
>  	new = kmem_cache_create_memcg(memcg, tmp_name, s->object_size, s->align,
>  				      (s->flags & ~SLAB_PANIC), s->ctor, s);
> -
>  	if (new)
>  		new->allocflags |= __GFP_KMEMCG;
>  	else
>  		new = s;
> -
> +out:
>  	mutex_unlock(&mutex);
>  	return new;
>  }

Well gee, how did that one get through?

What was the point in permanently allocating tmp_name, btw?  "This
static temporary buffer is used to prevent from pointless shortliving
allocation"?  That's daft - memcg_create_kmem_cache() is not a fastpath
and there are a million places in the kernel where we could permanently
leak memory because it is "pointless" to allocate on demand.

The allocation of PATH_MAX bytes is unfortunate - kasprintf() wouild
work well here, but cgroup_name()'s need for rcu_read_lock() screws us
up.


So how about doing this?

From: Andrew Morton <akpm@linux-foundation.org>
Subject: mm/memcontrol.c: memcg_create_kmem_cache() tweaks

Allocate tmp_name on demand rather than permanently consuming PATH_MAX
bytes of memory.  This permits a small reduction in the mutex hold time as
well.

Cc: Glauber Costa <glommer@parallels.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: Vladimir Davydov <vdavydov@parallels.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/memcontrol.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff -puN mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks mm/memcontrol.c
--- a/mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks
+++ a/mm/memcontrol.c
@@ -3401,17 +3401,14 @@ static struct kmem_cache *memcg_create_k
 						  struct kmem_cache *s)
 {
 	struct kmem_cache *new = NULL;
-	static char *tmp_name = NULL;
+	static char *tmp_name;
 	static DEFINE_MUTEX(mutex);	/* protects tmp_name */
 
 	BUG_ON(!memcg_can_account_kmem(memcg));
 
-	mutex_lock(&mutex);
 	/*
-	 * kmem_cache_create_memcg duplicates the given name and
-	 * cgroup_name for this name requires RCU context.
-	 * This static temporary buffer is used to prevent from
-	 * pointless shortliving allocation.
+	 * kmem_cache_create_memcg duplicates the given name and cgroup_name()
+	 * for this name requires rcu_read_lock().
 	 */
 	if (!tmp_name) {
 		tmp_name = kmalloc(PATH_MAX, GFP_KERNEL);
@@ -3419,6 +3416,7 @@ static struct kmem_cache *memcg_create_k
 			goto out;
 	}
 
+	mutex_lock(&mutex);
 	rcu_read_lock();
 	snprintf(tmp_name, PATH_MAX, "%s(%d:%s)", s->name,
 			 memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));
@@ -3432,6 +3430,7 @@ static struct kmem_cache *memcg_create_k
 		new = s;
 out:
 	mutex_unlock(&mutex);
+	kfree(tmp_name);
 	return new;
 }
 
_

Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path

[David Rientjes]

On Thu, 30 Jan 2014, Vladimir Davydov wrote:

> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>

Some changelog would be helpful since this fixes an issue already in 
Linus's tree.

Commit 842e2873697e ("memcg: get rid of kmem_cache_dup()") introduced a 
mutex for memcg_create_kmem_cache() to protect the tmp_name buffer that 
holds the memcg name.  It failed to unlock the mutex if this buffer could 
not be allocated.

This patch fixes the issue by appropriately unlocking the mutex if the 
allocation fails.

Acked-by: David Rientjes <rientjes@google.com>


That said, this tmp_name stuff seems totally unnecessary.  
kmem_cache_create_memcg() already does the kstrdup() so why not just pass 
in a pointer to already allocated memory for s->name rather than having 
this mutex or global buffer at all?

Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path

[David Rientjes]

On Thu, 30 Jan 2014, Andrew Morton wrote:

> Well gee, how did that one get through?
> 
> What was the point in permanently allocating tmp_name, btw?  "This
> static temporary buffer is used to prevent from pointless shortliving
> allocation"?  That's daft - memcg_create_kmem_cache() is not a fastpath
> and there are a million places in the kernel where we could permanently
> leak memory because it is "pointless" to allocate on demand.
> 
> The allocation of PATH_MAX bytes is unfortunate - kasprintf() wouild
> work well here, but cgroup_name()'s need for rcu_read_lock() screws us
> up.
> 

What's funnier is that tmp_name isn't required at all since 
kmem_cache_create_memcg() is just going to do a kstrdup() on it anyway, so 
you could easily just pass in the pointer to memory that has been 
allocated for s->name rather than allocating memory twice.

> So how about doing this?
> 
> From: Andrew Morton <akpm@linux-foundation.org>
> Subject: mm/memcontrol.c: memcg_create_kmem_cache() tweaks
> 
> Allocate tmp_name on demand rather than permanently consuming PATH_MAX
> bytes of memory.  This permits a small reduction in the mutex hold time as
> well.
> 
> Cc: Glauber Costa <glommer@parallels.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@suse.cz>
> Cc: Vladimir Davydov <vdavydov@parallels.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  mm/memcontrol.c |   11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff -puN mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks mm/memcontrol.c
> --- a/mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks
> +++ a/mm/memcontrol.c
> @@ -3401,17 +3401,14 @@ static struct kmem_cache *memcg_create_k
>  						  struct kmem_cache *s)
>  {
>  	struct kmem_cache *new = NULL;
> -	static char *tmp_name = NULL;
> +	static char *tmp_name;

You're keeping it static and the mutex so you're still keeping it global, 
ok...

>  	static DEFINE_MUTEX(mutex);	/* protects tmp_name */
>  
>  	BUG_ON(!memcg_can_account_kmem(memcg));
>  
> -	mutex_lock(&mutex);
>  	/*
> -	 * kmem_cache_create_memcg duplicates the given name and
> -	 * cgroup_name for this name requires RCU context.
> -	 * This static temporary buffer is used to prevent from
> -	 * pointless shortliving allocation.
> +	 * kmem_cache_create_memcg duplicates the given name and cgroup_name()
> +	 * for this name requires rcu_read_lock().
>  	 */
>  	if (!tmp_name) {
>  		tmp_name = kmalloc(PATH_MAX, GFP_KERNEL);

Eek, memory leak.  Two concurrent calls to memcg_create_kmem_cache() find 
!tmp_name and do the kmalloc() concurrently.

> @@ -3419,6 +3416,7 @@ static struct kmem_cache *memcg_create_k
>  			goto out;
>  	}
>  
> +	mutex_lock(&mutex);
>  	rcu_read_lock();
>  	snprintf(tmp_name, PATH_MAX, "%s(%d:%s)", s->name,
>  			 memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));
> @@ -3432,6 +3430,7 @@ static struct kmem_cache *memcg_create_k
>  		new = s;
>  out:
>  	mutex_unlock(&mutex);
> +	kfree(tmp_name);

Why would we free the global buffer?

>  	return new;
>  }
>  

Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path

[Andrew Morton]

On Thu, 30 Jan 2014 13:14:46 -0800 (PST) David Rientjes <rientjes@google.com> wrote:

> On Thu, 30 Jan 2014, Andrew Morton wrote:
> 
> > Well gee, how did that one get through?
> > 
> > What was the point in permanently allocating tmp_name, btw?  "This
> > static temporary buffer is used to prevent from pointless shortliving
> > allocation"?  That's daft - memcg_create_kmem_cache() is not a fastpath
> > and there are a million places in the kernel where we could permanently
> > leak memory because it is "pointless" to allocate on demand.
> > 
> > The allocation of PATH_MAX bytes is unfortunate - kasprintf() wouild
> > work well here, but cgroup_name()'s need for rcu_read_lock() screws us
> > up.
> > 
> 
> What's funnier is that tmp_name isn't required at all since 
> kmem_cache_create_memcg() is just going to do a kstrdup() on it anyway, so 
> you could easily just pass in the pointer to memory that has been 
> allocated for s->name rather than allocating memory twice.

We need a buffer to sprintf() into.

> > --- a/mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks
> > +++ a/mm/memcontrol.c
> > @@ -3401,17 +3401,14 @@ static struct kmem_cache *memcg_create_k
> >  						  struct kmem_cache *s)
> >  {
> >  	struct kmem_cache *new = NULL;
> > -	static char *tmp_name = NULL;
> > +	static char *tmp_name;
> 
> You're keeping it static and the mutex so you're still keeping it global, 
> ok...

oop, I forgot to remove the `static'.

And I suppose the mutex now doesn't do anything, so...


From: Andrew Morton <akpm@linux-foundation.org>
Subject: mm/memcontrol.c: memcg_create_kmem_cache() tweaks

Allocate tmp_name on demand rather than permanently consuming PATH_MAX
bytes of memory.  Remove the mutex which protected the static tmp_name.

Cc: Glauber Costa <glommer@parallels.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: Vladimir Davydov <vdavydov@parallels.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/memcontrol.c |   23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff -puN mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks mm/memcontrol.c
--- a/mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks
+++ a/mm/memcontrol.c
@@ -3400,24 +3400,18 @@ void mem_cgroup_destroy_cache(struct kme
 static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
 						  struct kmem_cache *s)
 {
-	struct kmem_cache *new = NULL;
-	static char *tmp_name = NULL;
-	static DEFINE_MUTEX(mutex);	/* protects tmp_name */
+	struct kmem_cache *new;
+	char *tmp_name;
 
 	BUG_ON(!memcg_can_account_kmem(memcg));
 
-	mutex_lock(&mutex);
 	/*
-	 * kmem_cache_create_memcg duplicates the given name and
-	 * cgroup_name for this name requires RCU context.
-	 * This static temporary buffer is used to prevent from
-	 * pointless shortliving allocation.
+	 * kmem_cache_create_memcg duplicates the given name and cgroup_name()
+	 * for this name requires rcu_read_lock().
 	 */
-	if (!tmp_name) {
-		tmp_name = kmalloc(PATH_MAX, GFP_KERNEL);
-		if (!tmp_name)
-			goto out;
-	}
+	tmp_name = kmalloc(PATH_MAX, GFP_KERNEL);
+	if (!tmp_name)
+		return NULL;
 
 	rcu_read_lock();
 	snprintf(tmp_name, PATH_MAX, "%s(%d:%s)", s->name,
@@ -3430,8 +3424,7 @@ static struct kmem_cache *memcg_create_k
 		new->allocflags |= __GFP_KMEMCG;
 	else
 		new = s;
-out:
-	mutex_unlock(&mutex);
+	kfree(tmp_name);
 	return new;
 }
 
_

Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path

[David Rientjes]

On Thu, 30 Jan 2014, David Rientjes wrote:

> What's funnier is that tmp_name isn't required at all since 
> kmem_cache_create_memcg() is just going to do a kstrdup() on it anyway, so 
> you could easily just pass in the pointer to memory that has been 
> allocated for s->name rather than allocating memory twice.
> 

Something like this untested patch?


mm, memcg: only allocate memory for kmem slab cache name once

We must allocate memory to store a slab cache name when using kmem 
accounting since it involves the name of the memcg itself.  This should be 
the one and only memory allocation for that name, though.

Currently, we keep around a global buffer to construct the "memcg slab 
cache name" since it requires rcu protection and then pass it into
kmem_cache_create_memcg() which does its own kstrdup().

This patch only allocates and creates the slab cache name once and then 
passes a pointer into kmem_cache_create_memcg() as the name.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 mm/memcontrol.c  | 25 +++++++------------------
 mm/slab_common.c | 11 +++++------
 2 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3400,31 +3400,21 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
 static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
 						  struct kmem_cache *s)
 {
+	char *name = NULL;
 	struct kmem_cache *new;
-	static char *tmp_name = NULL;
-	static DEFINE_MUTEX(mutex);	/* protects tmp_name */
 
 	BUG_ON(!memcg_can_account_kmem(memcg));
 
-	mutex_lock(&mutex);
-	/*
-	 * kmem_cache_create_memcg duplicates the given name and
-	 * cgroup_name for this name requires RCU context.
-	 * This static temporary buffer is used to prevent from
-	 * pointless shortliving allocation.
-	 */
-	if (!tmp_name) {
-		tmp_name = kmalloc(PATH_MAX, GFP_KERNEL);
-		if (!tmp_name)
-			return NULL;
-	}
+	name = kmalloc(PATH_MAX, GFP_KERNEL);
+	if (unlikely(!name))
+		return NULL;
 
 	rcu_read_lock();
-	snprintf(tmp_name, PATH_MAX, "%s(%d:%s)", s->name,
-			 memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));
+	snprintf(name, PATH_MAX, "%s(%d:%s)", s->name, memcg_cache_id(memcg),
+		 cgroup_name(memcg->css.cgroup));
 	rcu_read_unlock();
 
-	new = kmem_cache_create_memcg(memcg, tmp_name, s->object_size, s->align,
+	new = kmem_cache_create_memcg(memcg, name, s->object_size, s->align,
 				      (s->flags & ~SLAB_PANIC), s->ctor, s);
 
 	if (new)
@@ -3432,7 +3422,6 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
 	else
 		new = s;
 
-	mutex_unlock(&mutex);
 	return new;
 }
 
diff --git a/mm/slab_common.c b/mm/slab_common.c
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -142,7 +142,7 @@ unsigned long calculate_alignment(unsigned long flags,
 
 /*
  * kmem_cache_create - Create a cache.
- * @name: A string which is used in /proc/slabinfo to identify this cache.
+ * @name: A string allocated for this cache used in /proc/slabinfo.
  * @size: The size of objects to be created in this cache.
  * @align: The required alignment for the objects.
  * @flags: SLAB flags
@@ -212,10 +212,7 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
 	s->object_size = s->size = size;
 	s->align = calculate_alignment(flags, align, size);
 	s->ctor = ctor;
-
-	s->name = kstrdup(name, GFP_KERNEL);
-	if (!s->name)
-		goto out_free_cache;
+	s->name = name;
 
 	err = memcg_alloc_cache_params(memcg, s, parent_cache);
 	if (err)
@@ -258,7 +255,6 @@ out_unlock:
 
 out_free_cache:
 	memcg_free_cache_params(s);
-	kfree(s->name);
 	kmem_cache_free(kmem_cache, s);
 	goto out_unlock;
 }
@@ -267,6 +263,9 @@ struct kmem_cache *
 kmem_cache_create(const char *name, size_t size, size_t align,
 		  unsigned long flags, void (*ctor)(void *))
 {
+	const char *cache_name = kstrdup(name, GFP_KERNEL);
+	if (unlikely(!cache_name))
+		return NULL;
 	return kmem_cache_create_memcg(NULL, name, size, align, flags, ctor, NULL);
 }
 EXPORT_SYMBOL(kmem_cache_create);

Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path

[David Rientjes]

On Thu, 30 Jan 2014, Andrew Morton wrote:

> > What's funnier is that tmp_name isn't required at all since 
> > kmem_cache_create_memcg() is just going to do a kstrdup() on it anyway, so 
> > you could easily just pass in the pointer to memory that has been 
> > allocated for s->name rather than allocating memory twice.
> 
> We need a buffer to sprintf() into.
> 

Yeah, it shouldn't be temporary it should be the one and only allocation.  
We should construct the name in memcg_create_kmem_cache() and be done with 
it.

> diff -puN mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks mm/memcontrol.c
> --- a/mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks
> +++ a/mm/memcontrol.c
> @@ -3400,24 +3400,18 @@ void mem_cgroup_destroy_cache(struct kme
>  static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
>  						  struct kmem_cache *s)
>  {
> -	struct kmem_cache *new = NULL;
> -	static char *tmp_name = NULL;
> -	static DEFINE_MUTEX(mutex);	/* protects tmp_name */
> +	struct kmem_cache *new;
> +	char *tmp_name;
>  
>  	BUG_ON(!memcg_can_account_kmem(memcg));
>  
> -	mutex_lock(&mutex);
>  	/*
> -	 * kmem_cache_create_memcg duplicates the given name and
> -	 * cgroup_name for this name requires RCU context.
> -	 * This static temporary buffer is used to prevent from
> -	 * pointless shortliving allocation.
> +	 * kmem_cache_create_memcg duplicates the given name and cgroup_name()
> +	 * for this name requires rcu_read_lock().
>  	 */
> -	if (!tmp_name) {
> -		tmp_name = kmalloc(PATH_MAX, GFP_KERNEL);
> -		if (!tmp_name)
> -			goto out;
> -	}
> +	tmp_name = kmalloc(PATH_MAX, GFP_KERNEL);
> +	if (!tmp_name)
> +		return NULL;
>  
>  	rcu_read_lock();
>  	snprintf(tmp_name, PATH_MAX, "%s(%d:%s)", s->name,
> @@ -3430,8 +3424,7 @@ static struct kmem_cache *memcg_create_k
>  		new->allocflags |= __GFP_KMEMCG;
>  	else
>  		new = s;
> -out:
> -	mutex_unlock(&mutex);
> +	kfree(tmp_name);
>  	return new;
>  }
>  

This is fine, but kmem_cache_create_memcg() is still just going to do a 
pointless kstrdup() on it which isn't necessary.

Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path

[Andrew Morton]

On Thu, 30 Jan 2014 13:38:56 -0800 (PST) David Rientjes <rientjes@google.com> wrote:

> > > What's funnier is that tmp_name isn't required at all since 
> > > kmem_cache_create_memcg() is just going to do a kstrdup() on it anyway, so 
> > > you could easily just pass in the pointer to memory that has been 
> > > allocated for s->name rather than allocating memory twice.
> > 
> > We need a buffer to sprintf() into.
> > 
> 
> Yeah, it shouldn't be temporary it should be the one and only allocation.  
> We should construct the name in memcg_create_kmem_cache() and be done with 
> it.

Could.  That would require converting memcg_create_kmem_cache() to take 
a va_list and call kasprintf() on it.

The problem is that pesky rcu_read_lock() which is required around
cgroup_name() - we'd have to call memcg_create_kmem_cache() under
rcu_read_lock() so the usual GFP_foo limitations apply.

Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path

[David Rientjes]

On Thu, 30 Jan 2014, Andrew Morton wrote:

> > Yeah, it shouldn't be temporary it should be the one and only allocation.  
> > We should construct the name in memcg_create_kmem_cache() and be done with 
> > it.
> 
> Could.  That would require converting memcg_create_kmem_cache() to take 
> a va_list and call kasprintf() on it.
> 

Why?  We already construct the name in memcg_create_kmem_cache() 
appropriately, we just want to avoid the kstrdup() in 
kmem_cache_create_memcg() since it's pointless like my patch does.

Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path

[Andrew Morton]

On Thu, 30 Jan 2014 14:04:12 -0800 (PST) David Rientjes <rientjes@google.com> wrote:

> On Thu, 30 Jan 2014, Andrew Morton wrote:
> 
> > > Yeah, it shouldn't be temporary it should be the one and only allocation.  
> > > We should construct the name in memcg_create_kmem_cache() and be done with 
> > > it.
> > 
> > Could.  That would require converting memcg_create_kmem_cache() to take 
> > a va_list and call kasprintf() on it.
> > 
> 
> Why?  We already construct the name in memcg_create_kmem_cache() 
> appropriately, we just want to avoid the kstrdup() in 
> kmem_cache_create_memcg() since it's pointless like my patch does.

oh, OK, missed that.

The problem now is that the string at kmem_cache.name is PATH_MAX
bytes, and PATH_MAX is huuuuuuuge.

Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path

[David Rientjes]

On Thu, 30 Jan 2014, Andrew Morton wrote:

> > Why?  We already construct the name in memcg_create_kmem_cache() 
> > appropriately, we just want to avoid the kstrdup() in 
> > kmem_cache_create_memcg() since it's pointless like my patch does.
> 
> oh, OK, missed that.
> 
> The problem now is that the string at kmem_cache.name is PATH_MAX
> bytes, and PATH_MAX is huuuuuuuge.
> 

It always was.  Google uses pretty long memcg names (although admittedly 
not as long as PATH_MAX!) and it hasn't caused any problem with 
/proc/slabinfo or slabtop(1).

Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path

[Andrew Morton]

On Thu, 30 Jan 2014 14:13:18 -0800 (PST) David Rientjes <rientjes@google.com> wrote:

> On Thu, 30 Jan 2014, Andrew Morton wrote:
> 
> > > Why?  We already construct the name in memcg_create_kmem_cache() 
> > > appropriately, we just want to avoid the kstrdup() in 
> > > kmem_cache_create_memcg() since it's pointless like my patch does.
> > 
> > oh, OK, missed that.
> > 
> > The problem now is that the string at kmem_cache.name is PATH_MAX
> > bytes, and PATH_MAX is huuuuuuuge.
> > 
> 
> It always was.

eh?  kmem_cache_create_memcg()'s kstrdup() will allocate the minimum
needed amount of memory.

Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path

[David Rientjes]

On Thu, 30 Jan 2014, Andrew Morton wrote:

> > It always was.
> 
> eh?  kmem_cache_create_memcg()'s kstrdup() will allocate the minimum
> needed amount of memory.
> 

Ah, good point.  We could this incrementally on my patch:

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -637,6 +637,9 @@ int memcg_limited_groups_array_size;
  * better kept as an internal representation in cgroup.c. In any case, the
  * cgrp_id space is not getting any smaller, and we don't have to necessarily
  * increase ours as well if it increases.
+ *
+ * Updates to MAX_SIZE should update the space for the memcg name in
+ * memcg_create_kmem_cache().
  */
 #define MEMCG_CACHES_MIN_SIZE 4
 #define MEMCG_CACHES_MAX_SIZE MEM_CGROUP_ID_MAX
@@ -3400,8 +3403,10 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
 static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
 						  struct kmem_cache *s)
 {
-	char *name = NULL;
 	struct kmem_cache *new;
+	const char *cgrp_name;
+	char *name = NULL;
+	size_t len;
 
 	BUG_ON(!memcg_can_account_kmem(memcg));
 
@@ -3409,9 +3414,22 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
 	if (unlikely(!name))
 		return NULL;
 
+	/*
+	 * Format of a memcg's kmem cache name:
+	 * <cache-name>(<memcg-id>:<cgroup-name>)
+	 */
+	len = strlen(s->name);
+	/* Space for parentheses, colon, terminator */
+	len += 4;
+	/* MEMCG_CACHES_MAX_SIZE is USHRT_MAX */
+	len += 5;
+	BUILD_BUG_ON(MEMCG_CACHES_MAX_SIZE > USHRT_MAX);
+
 	rcu_read_lock();
-	snprintf(name, PATH_MAX, "%s(%d:%s)", s->name, memcg_cache_id(memcg),
-		 cgroup_name(memcg->css.cgroup));
+	cgrp_name = cgroup_name(memcg->css.cgroup);
+	len += strlen(cgrp_name);
+	snprintf(name, len, "%s(%d:%s)", s->name, memcg_cache_id(memcg),
+		 cgrp_name);
 	rcu_read_unlock();
 
 	new = kmem_cache_create_memcg(memcg, name, s->object_size, s->align,

Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path

[Vladimir Davydov]

Hi, David

Thank you for taking look at this and adding the missing patch
description. WRT your patch, please see the comment inline.

On 01/31/2014 02:39 AM, David Rientjes wrote:
> On Thu, 30 Jan 2014, Andrew Morton wrote:
>
>>> It always was.
>> eh?  kmem_cache_create_memcg()'s kstrdup() will allocate the minimum
>> needed amount of memory.
>>
> Ah, good point.  We could this incrementally on my patch:
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -637,6 +637,9 @@ int memcg_limited_groups_array_size;
>   * better kept as an internal representation in cgroup.c. In any case, the
>   * cgrp_id space is not getting any smaller, and we don't have to necessarily
>   * increase ours as well if it increases.
> + *
> + * Updates to MAX_SIZE should update the space for the memcg name in
> + * memcg_create_kmem_cache().
>   */
>  #define MEMCG_CACHES_MIN_SIZE 4
>  #define MEMCG_CACHES_MAX_SIZE MEM_CGROUP_ID_MAX
> @@ -3400,8 +3403,10 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
>  static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
>  						  struct kmem_cache *s)
>  {
> -	char *name = NULL;
>  	struct kmem_cache *new;
> +	const char *cgrp_name;
> +	char *name = NULL;
> +	size_t len;
>  
>  	BUG_ON(!memcg_can_account_kmem(memcg));
>  
> @@ -3409,9 +3414,22 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
>  	if (unlikely(!name))
>  		return NULL;
>  
> +	/*
> +	 * Format of a memcg's kmem cache name:
> +	 * <cache-name>(<memcg-id>:<cgroup-name>)
> +	 */
> +	len = strlen(s->name);
> +	/* Space for parentheses, colon, terminator */
> +	len += 4;
> +	/* MEMCG_CACHES_MAX_SIZE is USHRT_MAX */
> +	len += 5;
> +	BUILD_BUG_ON(MEMCG_CACHES_MAX_SIZE > USHRT_MAX);
> +

This looks cumbersome, IMO. Let's leave it as is for now. AFAIK,
cgroup_name() will be reworked soon so that it won't require RCU-context
(https://lkml.org/lkml/2014/1/28/530). Therefore, it will be possible to
get rid of this pointless tmp_name allocation by making
kmem_cache_create_memcg() take not just name, but printf-like format +
vargs.

Thanks.

Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path

[David Rientjes]

On Fri, 31 Jan 2014, Vladimir Davydov wrote:

> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -637,6 +637,9 @@ int memcg_limited_groups_array_size;
> >   * better kept as an internal representation in cgroup.c. In any case, the
> >   * cgrp_id space is not getting any smaller, and we don't have to necessarily
> >   * increase ours as well if it increases.
> > + *
> > + * Updates to MAX_SIZE should update the space for the memcg name in
> > + * memcg_create_kmem_cache().
> >   */
> >  #define MEMCG_CACHES_MIN_SIZE 4
> >  #define MEMCG_CACHES_MAX_SIZE MEM_CGROUP_ID_MAX
> > @@ -3400,8 +3403,10 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
> >  static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
> >  						  struct kmem_cache *s)
> >  {
> > -	char *name = NULL;
> >  	struct kmem_cache *new;
> > +	const char *cgrp_name;
> > +	char *name = NULL;
> > +	size_t len;
> >  
> >  	BUG_ON(!memcg_can_account_kmem(memcg));
> >  
> > @@ -3409,9 +3414,22 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
> >  	if (unlikely(!name))
> >  		return NULL;
> >  
> > +	/*
> > +	 * Format of a memcg's kmem cache name:
> > +	 * <cache-name>(<memcg-id>:<cgroup-name>)
> > +	 */
> > +	len = strlen(s->name);
> > +	/* Space for parentheses, colon, terminator */
> > +	len += 4;
> > +	/* MEMCG_CACHES_MAX_SIZE is USHRT_MAX */
> > +	len += 5;
> > +	BUILD_BUG_ON(MEMCG_CACHES_MAX_SIZE > USHRT_MAX);
> > +
> 
> This looks cumbersome, IMO. Let's leave it as is for now. AFAIK,
> cgroup_name() will be reworked soon so that it won't require RCU-context
> (https://lkml.org/lkml/2014/1/28/530). Therefore, it will be possible to
> get rid of this pointless tmp_name allocation by making
> kmem_cache_create_memcg() take not just name, but printf-like format +
> vargs.
> 

You believe it's less cumbersome to do two memory allocations to figure 
out how much memory you really need to allocate rather than just 
calculating the necessary size?

Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path

[Vladimir Davydov]

On 01/31/2014 02:42 PM, David Rientjes wrote:
> On Fri, 31 Jan 2014, Vladimir Davydov wrote:
>
>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>>> --- a/mm/memcontrol.c
>>> +++ b/mm/memcontrol.c
>>> @@ -637,6 +637,9 @@ int memcg_limited_groups_array_size;
>>>   * better kept as an internal representation in cgroup.c. In any case, the
>>>   * cgrp_id space is not getting any smaller, and we don't have to necessarily
>>>   * increase ours as well if it increases.
>>> + *
>>> + * Updates to MAX_SIZE should update the space for the memcg name in
>>> + * memcg_create_kmem_cache().
>>>   */
>>>  #define MEMCG_CACHES_MIN_SIZE 4
>>>  #define MEMCG_CACHES_MAX_SIZE MEM_CGROUP_ID_MAX
>>> @@ -3400,8 +3403,10 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
>>>  static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
>>>  						  struct kmem_cache *s)
>>>  {
>>> -	char *name = NULL;
>>>  	struct kmem_cache *new;
>>> +	const char *cgrp_name;
>>> +	char *name = NULL;
>>> +	size_t len;
>>>  
>>>  	BUG_ON(!memcg_can_account_kmem(memcg));
>>>  
>>> @@ -3409,9 +3414,22 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
>>>  	if (unlikely(!name))
>>>  		return NULL;
>>>  
>>> +	/*
>>> +	 * Format of a memcg's kmem cache name:
>>> +	 * <cache-name>(<memcg-id>:<cgroup-name>)
>>> +	 */
>>> +	len = strlen(s->name);
>>> +	/* Space for parentheses, colon, terminator */
>>> +	len += 4;
>>> +	/* MEMCG_CACHES_MAX_SIZE is USHRT_MAX */
>>> +	len += 5;
>>> +	BUILD_BUG_ON(MEMCG_CACHES_MAX_SIZE > USHRT_MAX);
>>> +
>> This looks cumbersome, IMO. Let's leave it as is for now. AFAIK,
>> cgroup_name() will be reworked soon so that it won't require RCU-context
>> (https://lkml.org/lkml/2014/1/28/530). Therefore, it will be possible to
>> get rid of this pointless tmp_name allocation by making
>> kmem_cache_create_memcg() take not just name, but printf-like format +
>> vargs.
>>
> You believe it's less cumbersome to do two memory allocations to figure 
> out how much memory you really need to allocate rather than just 
> calculating the necessary size?

Well, I mean not the approach - here everything is right - but how it
looks. This

len += 4
len += 5

looks scary even with comments, IMHO. Note, I do not stand for this
temporary buffer - it was introduced long before I started tweaking this
code. I just want to say that substituting it now with something (OK,
less, but IMHO still) cumbersome is not a good idea provided soon it
will be possible to remove tmp_name while still having the code looking
nice. If you insist, I don't mind, but... why?

Thanks.