Re: [RFC PATCH 1/2] mm/slub: make the case handling in __slab_free() easier to follow

[Hao Li]

On Fri, Sep 04, 2026 at 06:04:25PM +0200, Vlastimil Babka (SUSE) wrote:
> On 8/24/26 14:25, Hao Li wrote:
> > There are 7 possible transitions in __slab_free():
> > 
> >   a. partial->partial
> 
> maybe add "(offlist/onlist doesn't matter)"

Exactly, this should be split into two subcases. will do.

> 
> >   b. partial->empty, offlist
> >   c. partial->empty, onlist, exceeding min_partial
> >   d. partial->empty, onlist, not exceeding min_partial
> >   e. full->empty, exceeding min_partial
> >   f. full->empty, not exceeding min_partial
> >   g. full->partial
> > 
> > (There is no offlist variant of e, f and g as a full slab is on no
> > list.)
> > 
> > Clarify which case each branch handles, and replace the goto with a
> > return at the end of the skipped block so that every branch explicitly
> > states its coverage.
> > 
> > Case 'a' is the only path that needs neither list_lock nor list
> > handling. Give it an early continue: handling it upfront is much clearer
> > than forcing every other case into a nested block.
> > 
> > Also, read SL_partial once after the loop right where it is used, rather
> > than re-reading it on every iteration.
> > 
> > No functional change.
> > 
> > Signed-off-by: Hao Li <hao.li@xxxxxxxxx>
> 
> Reviewed-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>

Thanks!

> 
> Nit:
> 
> > ---
> >  mm/slub.c | 95 ++++++++++++++++++++++++++++---------------------------
> >  1 file changed, 49 insertions(+), 46 deletions(-)
> > 
> > diff --git a/mm/slub.c b/mm/slub.c
> > index b0cd0572e2f2..e20375307770 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -5748,76 +5748,79 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
> >  		new.inuse -= cnt;
> >  
> >  		/*
> > -		 * Might need to be taken off (due to becoming empty) or added
> > -		 * to (due to not being full anymore) the partial list.
> > -		 * Unless it's frozen.
> > +		 * partial->partial: the slab was on the node partial list and
> > +		 * stays there, so we need no list handling and no list_lock.
> 
> I think more accurate is "if the slab was on the node partial list, it stays
> there, and if it was off, it stays off, so we need no..." ?

Yes, this is clearer, will do!

> 
> > +		 *
> > +		 * Note that continue in a do-while goes on to evaluate the
> > +		 * condition below, so we do perform the freelist update.
> >  		 */
> > -		if (!new.inuse || was_full) {
> > -
> > -			n = get_node(s, slab_nid(slab));
> > -			/*
> > -			 * Speculatively acquire the list_lock.
> > -			 * If the cmpxchg does not succeed then we may
> > -			 * drop the list_lock without any processing.
> > -			 *
> > -			 * Otherwise the list_lock will synchronize with
> > -			 * other processors updating the list of slabs.
> > -			 */
> > -			spin_lock_irqsave(&n->list_lock, flags);
> > +		if (!was_full && new.inuse)
> > +			continue;
> >  
> > -			on_node_partial = slab_test_node_partial(slab);
> > -		}
> > +		/*
> > +		 * The slab might need to be taken off (due to becoming empty)
> > +		 * or added to (due to not being full anymore) the partial
> > +		 * list.
> > +		 *
> > +		 * Speculatively acquire list_lock before calling cmpxchg(), as
> > +		 * performing cmpxchg() prior to lock acquisition races with
> > +		 * concurrent paths, such as the shrinker.
> > +		 *
> > +		 * If the cmpxchg does not succeed then we will drop the
> > +		 * list_lock and retry.
> > +		 */
> > +		n = get_node(s, slab_nid(slab));
> > +		spin_lock_irqsave(&n->list_lock, flags);
> >  
> >  	} while (!slab_update_freelist(s, slab, &old, &new, "__slab_free"));
> >  
> >  	if (likely(!n)) {
> > +		/* partial->partial: we didn't take the list_lock */
> > +		return;
> > +	}
> > +
> > +	on_node_partial = slab_test_node_partial(slab);
> > +
> > +	if (!was_full && !on_node_partial) {
> >  		/*
> > -		 * We didn't take the list_lock because the slab was already on
> > -		 * the partial list and will remain there.
> > +		 * partial->empty, offlist: a bulk refill has taken the slab
> > +		 * off the partial list and will put it back, so its list
> > +		 * handling is not ours to do.
> >  		 */
> > +		spin_unlock_irqrestore(&n->list_lock, flags);
> >  		return;
> >  	}
> >  
> > -	/*
> > -	 * This slab was partially empty but not on the per-node partial list,
> > -	 * in which case we shouldn't manipulate its list, just return.
> > -	 */
> > -	if (!was_full && !on_node_partial) {
> > +	/* full/partial->empty, exceed: we have enough partial slabs already */
> > +	if (unlikely(!new.inuse && n->nr_partial >= s->min_partial)) {
> > +		/* partial->empty, onlist, exceed */
> > +		if (likely(!was_full)) {
> > +			remove_partial(n, slab);
> > +			stat(s, FREE_REMOVE_PARTIAL);
> > +		}
> > +		/* full->empty, exceed: it is on no list to remove from */
> > +
> >  		spin_unlock_irqrestore(&n->list_lock, flags);
> > +		stat(s, FREE_SLAB);
> > +		discard_slab(s, slab);
> >  		return;
> >  	}
> >  
> >  	/*
> > -	 * If slab became empty, should we add/keep it on the partial list or we
> > -	 * have enough?
> > +	 * At this point, only three cases remain:
> > +	 *   full->partial
> > +	 *   full->empty, not exceed
> > +	 *   partial->empty, onlist, not exceed
> >  	 */
> > -	if (unlikely(!new.inuse && n->nr_partial >= s->min_partial))
> > -		goto slab_empty;
> >  
> > -	/*
> > -	 * Objects left in the slab. If it was not on the partial list before
> > -	 * then add it.
> > -	 */
> > +	/* full->partial; full->empty, not exceed */
> >  	if (unlikely(was_full)) {
> >  		add_partial(n, slab, ADD_TO_TAIL);
> >  		stat(s, FREE_ADD_PARTIAL);
> >  	}
> > -	spin_unlock_irqrestore(&n->list_lock, flags);
> > -	return;
> > -
> > -slab_empty:
> > -	/*
> > -	 * The slab could have a single object and thus go from full to empty in
> > -	 * a single free, but more likely it was on the partial list. Remove it.
> > -	 */
> > -	if (likely(!was_full)) {
> > -		remove_partial(n, slab);
> > -		stat(s, FREE_REMOVE_PARTIAL);
> > -	}
> > +	/* partial->empty, onlist, not exceed: it stays where it is */
> >  
> >  	spin_unlock_irqrestore(&n->list_lock, flags);
> > -	stat(s, FREE_SLAB);
> > -	discard_slab(s, slab);
> >  }
> >  
> >  /*
> 

-- 
Thanks,
Hao