linux-kernel - Re: [patch 2/8] mm: memcg-aware global reclaim

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 9 Jun 2011 18:57:48 +0200
From:	Johannes Weiner <hannes@...xchg.org>
To:	Christoph Hellwig <hch@...radead.org>
Cc:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Daisuke Nishimura <nishimura@....nes.nec.co.jp>,
	Balbir Singh <balbir@...ux.vnet.ibm.com>,
	Ying Han <yinghan@...gle.com>, Michal Hocko <mhocko@...e.cz>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Rik van Riel <riel@...hat.com>,
	Minchan Kim <minchan.kim@...il.com>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Mel Gorman <mgorman@...e.de>, Greg Thelen <gthelen@...gle.com>,
	Michel Lespinasse <walken@...gle.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [patch 2/8] mm: memcg-aware global reclaim

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/