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

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

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