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

[Vlastimil Babka (SUSE)]

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?

> 
> --
> With Best Regards,
> Shengming