Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752818Ab1FIQ6T (ORCPT ); Thu, 9 Jun 2011 12:58:19 -0400 Received: from zene.cmpxchg.org ([85.214.230.12]:54747 "EHLO zene.cmpxchg.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750959Ab1FIQ6S (ORCPT ); Thu, 9 Jun 2011 12:58:18 -0400 Date: Thu, 9 Jun 2011 18:57:48 +0200 From: Johannes Weiner To: Christoph Hellwig Cc: KAMEZAWA Hiroyuki , Daisuke Nishimura , Balbir Singh , Ying Han , Michal Hocko , Andrew Morton , Rik van Riel , Minchan Kim , KOSAKI Motohiro , Mel Gorman , Greg Thelen , Michel Lespinasse , linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [patch 2/8] mm: memcg-aware global reclaim Message-ID: <20110609165748.GA20333@cmpxchg.org> References: <1306909519-7286-1-git-send-email-hannes@cmpxchg.org> <1306909519-7286-3-git-send-email-hannes@cmpxchg.org> <20110607122519.GA18571@infradead.org> <20110608093046.GB17886@cmpxchg.org> <20110609092617.GB10741@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20110609092617.GB10741@infradead.org> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3127 Lines: 81 On Thu, Jun 09, 2011 at 05:26:17AM -0400, Christoph Hellwig wrote: > On Wed, Jun 08, 2011 at 11:30:46AM +0200, Johannes Weiner wrote: > > On Tue, Jun 07, 2011 at 08:25:19AM -0400, Christoph Hellwig wrote: > > > A few small nitpicks: > > > > > > > +struct mem_cgroup *mem_cgroup_hierarchy_walk(struct mem_cgroup *root, > > > > + struct mem_cgroup *prev) > > > > +{ > > > > + struct mem_cgroup *mem; > > > > + > > > > + if (mem_cgroup_disabled()) > > > > + return NULL; > > > > + > > > > + if (!root) > > > > + root = root_mem_cgroup; > > > > + /* > > > > + * Even without hierarchy explicitely enabled in the root > > > > + * memcg, it is the ultimate parent of all memcgs. > > > > + */ > > > > + if (!(root == root_mem_cgroup || root->use_hierarchy)) > > > > + return root; > > > > > > The logic here reads a bit weird, why not simply: > > > > > > /* > > > * Even without hierarchy explicitely enabled in the root > > > * memcg, it is the ultimate parent of all memcgs. > > > */ > > > if (!root || root == root_mem_cgroup) > > > return root_mem_cgroup; > > > if (root->use_hierarchy) > > > return root; > > > > What you are proposing is not equivalent, so... case in point! It's > > meant to do the hierarchy walk for when foo->use_hierarchy, obviously, > > but ALSO for root_mem_cgroup, which is parent to everyone else even > > without use_hierarchy set. I changed it to read like this: > > > > if (!root) > > root = root_mem_cgroup; > > if (!root->use_hierarchy && root != root_mem_cgroup) > > return root; > > /* actually iterate hierarchy */ > > > > Does that make more sense? > > It does, sorry for misparsing it. The thing that I really hated was > the conditional assignment of root. Can we clean this up somehow > by making the caller pass root_mem_cgroup in the case where it > passes root right now, or at least always pass NULL when it means > root_mem_cgroup. > > Note really that important in the end, it just irked me when I looked > over it, especially the conditional assigned of root to root_mem_cgroup, > and then a little later checking for the equality of the two. Yeah, the assignment is an ugly interface fixup because root_mem_cgroup is local to memcontrol.c, as is struct mem_cgroup as a whole. I'll look into your suggestion from the other mail of making struct mem_cgroup and struct mem_cgroup_per_zone always available, and have everyone operate against root_mem_cgroup per default. > Thinking about it it's probably better left as-is for now to not > complicate the series, and maybe revisit it later once things have > settled a bit. I may take you up on that if this approach turns out to require more change than is sensible to add to this series. I'll at least add an /* XXX: until vmscan.c knows about root_mem_cgroup */ or so, if this is the case, to explain the temporary nastiness. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/