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

[hu.shengming]

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?

--
With Best Regards,
Shengming