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

[PATCH] mm, memcg: fix error return value of mem_cgroup_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 -ENOSPC, as explained above the function idr_alloc().

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': No space left on device

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 mm/memcontrol.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index ca19486..9c85470 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4980,7 +4980,7 @@ static void mem_cgroup_free(struct mem_cgroup *memcg)
 	__mem_cgroup_free(memcg);
 }
 
-static struct mem_cgroup *mem_cgroup_alloc(void)
+static struct mem_cgroup *mem_cgroup_alloc(long *error)
 {
 	struct mem_cgroup *memcg;
 	unsigned int size;
@@ -4997,8 +4997,10 @@ static struct mem_cgroup *mem_cgroup_alloc(void)
 	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) {
+		*error = memcg->id.id;
 		goto fail;
+	}
 
 	memcg->vmstats_local = alloc_percpu(struct memcg_vmstats_percpu);
 	if (!memcg->vmstats_local)
@@ -5052,7 +5054,7 @@ static struct mem_cgroup *mem_cgroup_alloc(void)
 	struct mem_cgroup *memcg;
 	long error = -ENOMEM;
 
-	memcg = mem_cgroup_alloc();
+	memcg = mem_cgroup_alloc(&error);
 	if (!memcg)
 		return ERR_PTR(error);
 
-- 
1.8.3.1

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

[Matthew Wilcox]

On Mon, Apr 06, 2020 at 08:54:07AM -0400, 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 -ENOSPC, as explained above the function idr_alloc().

I think idr_alloc() is wrong.  I think the right errno to return here is
EBUSY "Device or resource busy".

> -static struct mem_cgroup *mem_cgroup_alloc(void)
> +static struct mem_cgroup *mem_cgroup_alloc(long *error)

The normal way to do this is to return an ERR_PTR().  See
include/linux/err.h.

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

[Yafang Shao]

On Mon, Apr 6, 2020 at 9:05 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Apr 06, 2020 at 08:54:07AM -0400, 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 -ENOSPC, as explained above the function idr_alloc().
>
> I think idr_alloc() is wrong.  I think the right errno to return here is
> EBUSY "Device or resource busy".
>

Agree with you that EBUSY is better.
I will correct it.

> > -static struct mem_cgroup *mem_cgroup_alloc(void)
> > +static struct mem_cgroup *mem_cgroup_alloc(long *error)
>
> The normal way to do this is to return an ERR_PTR().  See
> include/linux/err.h.
>

Thanks for your advise. I will take a look at it.


Thanks
Yafang

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

[Yafang Shao]

On Mon, Apr 6, 2020 at 9:30 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Mon, Apr 6, 2020 at 9:05 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Mon, Apr 06, 2020 at 08:54:07AM -0400, 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 -ENOSPC, as explained above the function idr_alloc().
> >
> > I think idr_alloc() is wrong.  I think the right errno to return here is
> > EBUSY "Device or resource busy".
> >
>
> Agree with you that EBUSY is better.
> I will correct it.
>

Hi Matthew,

What about ida_alloc_range() ? Should we correct it as well ?


* Return: The allocated ID, or %-ENOMEM if memory could not be allocated,
* or %-ENOSPC if there are no free IDs.
*/
int ida_alloc_range(struct ida *ida, unsigned int min, unsigned int max,
                        gfp_t gfp)

If there're no free IDs, ida_alloc_range() also returns ENOSPC.

> > > -static struct mem_cgroup *mem_cgroup_alloc(void)
> > > +static struct mem_cgroup *mem_cgroup_alloc(long *error)
> >
> > The normal way to do this is to return an ERR_PTR().  See
> > include/linux/err.h.
> >
>
> Thanks for your advise. I will take a look at it.
>
>

Thanks
Yafang

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

[Matthew Wilcox]

On Mon, Apr 06, 2020 at 10:09:55PM +0800, Yafang Shao wrote:
> What about ida_alloc_range() ? Should we correct it as well ?
> 
> 
> * Return: The allocated ID, or %-ENOMEM if memory could not be allocated,
> * or %-ENOSPC if there are no free IDs.
> */
> int ida_alloc_range(struct ida *ida, unsigned int min, unsigned int max,
>                         gfp_t gfp)
> 
> If there're no free IDs, ida_alloc_range() also returns ENOSPC.

