On Thu, 30 Jan 2014 20:01:33 +0400 Vladimir Davydov <vdavydov@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> Cc: Johannes Weiner <hannes@xxxxxxxxxxx> Cc: Michal Hocko <mhocko@xxxxxxx> Cc: Vladimir Davydov <vdavydov@xxxxxxxxxxxxx> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> --- 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; } _ -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx"> email@xxxxxxxxx </a>