From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755083Ab3CXHdk (ORCPT ); Sun, 24 Mar 2013 03:33:40 -0400 Received: from szxga02-in.huawei.com ([119.145.14.65]:49305 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755018Ab3CXHde (ORCPT ); Sun, 24 Mar 2013 03:33:34 -0400 Message-ID: <514EAC41.5050700@huawei.com> Date: Sun, 24 Mar 2013 15:33:21 +0800 From: Li Zefan User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/20130307 Thunderbird/17.0.4 MIME-Version: 1.0 To: Michal Hocko CC: Glauber Costa , Tejun Heo , LKML , Cgroups , , KAMEZAWA Hiroyuki , Johannes Weiner 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 X-Originating-IP: [10.135.68.215] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org >> 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; > } > >