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

[Michal Hocko <mhocko@suse.cz>]

On Tue 04-02-14 11:05:09, Johannes Weiner wrote:
> On Tue, Feb 04, 2014 at 02:28:56PM +0100, Michal Hocko wrote:
[...]
> > +static bool current_bypass_charge(void)
> > +{
> > +	/*
> > +	 * Unlike gloval-vm's OOM-kill, we're not in memory shortage
> > +	 * in system level. So, allow to go ahead dying process in addition to
> > +	 * MEMDIE process.
> > +	 */
> > +	if (unlikely(test_thread_flag(TIF_MEMDIE)
> > +		     || fatal_signal_pending(current)))
> > +		return true;
> > +
> > +	return false;
> > +}
> 
> I'd just leave it inline at this point, it lines up nicely with the
> other pre-charge checks in try_charge, which is at this point short
> enough to take this awkward 3-liner.

I can keep it inline of course. I thought having it out of line would
make it more obvious what are the bypass conditions. But as there is
still mem_cgroup_is_root then it is probably not the best thing to do.

> > +static int mem_cgroup_try_charge_memcg(gfp_t gfp_mask,
> >  				   unsigned int nr_pages,
> > -				   struct mem_cgroup **ptr,
> > +				   struct mem_cgroup *memcg,
> >  				   bool oom)
> >  {
> >  	unsigned int batch = max(CHARGE_BATCH, nr_pages);
> >  	int nr_oom_retries = MEM_CGROUP_RECLAIM_RETRIES;
> > -	struct mem_cgroup *memcg = NULL;
> >  	int ret;
> >  
> > -	/*
> > -	 * Unlike gloval-vm's OOM-kill, we're not in memory shortage
> > -	 * in system level. So, allow to go ahead dying process in addition to
> > -	 * MEMDIE process.
> > -	 */
> > -	if (unlikely(test_thread_flag(TIF_MEMDIE)
> > -		     || fatal_signal_pending(current)))
> > +	if (mem_cgroup_is_root(memcg) || current_bypass_charge())
> >  		goto bypass;
> >  
> >  	if (unlikely(task_in_memcg_oom(current)))
> >  		goto nomem;
> >  
> > +	if (consume_stock(memcg, nr_pages))
> > +		return 0;
> > +
> >  	if (gfp_mask & __GFP_NOFAIL)
> >  		oom = false;
> >  
> > -	/*
> > -	 * 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.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>