Re: [PATCH] mm/slab: take n->list_lock for the list_add() in __refill_objects_node()

[Harry Yoo <harry@kernel.org>]

On Mon, Aug 31, 2026 at 02:55:06PM +0200, Vlastimil Babka (SUSE) wrote:
> On 8/30/26 16:35, Hao Li wrote:
> > On Sun, Aug 30, 2026 at 12:45:17PM +0000, Harry Yoo wrote:
> >> On Sun, Aug 30, 2026 at 04:25:45PM +0900, Hyunwoo Kim wrote:
> > Since introducing a new variable seems unavoidable, what if we temporarily
> > stash this slab in a pointer like below, and then add it to pc.slabs once we
> > acquire the lock.
> > 
> > struct slab *leftover_slab = NULL;
> > 
> > ...
> > ...
> > 	if (__slab_try_return_freelist(s, slab, head, count)) {
> > 		leftover_slab = slab;
> > 		break;
> > 	}
> > 
> > ...
> > ...
> > if (!list_empty(&pc.slabs)) {
> > 	spin_lock_irqsave(&n->list_lock, flags);
> > 
> > 	if (leftover_slab)
> > 		list_add(&leftover_slab->slab_list, &pc.slabs);
> > 	...
> > 	...
> > }
> > 
> > PS: If I recall correctly, Vlastimil's initial patch was actually fine. It was
> > my suggestion to save an extra lock/unlock pair that accidentally led to this
> > trap...
> 
> Ah, thanks for the reminder. This [1] was the original attempt.
> 
> [1]
> https://lore.kernel.org/all/20260421-b4-refill-optimistic-return-v1-1-24f0bfc1acff@kernel.org/
> 
> I wonder if the fix should be to return to that approach and just have
> __slab_try_return_freelist() handle the list_lock. The code would be simpler
> with not "bool locked".
> 
> It should be really very rare that we would end up returning a partial list
> and also have additional slabs to return on pc.slabs? So I think there would
> be no noticeable performance downside to the simpler code potentially ending
> up taking the list_lock twice instead of once.

Agreed that it should be rare and not worth the complexity unless
we have data to support that.

Hyunwoo, would you please adjust the feedback and post v2?

-- 
Cheers,
Harry / Hyeonggon