Re: [PATCH v3] mm/slub: defer freelist construction until after bulk allocation from a new slab

[hu.shengming]

Harry wrote:
> > > > -        slab->inuse++;
> > > >          allocated++;
> > > >      }
> > > > -    slab->freelist = object;
> > > > +    slab->inuse = target_inuse;
> > > >  
> > > >      if (needs_add_partial) {
> > > > -
> > > > +        build_slab_freelist(s, slab, &iter);
> > > 
> > > When allow_spin is true, it's building the freelist while holding the
> > > spinlock, and that's not great.
> > > 
> > > Hmm, can we do better?
> > > 
> > > Perhaps just allocate object(s) from the slab and build the freelist
> > > with the objects left (if exists), but free the slab if allow_spin
> > > is false AND trylock fails, and accept the fact that the slab may not be
> > > fully free when it's freed due to trylock failure?
> > > 
> > > something like:
> > > 
> > > alloc_from_new_slab() {
> > >     needs_add_partial = (slab->objects > count);
> > >     target_inuse = needs_add_partial ? count : slab->objects;
> > > 
> > >     init_slab_obj_iter(s, slab, &iter, allow_spin);
> > >     while (allocated < target_inuse) {
> > >         p[allocated] = next_slab_obj(s, &iter);
> > >         allocated++;
> > >     }
> > >     slab->inuse = target_inuse;
> > > 
> > >     if (needs_add_partial) {
> > >         build_slab_freelist(s, slab, &iter);
> > >         n = get_node(s, slab_nid(slab))
> > >         if (allow_spin) {
> > >             spin_lock_irqsave(&n->list_lock, flags);
> > >         } else if (!spin_trylock_irqsave(&n->list_lock, flags)) {
> > >             /* 
> > >              * Unlucky, discard newly allocated slab.
> > >              * The slab is not fully free, but it's fine as
> > >              * objects are not allocated to users.
> > >              */
> > >             free_new_slab_nolock(s, slab);
> > >             return 0;
> > >         }
> > >         add_partial(n, slab, ADD_TO_HEAD);
> > >         spin_unlock_irqrestore(&n->list_lock, flags);
> > >     }
> > >     [...]
> > > }
> > > 
> > > And do something similar in alloc_single_from_new_slab() as well.
> > > 
> > 
> > Good point. I'll restructure the path so objects are emitted first, the leftover
> > freelist is built only if needed, and the slab is added to partial afterwards. For
> > the !allow_spin trylock failure case, I'll discard the new slab and return 0. I'll
> > do the same for the single-object path as well.
> 
> Thanks. Please keep in mind that you need to enable SLUB_TINY or slab_debug
> to test the alloc_single_from_new_slab() path.

Sure, will do. I’ll make sure to test the alloc_single_from_new_slab() path as well
by enabling CONFIG_SLUB_TINY or slab_debug to ensure it gets exercised.

--
With Best Regards,
Shengming