Re: [PATCH v2] mm, memcg: fix error return value of mem_cgroup_css_alloc()

[Michal Hocko <mhocko@kernel.org>]

On Mon 06-04-20 23:48:54, Yafang Shao wrote:
> When I run my memcg testcase which creates lots of memcgs, I found
> there're unexpected out of memory logs while there're still enough
> available free memory. The error log is,
> mkdir: cannot create directory 'foo.65533': Cannot allocate memory
> 
> The reason is when we try to create more than MEM_CGROUP_ID_MAX memcgs, an
> -ENOMEM errno will be set by mem_cgroup_css_alloc(), but the right errno
> should be -EBUSY "Device or resource busy". That is same with
> memcg_alloc_cache_id().

I do not see EBUSY being listed as expected return value for mkdir(2)
which is the primary way to create a cgroup.

> As the errno really misled me, we should make it right. After this patch,
> the error log will be,
> mkdir: cannot create directory 'foo.65533': Device or resource busy

I do see ENOMEM being slightly confusing but if we really need to fix
this then ENOSPC sounds like a better fit to me.

man page says
ENOSPC The new directory cannot be created because the user's disk quota is exhausted.

and that actually matches because id is a form of a quota.
 
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  mm/memcontrol.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index ca194864d802..94319e4dd1bd 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2717,8 +2717,12 @@ static int memcg_alloc_cache_id(void)
>  
>  	id = ida_simple_get(&memcg_cache_ida,
>  			    0, MEMCG_CACHES_MAX_SIZE, GFP_KERNEL);
> -	if (id < 0)
> +	if (id < 0) {
> +		if (id == -ENOSPC)
> +			id = -EBUSY;
> +
>  		return id;
> +	}
>  
>  	if (id < memcg_nr_cache_ids)
>  		return id;
> @@ -4986,19 +4990,26 @@ static struct mem_cgroup *mem_cgroup_alloc(void)
>  	unsigned int size;
>  	int node;
>  	int __maybe_unused i;
> +	long error = -ENOMEM;
>  
>  	size = sizeof(struct mem_cgroup);
>  	size += nr_node_ids * sizeof(struct mem_cgroup_per_node *);
>  
>  	memcg = kzalloc(size, GFP_KERNEL);
>  	if (!memcg)
> -		return NULL;
> +		return ERR_PTR(error);
>  
>  	memcg->id.id = idr_alloc(&mem_cgroup_idr, NULL,
>  				 1, MEM_CGROUP_ID_MAX,
>  				 GFP_KERNEL);
> -	if (memcg->id.id < 0)
> +	if (memcg->id.id < 0) {
> +		if (memcg->id.id == -ENOSPC)
> +			error = -EBUSY;
> +		else
> +			error = memcg->id.id;
> +
>  		goto fail;
> +	}
>  
>  	memcg->vmstats_local = alloc_percpu(struct memcg_vmstats_percpu);
>  	if (!memcg->vmstats_local)
> @@ -5042,7 +5053,7 @@ static struct mem_cgroup *mem_cgroup_alloc(void)
>  fail:
>  	mem_cgroup_id_remove(memcg);
>  	__mem_cgroup_free(memcg);
> -	return NULL;
> +	return ERR_PTR(error);
>  }
>  
>  static struct cgroup_subsys_state * __ref
> @@ -5053,8 +5064,8 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
>  	long error = -ENOMEM;
>  
>  	memcg = mem_cgroup_alloc();
> -	if (!memcg)
> -		return ERR_PTR(error);
> +	if (IS_ERR(memcg))
> +		return ERR_CAST(memcg);
>  
>  	WRITE_ONCE(memcg->high, PAGE_COUNTER_MAX);
>  	memcg->soft_limit = PAGE_COUNTER_MAX;
> @@ -5104,7 +5115,7 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
>  fail:
>  	mem_cgroup_id_remove(memcg);
>  	mem_cgroup_free(memcg);
> -	return ERR_PTR(-ENOMEM);
> +	return ERR_PTR(error);
>  }
>  
>  static int mem_cgroup_css_online(struct cgroup_subsys_state *css)
> -- 
> 2.18.1
> 

-- 
Michal Hocko
SUSE Labs