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

[Yafang Shao <laoar.shao@gmail.com>]

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