Re: [PATCH] mm/slub: refill prefilled sheaves from the barn

[Hao Li]

On Fri, Sep 18, 2026 at 04:52:43PM +0100, Harry Yoo wrote:
> On Fri, Sep 18, 2026 at 04:35:17PM +0100, Harry Yoo wrote:
> > On Fri, Sep 18, 2026 at 07:41:56PM +0800, Hao Li wrote:
> > > +/*
> > > + * Exchange @sheaf, which holds fewer objects than requested, for a full one,
> > > + * keeping the leftover objects in the barn's partial sheaf instead of
> > > + * flushing them.
> > > + *
> > > + * Returns a full sheaf, or NULL if the barn cannot make one.
> > > + * The returned sheaf might be @sheaf itself or a new one.
> > > + */
> > > +static struct slab_sheaf *barn_replace_partial_sheaf(struct kmem_cache *s,
> > > +						     struct node_barn *barn,
> > > +						     struct slab_sheaf *sheaf)
> > > +{
> > > +	struct slab_sheaf *full = NULL, *partial;
> > > +	unsigned int to_move;
> > > +	unsigned long flags;
> > > +
> > > +	if (!data_race(barn->nr_full) && !data_race(barn->sheaf_partial))
> > > +		return NULL;
> > > +
> > > +	spin_lock_irqsave(&barn->lock, flags);
> > > +
> > > +	partial = barn->sheaf_partial;
> > > +	if (partial && partial->size + sheaf->size >= s->sheaf_capacity) {
> > > +		/* Fill the larger one to capacity from the smaller */
> > > +		if (partial->size > sheaf->size)
> > > +			swap(partial, sheaf);
> > 
> > Hmm but why switch sheaves when we don't have to?
> > Sounds like we're losing cache affinity unnecessarily.
> > 
> > I think we should try to refill from barn->sheaf_partial,
> > or if that's not available, refill from a full sheaf, and then move
> > the previously-full-sheaf to barn->sheaf_partial or barn->sheaf_empty.
> > 
> > Then we'll never replace the sheaf with a new one.
> > 
> > With that, the control flow could be simplified quite a bit.
> > Something like this. (Warning: pseudocode, it won't compile)
> > 
> > // refill a sheaf from barn.
> > // return true when the sheaf becomes full
> > // return false when the sheaf is not full
> > 
> > bool refill_sheaf_from_barn(s, sheaf) {
> > 	struct node_barn *barn = get_barn(s);
> > 	struct slab_sheaf *partial;
> > 	unsigned int to_move;
> > 	unsigned long flags;
> > 
> > 	spin_lock_irqsave(&barn->lock, flags);
> > 
> > 	partial = barn->sheaf_partial;
> > 	barn->sheaf_partial = NULL;
> > 
> > 	if (!partial && barn->nr_full) {
> > 		// grab one from full list
> > 		partial = [...];
> > 	}
> > 
> > 	if (!partial)
> > 		// cannot refill from the barn. the caller will try
> > 		// refilling from n->partial list
> > 		goto done;
> > 
> > 	to_move = min(s->sheaf_capacity - sheaf->size, partial->size);
> > 	partial->size -= to_move;
> > 	// copy `to_move` objects from `partial` to `sheaf`
> > 	memcpy(...);
> > 	sheaf->size += to_move;

Thanks! Make sense and I like this simple and straightforward approach.

When writing the patch, I was too focused on trying to avoid memcpy, or at
least minimizing the data to copy if it was unavoidable. However, I didn't
actually measure first whether memcpy makes any noticeable performance
difference. :P

The approach above is very intuitive, and as long as performance holds up, I
completely agree with taking the simpler way. I went ahead and tested it, and
the performance numbers are basically on par with the current patch. So I'll
switch to this cleaner approach in v2!

> 
> Hmm, but if it's from barn->sheaf_partial, it might end up refilling
> the sheaf from n->partial. Needs bit more thoughts. Perhaps retry if
> it's still not full?

Right, there are mainly two cases here. First, sheaf_partial might not have
enough objects. Second, a full sheaf obtained from the barn might not actually
be completely full, as noted in the comment of rcu_free_sheaf().

So we need a loop to keep going until @sheaf is completely filled up.

Also, I thought maybe we don't need to fill @sheaf completely, and only need to
fill it to the requested size. But the actual test showed that the performance
was not as good as expected :/

The resulting code looks something like this:

spin_lock_irqsave(&barn->lock, flags);

while (sheaf->size < s->sheaf_capacity) {
	src = barn->sheaf_partial;
	barn->sheaf_partial = NULL;
	if (!src) {
		if (!barn->nr_full)
			break;
		src = list_first_entry(&barn->sheaves_full,
				       struct slab_sheaf, barn_list);
		list_del(&src->barn_list);
		barn->nr_full--;
	}

	to_move = min(s->sheaf_capacity - sheaf->size, src->size);
	src->size -= to_move;
	memcpy(&sheaf->objects[sheaf->size], &src->objects[src->size],
	       to_move * sizeof(void *));
	sheaf->size += to_move;

	if (src->size) {
		barn->sheaf_partial = src;
	} else {
		/*
		 * No empty-limit check: the sheaf put on the empty list
		 * was already in the barn, so the barn holds no more
		 * sheaves than before. barn_replace_empty_sheaf() skips
		 * the check for the same reason.
		 */
		list_add(&src->barn_list, &barn->sheaves_empty);
		barn->nr_empty++;
	}
}

spin_unlock_irqrestore(&barn->lock, flags);

if (sheaf->size < s->sheaf_capacity)
	return false;

-- 
Thanks,
Hao