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

[Johannes Weiner]

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