Re: [RFC][BUGFIX][PATCH] memcg: fix shrink_usage

[Balbir Singh <balbir@linux.vnet.ibm.com>]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2009-03-26 14:12:46]:

> On Thu, 26 Mar 2009 13:08:21 +0900
> Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> wrote:
> 
> > This is another bug I've working on recently.
> > 
> > I want this (and the stale swapcache problem) to be fixed for 2.6.30.
> > 
> > Any comments?
> > 
> > ===
> > From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> > 
> > Current mem_cgroup_shrink_usage has two problems.
> > 
> > 1. It doesn't call mem_cgroup_out_of_memory and doesn't update last_oom_jiffies,
> >    so pagefault_out_of_memory invokes global OOM.
> > 2. Considering hierarchy, shrinking has to be done from the mem_over_limit,
> >    not from the memcg where the page to be charged to.
> > 
> 
> Ah, i see. good cacth. 
> But it seems to be the patch is a bit big and includes duplications.
> Can't we divide this patch into 2 and reduce modification ?
> 
> mem_cgroup_shrink_usage() should do something proper...
> My brief thinking is a patch like this, how do you think ?
> 
> Maybe renaming this function is appropriate...
> ==
> mem_cgroup_shrink_usage() is called by shmem, but its purpose is
> not different from try_charge().
> 
> In current behavior, it ignores upward hierarchy and doesn't update
> OOM status of memcg. That's bad. We can simply call try_charge()
> and drop charge later.
>

This seems much better than the original patch from Daisuke, which
added too much code and changes, hard to review for correctness and
changes outside of memcontrol.c make it more risky.
 
> Reported-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  mm/memcontrol.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> Index: test/mm/memcontrol.c
> ===================================================================
> --- test.orig/mm/memcontrol.c
> +++ test/mm/memcontrol.c
> @@ -1655,16 +1655,16 @@ int mem_cgroup_shrink_usage(struct page 
>  	if (unlikely(!mem))
>  		return 0;
> 
> -	do {
> -		progress = mem_cgroup_hierarchical_reclaim(mem,
> -					gfp_mask, true, false);
> -		progress += mem_cgroup_check_under_limit(mem);
> -	} while (!progress && --retry);
> +	ret = __mem_cgroup_try_charge(NULL, GFP_KERNEL, mem, true);
>

Could you please add a comment/changelog to indicate why we try to
charge when we want to shrink? Is the limit setup so that a try_charge
will cause reclaim, BTW?
 
> +	if (!ret) {
> +		css_put(&mem->css); /* refcnt by charge *//

Does this compile?

> +		res_counter_uncharge(&mem->res, PAGE_SIZE);
> +		if (do_swap_account)
> +			res_counter_uncharge(&mem->memsw, PAGE_SIZE);
> +	}
>  	css_put(&mem->css);
> -	if (!retry)
> -		return -ENOMEM;
> -	return 0;
> +	return ret;
>  }
> 
>  static DEFINE_MUTEX(set_limit_mutex);
> 
> 

-- 
	Balbir

--
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>