Re: [PATCH mmotm] memcg: make resize limit hold mutex

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

Daisuke Nishimura said:
>
> This patch define a new mutex and make both mem_cgroup_resize_limit and
> mem_cgroup_memsw_resize_limit hold it to remove spin_lock_irqsave.
>
Thanks,

>
> Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> ---
<snip>
> -	while (res_counter_set_limit(&memcg->res, val)) {
> +	while (retry_count) {
>  		if (signal_pending(current)) {
>  			ret = -EINTR;
>  			break;
>  		}
> -		if (!retry_count) {
> -			ret = -EBUSY;
> -			break;
> +		/*
> +		 * Rather than hide all in some function, I do this in
> +		 * open coded manner. You see what this really does.
> +		 * We have to guarantee mem->res.limit < mem->memsw.limit.
> +		 */
> +		if (do_swap_account) {
> +			mutex_lock(&set_limit_mutex);
> +			memswlimit = res_counter_read_u64(&memcg->memsw,
> +							RES_LIMIT);
> +			if (memswlimit < val) {
> +				ret = -EINVAL;
> +				mutex_unlock(&set_limit_mutex);
> +				break;
> +			}
> +			ret = res_counter_set_limit(&memcg->res, val);
> +			mutex_unlock(&set_limit_mutex);
>  		}

Maybe !do_swap_account case is not handled.
I think in !do_swap_account case, memsw.limit is inifinite.
So, just removing this "if" is ok.

No objection to your direction, could you fix ?

Thanks,
-Kame

> +
> +		if (!ret)
> +			break;
> +
>  		progress = try_to_free_mem_cgroup_pages(memcg,
>  				GFP_HIGHUSER_MOVABLE, false);
>  		if (!progress)
> @@ -1180,7 +1195,6 @@ int mem_cgroup_resize_memsw_limit(struct mem_cgroup
> *memcg,
>  				unsigned long long val)
>  {
>  	int retry_count = MEM_CGROUP_RECLAIM_RETRIES;
> -	unsigned long flags;
>  	u64 memlimit, oldusage, curusage;
>  	int ret;
>
> @@ -1197,19 +1211,20 @@ int mem_cgroup_resize_memsw_limit(struct
> mem_cgroup *memcg,
>  		 * open coded manner. You see what this really does.
>  		 * We have to guarantee mem->res.limit < mem->memsw.limit.
>  		 */
> -		spin_lock_irqsave(&memcg->res.lock, flags);
> -		memlimit = memcg->res.limit;
> +		mutex_lock(&set_limit_mutex);
> +		memlimit = res_counter_read_u64(&memcg->res, RES_LIMIT);
>  		if (memlimit > val) {
> -			spin_unlock_irqrestore(&memcg->res.lock, flags);
>  			ret = -EINVAL;
> +			mutex_unlock(&set_limit_mutex);
>  			break;
>  		}
>  		ret = res_counter_set_limit(&memcg->memsw, val);
> -		oldusage = memcg->memsw.usage;
> -		spin_unlock_irqrestore(&memcg->res.lock, flags);
> +		mutex_unlock(&set_limit_mutex);
>
>  		if (!ret)
>  			break;
> +
> +		oldusage = res_counter_read_u64(&memcg->memsw, RES_USAGE);
>  		try_to_free_mem_cgroup_pages(memcg, GFP_HIGHUSER_MOVABLE, true);
>  		curusage = res_counter_read_u64(&memcg->memsw, RES_USAGE);
>  		if (curusage >= oldusage)
>
> --
> 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>
>


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