Re: [PATCH v4] mm/slub: use empty sheaf helpers for oversized sheaves

[hu.shengming]

VlastimilBabka(SUSE)<vbabka@xxxxxxxxxx> wrote:
>On 5/27/26 15:48, hu.shengming@xxxxxxxxxx wrote:
>> VlastimilBabka(SUSE)<vbabka@xxxxxxxxxx> wrote:
>>> On 5/26/26 17:24, hu.shengming@xxxxxxxxxx wrote:
>>> > From: Shengming Hu <hu.shengming@xxxxxxxxxx>
>>> > 
>>> > Oversized prefilled sheaves are allocated separately, but after they are
>>> > flushed they are empty sheaves as well. Release them through
>>> > free_empty_sheaf() instead of calling kfree() directly.
>>> > 
>>> > Use __alloc_empty_sheaf() for oversized prefilled sheaves as well, so
>>> > that they are allocated through the same helper as regular empty sheaves
>>> > and released through the matching helper.
>>> > 
>>> > Since oversized sheaves are now allocated and freed through the empty
>>> > sheaf helpers, SHEAF_ALLOC and SHEAF_FREE also account for oversized
>>> > sheaves. Update the stat comments accordingly.
>>> > 
>>> > This keeps the oversized and pfmemalloc prefill paths consistent after
>>> > flushing.
>>> > 
>>> > Signed-off-by: Shengming Hu <hu.shengming@xxxxxxxxxx>
>>> 
>>> Hmm, __alloc_empty_sheaf() does "gfp &= ~OBJCGS_CLEAR_MASK" which includes
>>> __GFP_NOFAIL. A caller of kmem_cache_prefill_sheaf() with __GFP_NOFAIL might
>>> not expect failure. The __GFP_NO_OBJ_EXT handling is also not necessary for
>>> prefilled sheaves.
>>> 
>>> Well maybe all the gfp handling could be simply moved to
>>> alloc_empty_sheaf()? The other caller of __alloc_empty_sheaf() is
>>> bootstrap_cache_sheaves() and I think it doesn't need it either.
>>> 
>> 
>> Thanks for the review!
>> 
>> I agree that clearing OBJCGS_CLEAR_MASK in __alloc_empty_sheaf()
>> is not right for the oversized prefill path. In particular, it changes
>> the caller-provided GFP flags and can drop __GFP_NOFAIL. I will move that
>> part out of __alloc_empty_sheaf().
>> 
>> I am less sure about moving the __GFP_NO_OBJ_EXT handling out of
>> __alloc_empty_sheaf() as well, because that seems to make the free side
>> more complicated.
>> 
>> Currently free_empty_sheaf() assumes that sheaves for SLAB_KMALLOC caches
>> were allocated with __GFP_NO_OBJ_EXT, and therefore calls
>> mark_obj_codetag_empty() before kfree(). If __GFP_NO_OBJ_EXT is moved out
>> of __alloc_empty_sheaf(), then oversized prefilled sheaves allocated by
>> the raw helper would not have that flag anymore, while regular sheaves
>> allocated through alloc_empty_sheaf() would still have it.
>> 
>> In that case, the return path could no longer use one free helper for both
>> oversized and pfmemalloc sheaves after flushing. It would need to split
>> the two cases, for example:
>> 
>>         if (unlikely(sheaf->capacity != s->sheaf_capacity)) {
>>                 sheaf_flush_unused(s, sheaf);
>>                 __free_empty_sheaf(s, sheaf);
>>                 return;
>>         }
>> 
>>         if (unlikely(sheaf->pfmemalloc)) {
>>                 sheaf_flush_unused(s, sheaf);
>>                 free_empty_sheaf(s, sheaf);
>>                 return;
>>         }
>> 
>> There is also the bootstrap_cache_sheaves() case. The sheaves allocated
>> there are regular-sized sheaves. If such a sheaf later reaches
>> kmem_cache_return_sheaf(), it could not safely be released through
>> free_empty_sheaf() unless it was also allocated with __GFP_NO_OBJ_EXT,
>> because free_empty_sheaf() currently assumes that SLAB_KMALLOC sheaves
>> were allocated with __GFP_NO_OBJ_EXT and calls mark_obj_codetag_empty().
>> Unlike oversized sheaves, these bootstrap-created sheaves have the regular
>> capacity, so the return path cannot distinguish them by
>> sheaf->capacity != s->sheaf_capacity.
>> 
>> So I wonder if it would be better to move only the OBJCGS_CLEAR_MASK
>> clearing out of __alloc_empty_sheaf(), but keep the __GFP_NO_OBJ_EXT
>> handling there. That would fix the __GFP_NOFAIL issue for oversized
>> prefilled sheaves, while still keeping the allocation/free assumptions
>> consistent and allowing flushed oversized and pfmemalloc sheaves to use
>> the same free_empty_sheaf() path.
>> 
>> Would this approach be acceptable, or did I misunderstand your suggestion?
>
> I see, thanks. Could we perhaps move the mark_obj_codetag_empty() code to
> alloc_empty_sheaf()? If not, then do as you suggest?
>

I looked at moving mark_obj_codetag_empty() to alloc_empty_sheaf(), but I
don't think it is equivalent to doing it on the free path.

mark_obj_codetag_empty() only marks an entry if the slab already has
slabobj_exts. For sheaf metadata allocated with __GFP_NO_OBJ_EXT, the
allocation hook will not create the object extension. So if the slab has
no obj_exts at allocation time, an allocation-time mark would be a no-op.
A slabobj_ext can still be created later for the same kmalloc slab by
another allocation, and then freeing the sheaf without marking it on the
free path can still leave the sheaf entry with a NULL tag.

So I think the marking should stay in free_empty_sheaf(). I can still
move the caller-GFP filtering out of __alloc_empty_sheaf(). That includes
both clearing OBJCGS_CLEAR_MASK and handling the early rejection of
caller-supplied __GFP_NO_OBJ_EXT:

	if (gfp & __GFP_NO_OBJ_EXT)
		return NULL;

Moving this filtering to the caller would let the oversized prefill path
avoid losing __GFP_NOFAIL.But I would keep the SLAB_KMALLOC-specific
__GFP_NO_OBJ_EXT allocation and the matching free-time mark together.

Please let me know if you have a better suggestion.

--
With Best Regards,
Shengming