Re: [PATCH] memcg: refactor mem_cgroup_resize_limit()

[Nikolay Borisov]

On  2.06.2017 02:02, Yu Zhao wrote:
> mem_cgroup_resize_limit() and mem_cgroup_resize_memsw_limit() have
> identical logics. Refactor code so we don't need to keep two pieces
> of code that does same thing.
> 
> Signed-off-by: Yu Zhao <yuzhao@xxxxxxxxxx>
> ---
>  mm/memcontrol.c | 71 +++++++++------------------------------------------------
>  1 file changed, 11 insertions(+), 60 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 94172089f52f..a4f0daaff704 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2422,13 +2422,14 @@ static inline int mem_cgroup_move_swap_account(swp_entry_t entry,
>  static DEFINE_MUTEX(memcg_limit_mutex);
>  
>  static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
> -				   unsigned long limit)
> +				   unsigned long limit, bool memsw)
>  {
>  	unsigned long curusage;
>  	unsigned long oldusage;
>  	bool enlarge = false;
>  	int retry_count;
>  	int ret;
> +	struct page_counter *counter = memsw ? &memcg->memsw : &memcg->memory;
>  
>  	/*
>  	 * For keeping hierarchical_reclaim simple, how long we should retry
> @@ -2438,58 +2439,7 @@ static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
>  	retry_count = MEM_CGROUP_RECLAIM_RETRIES *
>  		      mem_cgroup_count_children(memcg);
>  
> -	oldusage = page_counter_read(&memcg->memory);
> -
> -	do {
> -		if (signal_pending(current)) {
> -			ret = -EINTR;
> -			break;
> -		}
> -
> -		mutex_lock(&memcg_limit_mutex);
> -		if (limit > memcg->memsw.limit) {
> -			mutex_unlock(&memcg_limit_mutex);
> -			ret = -EINVAL;
> -			break;
> -		}
> -		if (limit > memcg->memory.limit)
> -			enlarge = true;
> -		ret = page_counter_limit(&memcg->memory, limit);
> -		mutex_unlock(&memcg_limit_mutex);
> -
> -		if (!ret)
> -			break;
> -
> -		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, true);
> -
> -		curusage = page_counter_read(&memcg->memory);
> -		/* Usage is reduced ? */
> -		if (curusage >= oldusage)
> -			retry_count--;
> -		else
> -			oldusage = curusage;
> -	} while (retry_count);
> -
> -	if (!ret && enlarge)
> -		memcg_oom_recover(memcg);
> -
> -	return ret;
> -}
> -
> -static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
> -					 unsigned long limit)
> -{
> -	unsigned long curusage;
> -	unsigned long oldusage;
> -	bool enlarge = false;
> -	int retry_count;
> -	int ret;
> -
> -	/* see mem_cgroup_resize_res_limit */
> -	retry_count = MEM_CGROUP_RECLAIM_RETRIES *
> -		      mem_cgroup_count_children(memcg);
> -
> -	oldusage = page_counter_read(&memcg->memsw);
> +	oldusage = page_counter_read(counter);
>  
>  	do {
>  		if (signal_pending(current)) {
> @@ -2498,22 +2448,23 @@ static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
>  		}
>  
>  		mutex_lock(&memcg_limit_mutex);
> -		if (limit < memcg->memory.limit) {
> +		if (memsw ? limit < memcg->memory.limit :
> +			    limit > memcg->memsw.limit) {

No, just no. Please createa a local variable and use that. Using the
ternary operator in an 'if' statement is just ugly!

>  			mutex_unlock(&memcg_limit_mutex);
>  			ret = -EINVAL;
>  			break;
>  		}
> -		if (limit > memcg->memsw.limit)
> +		if (limit > counter->limit)
>  			enlarge = true;
> -		ret = page_counter_limit(&memcg->memsw, limit);
> +		ret = page_counter_limit(counter, limit);
>  		mutex_unlock(&memcg_limit_mutex);
>  
>  		if (!ret)
>  			break;
>  
> -		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, false);
> +		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, !memsw);
>  
> -		curusage = page_counter_read(&memcg->memsw);
> +		curusage = page_counter_read(counter);
>  		/* Usage is reduced ? */
>  		if (curusage >= oldusage)
>  			retry_count--;
> @@ -2975,10 +2926,10 @@ static ssize_t mem_cgroup_write(struct kernfs_open_file *of,
>  		}
>  		switch (MEMFILE_TYPE(of_cft(of)->private)) {
>  		case _MEM:
> -			ret = mem_cgroup_resize_limit(memcg, nr_pages);
> +			ret = mem_cgroup_resize_limit(memcg, nr_pages, false);
>  			break;
>  		case _MEMSWAP:
> -			ret = mem_cgroup_resize_memsw_limit(memcg, nr_pages);
> +			ret = mem_cgroup_resize_limit(memcg, nr_pages, true);
>  			break;
>  		case _KMEM:
>  			ret = memcg_update_kmem_limit(memcg, nr_pages);
>