From mboxrd@z Thu Jan 1 00:00:00 1970 From: Glauber Costa Subject: Re: [PATCH 4/4] memcg: replace cgroup_lock with memcg specific memcg_lock Date: Tue, 4 Dec 2012 12:31:31 +0400 Message-ID: <50BDB4E3.4040107@parallels.com> References: <1354282286-32278-1-git-send-email-glommer@parallels.com> <1354282286-32278-5-git-send-email-glommer@parallels.com> <20121203171532.GG17093@dhcp22.suse.cz> <50BDAD38.6030200@parallels.com> <20121204082316.GB31319@dhcp22.suse.cz> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20121204082316.GB31319-2MMpYkNvuYDjFM9bn6wA6Q@public.gmane.org> Sender: cgroups-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: Content-Type: text/plain; charset="us-ascii" To: Michal Hocko Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, Tejun Heo , kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org, Johannes Weiner On 12/04/2012 12:23 PM, Michal Hocko wrote: > On Tue 04-12-12 11:58:48, Glauber Costa wrote: >> On 12/03/2012 09:15 PM, Michal Hocko wrote: >>> On Fri 30-11-12 17:31:26, Glauber Costa wrote: >>> [...] >>>> +/* >>>> + * must be called with memcg_lock held, unless the cgroup is guaranteed to be >>>> + * already dead (like in mem_cgroup_force_empty, for instance). >>>> + */ >>>> +static inline bool memcg_has_children(struct mem_cgroup *memcg) >>>> +{ >>>> + return mem_cgroup_count_children(memcg) != 1; >>>> +} >>> >>> Why not just keep list_empty(&cgrp->children) which is much simpler much >>> more effective and correct here as well because cgroup cannot vanish >>> while we are at the call because all callers come from cgroup fs? >>> >> Because it depends on cgroup's internal representation, which I think >> we're better off not depending upon, even if this is not as serious a >> case as the locking stuff. But also, technically, cgrp->children is >> protected by the cgroup_lock(), while since we'll hold the memcg_lock >> during creation and also around the iterators, we cover everything with >> the same lock. > > The list is RCU safe so we do not have to use cgroup_lock there for this > kind of test. > >> That said, of course we don't need to do the full iteration here, and >> mem_cgroup_count_children is indeed overkill. We could just as easily >> verify if any child exist - it is just an emptiness test after all. But >> it is not living in any fast path, though, and I just assumed code reuse >> to win over efficiency in this particular case - >> mem_cgroup_count_children already existed... > > Yes but the function name suggests a more generic usage and the test is > really an overkill. Maybe we can get a cgroup generic helper > cgroup_as_children which would do the thing without exhibiting cgroup > internals. What do you think? > I will give it another round of thinking, but I still don't see the reason for calling to cgroup core with this. If you really dislike doing a children count (I don't like as well, I just don't dislike), maybe we can do something like: i = 0; for_each_mem_cgroup_tree(iter, memcg) { if (i++ == 1) return false; } return true;