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

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

[Yafang Shao]

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().

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

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

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

[Matthew Wilcox]

On Mon, Apr 06, 2020 at 11:48:54PM +0800, Yafang Shao wrote:
> @@ -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;
> +	}

This seems more complex than my original suggestion?

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

[Yafang Shao]

On Tue, Apr 7, 2020 at 12:42 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Apr 06, 2020 at 11:48:54PM +0800, Yafang Shao wrote:
> > @@ -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;
> > +     }
>
> This seems more complex than my original suggestion?
>

I thought the (id < 0) is an unlikely case, writing it like this only
checks id once for the most common case, while your original
suggestion will check id twice if the compiler is not good enough.
But that is not a big problem, I can change it as you suggestion. Your
suggestion is more readable.


Thanks
Yafang

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

[Michal Hocko]

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

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

[Michal Hocko]

On Tue 07-04-20 08:36:24, Michal Hocko wrote:
> 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.

Btw. I have just checked 73f576c04b94 ("mm: memcontrol: fix cgroup
creation failure after many small jobs") and it explicitly talks about
ENOSPC being returned prior to this patch.
-- 
Michal Hocko
SUSE Labs