Re: [External] Re: [PATCH 3/3] mm/slub: Fix release all resources used by a slab cache

[Joonsoo Kim]

2020ë 6ì 15ì (ì) ìí 3:41, Muchun Song <songmuchun@xxxxxxxxxxxxx>ëì ìì:
>
> On Mon, Jun 15, 2020 at 2:23 PM Joonsoo Kim <js1304@xxxxxxxxx> wrote:
> >
> > 2020ë 6ì 14ì (ì) ìí 9:39, Muchun Song <songmuchun@xxxxxxxxxxxxx>ëì ìì:
> > >
> > > The function of __kmem_cache_shutdown() is that release all resources
> > > used by the slab cache, while currently it stop release resources when
> > > the preceding node is not empty.
> > >
> > > Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
> > > ---
> > >  mm/slub.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/mm/slub.c b/mm/slub.c
> > > index b73505df3de2..4e477ef0f2b9 100644
> > > --- a/mm/slub.c
> > > +++ b/mm/slub.c
> > > @@ -3839,6 +3839,7 @@ bool __kmem_cache_empty(struct kmem_cache *s)
> > >   */
> > >  int __kmem_cache_shutdown(struct kmem_cache *s)
> > >  {
> > > +       int ret = 0;
> > >         int node;
> > >         struct kmem_cache_node *n;
> > >
> > > @@ -3846,11 +3847,11 @@ int __kmem_cache_shutdown(struct kmem_cache *s)
> > >         /* Attempt to free all objects */
> > >         for_each_kmem_cache_node(s, node, n) {
> > >                 free_partial(s, n);
> > > -               if (node_nr_slabs(n))
> > > -                       return 1;
> > > +               if (!ret && node_nr_slabs(n))
> > > +                       ret = 1;
> > >         }
> >
> > I don't think that this is an improvement.
> >
> > If the shutdown condition isn't met, we don't need to process further.
> > Just 'return 1' looks okay to me.
> >
> > And, with this change, sysfs_slab_remove() is called even if the
> > shutdown is failed.
> > It's better not to have side effects when failing.
>
> If someone calls __kmem_cache_shutdown, he may want to release
> resources used by the slab cache as much as possible. If we continue,
> we may release more pages. From this point, is it an improvement?

My opinion is not strong but I still think that it's not useful enough
to complicate
the code.

If shutdown is failed, it implies there are some bugs and someone should fix it.
Releasing more resources would mitigate the resource problem but doesn't
change the situation that someone should fix it soon.

Anyway, I don't object more if you don't agree with my opinion. In that case,
please fix not to call sysfs_slab_remove() when ret is 1.

Thanks.