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

[Vlastimil Babka (SUSE)]

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)"

>   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>

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..." ?

> +		 *
> +		 * 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);
>  }
>  
>  /*