linux-kernel - Re: [PATCH -v2 2/6] memcg: cleanup charge routines

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]

Message-ID: <20140204164050.GR6963@cmpxchg.org>
Date:	Tue, 4 Feb 2014 11:40:50 -0500
From:	Johannes Weiner <hannes@...xchg.org>
To:	Michal Hocko <mhocko@...e.cz>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	LKML <linux-kernel@...r.kernel.org>, linux-mm@...ck.org
Subject: Re: [PATCH -v2 2/6] memcg: cleanup charge routines

On Tue, Feb 04, 2014 at 05:12:30PM +0100, Michal Hocko wrote:
> On Tue 04-02-14 11:05:09, Johannes Weiner wrote:
> > On Tue, Feb 04, 2014 at 02:28:56PM +0100, Michal Hocko wrote:
> > > -	/*
> > > -	 * We always charge the cgroup the mm_struct belongs to.
> > > -	 * The mm_struct's mem_cgroup changes on task migration if the
> > > -	 * thread group leader migrates. It's possible that mm is not
> > > -	 * set, if so charge the root memcg (happens for pagecache usage).
> > > -	 */
> > > -	if (!*ptr && !mm)
> > > -		*ptr = root_mem_cgroup;
> > 
> > [...]
> > 
> > >  /*
> > > + * Charges and returns memcg associated with the given mm (or root_mem_cgroup
> > > + * if mm is NULL). Returns NULL if memcg is under OOM.
> > > + */
> > > +static struct mem_cgroup *mem_cgroup_try_charge_mm(struct mm_struct *mm,
> > > +				   gfp_t gfp_mask,
> > > +				   unsigned int nr_pages,
> > > +				   bool oom)
> > > +{
> > > +	struct mem_cgroup *memcg;
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * We always charge the cgroup the mm_struct belongs to.
> > > +	 * The mm_struct's mem_cgroup changes on task migration if the
> > > +	 * thread group leader migrates. It's possible that mm is not
> > > +	 * set, if so charge the root memcg (happens for pagecache usage).
> > > +	 */
> > > +	if (!mm)
> > > +		goto bypass;
> > 
> > Why shuffle it around right before you remove it anyway?  Just start
> > the series off with the patches that delete stuff without having to
> > restructure anything, get those out of the way.
> 
> As mentioned in the previous email. I wanted to have this condition
> removal bisectable. So it is removed in the next patch when it is
> replaced by VM_BUG_ON.

I'm not suggesting to sneak the removal into *this* patch, just put
the simple stand-alone patches that remove stuff first in the series.

Seems pretty logical to me to first reduce the code base as much as
possible before reorganizing it.  This does not change bisectability
but it sure makes the patches easier to read.
--
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/