Re: [PATCH 4/7] memcg : fix charge function of THP allocation.

[KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>]

On Fri, 21 Jan 2011 17:48:18 +0900
Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> wrote:

> On Fri, 21 Jan 2011 15:44:30 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > 
> > When THP is used, Hugepage size charge can happen. It's not handled
> > correctly in mem_cgroup_do_charge(). For example, THP can fallback
> > to small page allocation when HUGEPAGE allocation seems difficult
> > or busy, but memory cgroup doesn't understand it and continue to
> > try HUGEPAGE charging. And the worst thing is memory cgroup
> > believes 'memory reclaim succeeded' if limit - usage > PAGE_SIZE.
> > 
> > By this, khugepaged etc...can goes into inifinite reclaim loop
> > if tasks in memcg are busy.
> > 
> > After this patch 
> >  - Hugepage allocation will fail if 1st trial of page reclaim fails.
> >  - distinguish THP allocaton from Bached allocation. 
> > 
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > ---
> >  mm/memcontrol.c |   51 +++++++++++++++++++++++++++++++++++----------------
> >  1 file changed, 35 insertions(+), 16 deletions(-)
> > 
> > Index: mmotm-0107/mm/memcontrol.c
> > ===================================================================
> > --- mmotm-0107.orig/mm/memcontrol.c
> > +++ mmotm-0107/mm/memcontrol.c
> > @@ -1812,24 +1812,25 @@ enum {
> >  	CHARGE_OK,		/* success */
> >  	CHARGE_RETRY,		/* need to retry but retry is not bad */
> >  	CHARGE_NOMEM,		/* we can't do more. return -ENOMEM */
> > +	CHARGE_NEED_BREAK,	/* big size allocation failure */
> >  	CHARGE_WOULDBLOCK,	/* GFP_WAIT wasn't set and no enough res. */
> >  	CHARGE_OOM_DIE,		/* the current is killed because of OOM */
> >  };
> >  
> >  static int __mem_cgroup_do_charge(struct mem_cgroup *mem, gfp_t gfp_mask,
> > -				int csize, bool oom_check)
> > +			int page_size, bool do_reclaim, bool oom_check)
> 
> I'm sorry, I can't understand why we need 'do_reclaim'. See below.
> 
> >  {
> >  	struct mem_cgroup *mem_over_limit;
> >  	struct res_counter *fail_res;
> >  	unsigned long flags = 0;
> >  	int ret;
> >  
> > -	ret = res_counter_charge(&mem->res, csize, &fail_res);
> > +	ret = res_counter_charge(&mem->res, page_size, &fail_res);
> >  
> >  	if (likely(!ret)) {
> >  		if (!do_swap_account)
> >  			return CHARGE_OK;
> > -		ret = res_counter_charge(&mem->memsw, csize, &fail_res);
> > +		ret = res_counter_charge(&mem->memsw, page_size, &fail_res);
> >  		if (likely(!ret))
> >  			return CHARGE_OK;
> >  
> > @@ -1838,14 +1839,14 @@ static int __mem_cgroup_do_charge(struct
> >  	} else
> >  		mem_over_limit = mem_cgroup_from_res_counter(fail_res, res);
> >  
> > -	if (csize > PAGE_SIZE) /* change csize and retry */
> > +	if (!do_reclaim)
> >  		return CHARGE_RETRY;
> >  
> 
> From the very beginning, do we need this "CHARGE_RETRY" ?
> 

Reducing charge_size here in automatic and go back to the start of this function ? 
I think returning here is better.


> >  	if (!(gfp_mask & __GFP_WAIT))
> >  		return CHARGE_WOULDBLOCK;
> >  
> >  	ret = mem_cgroup_hierarchical_reclaim(mem_over_limit, NULL,
> > -					gfp_mask, flags, csize);
> > +					gfp_mask, flags, page_size);
> >  	/*
> >  	 * try_to_free_mem_cgroup_pages() might not give us a full
> >  	 * picture of reclaim. Some pages are reclaimed and might be
> > @@ -1853,19 +1854,28 @@ static int __mem_cgroup_do_charge(struct
> >  	 * Check the limit again to see if the reclaim reduced the
> >  	 * current usage of the cgroup before giving up
> >  	 */
> > -	if (ret || mem_cgroup_check_under_limit(mem_over_limit, csize))
> > +	if (ret || mem_cgroup_check_under_limit(mem_over_limit, page_size))
> >  		return CHARGE_RETRY;
> >  
> >  	/*
> > +	 * When page_size > PAGE_SIZE, THP calls this function and it's
> > +	 * ok to tell 'there are not enough pages for hugepage'. THP will
> > +	 * fallback into PAGE_SIZE allocation. If we do reclaim eagerly,
> > +	 * page splitting will occur and it seems much worse.
> > +	 */
> > +	if (page_size > PAGE_SIZE)
> > +		return CHARGE_NEED_BREAK;
> > +
> > +	/*
> >  	 * At task move, charge accounts can be doubly counted. So, it's
> >  	 * better to wait until the end of task_move if something is going on.
> >  	 */
> >  	if (mem_cgroup_wait_acct_move(mem_over_limit))
> >  		return CHARGE_RETRY;
> > -
> >  	/* If we don't need to call oom-killer at el, return immediately */
> >  	if (!oom_check)
> >  		return CHARGE_NOMEM;
> > +
> >  	/* check OOM */
> >  	if (!mem_cgroup_handle_oom(mem_over_limit, gfp_mask))
> >  		return CHARGE_OOM_DIE;
> > @@ -1885,7 +1895,7 @@ static int __mem_cgroup_try_charge(struc
> >  	int nr_oom_retries = MEM_CGROUP_RECLAIM_RETRIES;
> >  	struct mem_cgroup *mem = NULL;
> >  	int ret;
> > -	int csize = max(CHARGE_SIZE, (unsigned long) page_size);
> > +	bool use_pcp_cache = (page_size == PAGE_SIZE);
> >  
> >  	/*
> >  	 * Unlike gloval-vm's OOM-kill, we're not in memory shortage
> > @@ -1910,7 +1920,7 @@ again:
> >  		VM_BUG_ON(css_is_removed(&mem->css));
> >  		if (mem_cgroup_is_root(mem))
> >  			goto done;
> > -		if (page_size == PAGE_SIZE && consume_stock(mem))
> > +		if (use_pcp_cache && consume_stock(mem))
> >  			goto done;
> >  		css_get(&mem->css);
> >  	} else {
> > @@ -1933,7 +1943,7 @@ again:
> >  			rcu_read_unlock();
> >  			goto done;
> >  		}
> > -		if (page_size == PAGE_SIZE && consume_stock(mem)) {
> > +		if (use_pcp_cache && consume_stock(mem)) {
> >  			/*
> >  			 * It seems dagerous to access memcg without css_get().
> >  			 * But considering how consume_stok works, it's not
> > @@ -1967,17 +1977,26 @@ again:
> >  			oom_check = true;
> >  			nr_oom_retries = MEM_CGROUP_RECLAIM_RETRIES;
> >  		}
> > -
> > -		ret = __mem_cgroup_do_charge(mem, gfp_mask, csize, oom_check);
> > +		if (use_pcp_cache)
> > +			ret = __mem_cgroup_do_charge(mem, gfp_mask,
> > +					CHARGE_SIZE, false, oom_check);
> > +		else
> > +			ret = __mem_cgroup_do_charge(mem, gfp_mask,
> > +					page_size, true, oom_check);
> >  
> 
> hmm, this confuses me. I think 'use_pcp_cache' will be used to decide
> whether we should do consume_stock() or not, but why we change charge size
> and reclaim behavior depending on it ? I think this code itself is right,
> but using 'use_pcp_cache' confused me.
> 

Is it problem of function name ? 
'do_batched_charge' or some ?

I'd like to use a 'xxxx_size' variable rather than 2 xxxx_size variable.



> 
> >  		switch (ret) {
> >  		case CHARGE_OK:
> >  			break;
> >  		case CHARGE_RETRY: /* not in OOM situation but retry */
> > -			csize = page_size;
> > +			if (use_pcp_cache)/* need to reclaim pages */
> > +				use_pcp_cache = false;
> >  			css_put(&mem->css);
> >  			mem = NULL;
> >  			goto again;
> > +		case CHARGE_NEED_BREAK: /* page_size > PAGE_SIZE */
> > +			css_put(&mem->css);
> > +			/* returning faiulre doesn't mean OOM for hugepages */
> > +			goto nomem;
> 
> I like this change.
> 
> >  		case CHARGE_WOULDBLOCK: /* !__GFP_WAIT */
> >  			css_put(&mem->css);
> >  			goto nomem;
> > @@ -1994,9 +2013,9 @@ again:
> >  			goto bypass;
> >  		}
> >  	} while (ret != CHARGE_OK);
> > -
> > -	if (csize > page_size)
> > -		refill_stock(mem, csize - page_size);
> > +	/* This flag is cleared when we fail CHAEGE_SIZE charge. */
> > +	if (use_pcp_cache)
> > +		refill_stock(mem, CHARGE_SIZE - page_size);
> 
> Ditto. can't we keep 'csize' and old code here ?
> 

I remove csize. 2 'size' variable is confusing.


Thanks.
-Kame

--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>