From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx193.postini.com [74.125.245.193]) by kanga.kvack.org (Postfix) with SMTP id 9712F6B00C6 for ; Sun, 24 Mar 2013 03:33:54 -0400 (EDT) Message-ID: <514EAC41.5050700@huawei.com> Date: Sun, 24 Mar 2013 15:33:21 +0800 From: Li Zefan MIME-Version: 1.0 Subject: Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name() References: <514A60CD.60208@huawei.com> <20130321090849.GF6094@dhcp22.suse.cz> <20130321102257.GH6094@dhcp22.suse.cz> <514BB23E.70908@huawei.com> <20130322080749.GB31457@dhcp22.suse.cz> <514C1388.6090909@huawei.com> <514C14BF.3050009@parallels.com> <20130322093141.GE31457@dhcp22.suse.cz> In-Reply-To: <20130322093141.GE31457@dhcp22.suse.cz> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Michal Hocko Cc: Glauber Costa , Tejun Heo , LKML , Cgroups , linux-mm@kvack.org, KAMEZAWA Hiroyuki , Johannes Weiner >> Thanks for identifying and fixing this. >> >> Li is right. The cache name will live long, but this is because the >> slab/slub caches will strdup it internally. So the actual memcg >> allocation is short lived. > > OK, I have totally missed that. Sorry about the confusion. Then all the > churn around the allocation is pointless, no? > What about: > --- >>>From 7ed7f53bb597e8cb40d9ac91ce16142fb60f1e93 Mon Sep 17 00:00:00 2001 > From: Michal Hocko > Date: Fri, 22 Mar 2013 10:22:54 +0100 > Subject: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name() > > As cgroup supports rename, it's unsafe to dereference dentry->d_name > without proper vfs locks. Fix this by using cgroup_name() rather than > dentry directly. > > Also open code memcg_cache_name because it is called only from > kmem_cache_dup which frees the returned name right after > kmem_cache_create_memcg makes a copy of it. Such a short-lived > allocation doesn't make too much sense. So replace it by a static > buffer as kmem_cache_dup is called with memcg_cache_mutex. > I doubt it's a win to add 4K to kernel text size instead of adding a few extra lines of code... but it's up to you. > Signed-off-by: Li Zefan > Signed-off-by: Michal Hocko > --- > mm/memcontrol.c | 33 +++++++++++---------------------- > 1 file changed, 11 insertions(+), 22 deletions(-) ... > static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg, > struct kmem_cache *s) > { > char *name; > struct kmem_cache *new; > + static char tmp_name[PAGE_SIZE]; > > - name = memcg_cache_name(memcg, s); > - if (!name) > - return NULL; > + lockdep_assert_held(&memcg_cache_mutex); > + > + rcu_read_lock(); > + tmp_name = snprintf(tmp_name, sizeof(tmp_name), "%s(%d:%s)", s->name, > + memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup)); I guess you didn't turn on CONFIG_MEMCG_KMEM? snprintf() returns a int value. > + rcu_read_unlock(); > > - new = kmem_cache_create_memcg(memcg, name, s->object_size, s->align, > + 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; > > - kfree(name); > return new; > } > > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org