Do you want to check all the callers of ida_.*alloc()?

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

[Yafang Shao]

On Mon, Apr 6, 2020 at 10:11 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Apr 06, 2020 at 10:09:55PM +0800, Yafang Shao wrote:
> > What about ida_alloc_range() ? Should we correct it as well ?
> >
> >
> > * Return: The allocated ID, or %-ENOMEM if memory could not be allocated,
> > * or %-ENOSPC if there are no free IDs.
> > */
> > int ida_alloc_range(struct ida *ida, unsigned int min, unsigned int max,
> >                         gfp_t gfp)
> >
> > If there're no free IDs, ida_alloc_range() also returns ENOSPC.
>
> Do you want to check all the callers of ida_.*alloc()?

Sorry, I can't your point.

I just find the mem_cgroup_css_alloc() will use ida too.
mem_cgroup_css_alloc
    memcg_online_kmem
        memcg_alloc_cache_id
             ida_simple_get

I think we should keep the behavior consistent here.

Thanks
Yafang

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

[Matthew Wilcox]

On Mon, Apr 06, 2020 at 10:16:56PM +0800, Yafang Shao wrote:
> On Mon, Apr 6, 2020 at 10:11 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Mon, Apr 06, 2020 at 10:09:55PM +0800, Yafang Shao wrote:
> > > What about ida_alloc_range() ? Should we correct it as well ?
> > >
> > >
> > > * Return: The allocated ID, or %-ENOMEM if memory could not be allocated,
> > > * or %-ENOSPC if there are no free IDs.
> > > */
> > > int ida_alloc_range(struct ida *ida, unsigned int min, unsigned int max,
> > >                         gfp_t gfp)
> > >
> > > If there're no free IDs, ida_alloc_range() also returns ENOSPC.
> >
> > Do you want to check all the callers of ida_.*alloc()?
> 
> Sorry, I can't your point.

Changing the value returned by ida_alloc_range() would require checking
every caller to see if it would break anything.  That's why I didn't
change it when I rewrote it.

> I just find the mem_cgroup_css_alloc() will use ida too.
> mem_cgroup_css_alloc
>     memcg_online_kmem
>         memcg_alloc_cache_id
>              ida_simple_get
> 
> I think we should keep the behavior consistent here.

ENOSPC doesn't make much sense here.  So I'd do:

	id = ida_simple_get(&memcg_cache_ida,
                            0, MEMCG_CACHES_MAX_SIZE, GFP_KERNEL);
+	if (id == -ENOSPC)
+		return -EBUSY;
	if (id < 0)
		return id;

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

[Yafang Shao]

On Mon, Apr 6, 2020 at 10:25 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Apr 06, 2020 at 10:16:56PM +0800, Yafang Shao wrote:
> > On Mon, Apr 6, 2020 at 10:11 PM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Mon, Apr 06, 2020 at 10:09:55PM +0800, Yafang Shao wrote:
> > > > What about ida_alloc_range() ? Should we correct it as well ?
> > > >
> > > >
> > > > * Return: The allocated ID, or %-ENOMEM if memory could not be allocated,
> > > > * or %-ENOSPC if there are no free IDs.
> > > > */
> > > > int ida_alloc_range(struct ida *ida, unsigned int min, unsigned int max,
> > > >                         gfp_t gfp)
> > > >
> > > > If there're no free IDs, ida_alloc_range() also returns ENOSPC.
> > >
> > > Do you want to check all the callers of ida_.*alloc()?
> >
> > Sorry, I can't your point.
>
> Changing the value returned by ida_alloc_range() would require checking
> every caller to see if it would break anything.  That's why I didn't
> change it when I rewrote it.
>

Got it. Thanks for the  explaination.

> > I just find the mem_cgroup_css_alloc() will use ida too.
> > mem_cgroup_css_alloc
> >     memcg_online_kmem
> >         memcg_alloc_cache_id
> >              ida_simple_get
> >
> > I think we should keep the behavior consistent here.
>
> ENOSPC doesn't make much sense here.  So I'd do:
>
>         id = ida_simple_get(&memcg_cache_ida,
>                             0, MEMCG_CACHES_MAX_SIZE, GFP_KERNEL);
> +       if (id == -ENOSPC)
> +               return -EBUSY;
>         if (id < 0)
>                 return id;
>

Understood!

Thanks
Yafang