Re: [PATCH v3 14/15] mm/slab: remove __GFP_NO_OBJ_EXT usage from alloc_slab_obj_exts()

[Vlastimil Babka (SUSE)]

On 6/17/26 15:56, Harry Yoo wrote:
> 
> 
> On 6/15/26 8:54 PM, Vlastimil Babka (SUSE) wrote:
>> __GFP_NO_OBJ_EXT has limited scope within the slab allocator itself and
>> gfp flags are a scarce resource, unlike slab's alloc_flags.
>> 
>> Introduce SLAB_ALLOC_NO_RECURSE alloc flag that has the same intent as
>> __GFP_NO_OBJ_EXT but a more generic name, meaning that a kmalloc()
>> family function should not recurse into another kmalloc*() for the
>> purposes of allocating auxiliary structures (obj_ext arrays or sheaves).
>> 
>> First, replace the __GFP_NO_OBJ_EXT for allocating obj_ext arrays in
>> alloc_slab_obj_exts(). Make use of the newly added kmalloc_flags()
>> function, where we can pass alloc_flags with SLAB_ALLOC_NO_RECURSE
>> added. This will also pass through SLAB_ALLOC_NOLOCK so we don't need
>> to special case kmalloc_nolock() anymore.
>> 
>> Note that until now the kmalloc_nolock() ignored the incoming gfp flags
>> and hardcoded __GFP_ZERO | __GFP_NO_OBJ_EXT. But it's correct to pass on
>> the incoming gfp flags (only augmented with __GFP_ZERO), because if
>> alloc_flags contain SLAB_ALLOC_NOLOCK, the incoming gfp flags have to
>> be also compatible with it. However, we might have added __GFP_THISNODE
>> for opportunistic slab allocation, as pointed out by Hao Li, and
>> __GFP_COMP by allocate_slab() as pointed out by Shengming Hu. Solve this
>> by adding both flags to OBJCGS_CLEAR_MASK as it makes sense to strip
>> them anyway for non-kmalloc_nolock() allocations of sheaves or obj_ext
>> arrays as well.
>> 
>> To avoid recursion of sheaf -> obj_ext -> sheaf -> ... allocations at
>> this patch, until the next patch converts sheaves to
>> SLAB_ALLOC_NO_RECURSE, use both gfp and alloc_flags for obj_ext. The
>> next patch will remove the gfp part.
>> 
>> Link: https://patch.msgid.link/20260610-slab_alloc_flags-v2-15-7190909db118@xxxxxxxxxx
>> Signed-off-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>
>> ---
> 
> Looks good to me,
> Reviewed-by: Harry Yoo (Oracle) <harry@xxxxxxxxxx>

Thanks!
 
> With some comments below.
> 
> I was worried that perhaps replacing SLAB_ALLOC_NO_RECURSE with
> __GFP_NO_OBJ_EXT will create a cycle of
> 
> alloc_slab_obj_exts(SLAB_ALLOC_DEFAULT)
> -> kmalloc_flags(SLAB_ALLOC_NO_RECURSE)
> -> alloc_from_pcs(SLAB_ALLOC_NO_RECURSE)
> -> refill_objects(SLAB_ALLOC_DEFAULT)
> -> new_slab(SLAB_ALLOC_DEFAULT)
> -> account_slab(SLAB_ALLOC_DEFAULT)
> -> alloc_slab_obj_exts(SLAB_ALLOC_DEFAULT)
> 
> with __GFP_NO_OBJ_EXT, it would have been passed to refill_objects(),
> but SLAB_ALLOC_NO_RECURSE is not. However this cycle does not exist
> because alloc_slab_obj_exts() clears __GFP_ACCOUNT (as part of
> OBJCG_CLEAR_MASK) and memory profiling itself does not invoke
> alloc_slab_obj_exts() when allocating new slabs if SLAB_ACCOUNT is not
> set (which is interesting, by the way).

Hm yeah I think we should propagate alloc_flags to refill_objects() etc, to 
avoid later surprise. But can be done as a later cleanup.
 
> Also alloc_slab_obj_exts() propagating SLAB_ALLOC_NEW_SLAB to
> kmalloc_flags() is little bit confusing because it does not have any
> effect due to SLAB_ALLOC_NO_RECURSE.

OK let's address this one by this fixup:

diff --git a/mm/slub.c b/mm/slub.c
index fc5b8c85b690..dc4b4ae874ce 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2164,6 +2164,7 @@ int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
 {
 	const bool allow_spin = alloc_flags_allow_spinning(alloc_flags);
 	unsigned int objects = objs_per_slab(s, slab);
+	bool new_slab = alloc_flags & SLAB_ALLOC_NEW_SLAB;
 	unsigned long new_exts;
 	unsigned long old_exts;
 	struct slabobj_ext *vec;
@@ -2173,6 +2174,7 @@ int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
 	/* Prevent recursive extension vector allocation */
 	gfp |= __GFP_NO_OBJ_EXT;
 	alloc_flags |= SLAB_ALLOC_NO_RECURSE;
+	alloc_flags &= ~SLAB_ALLOC_NEW_SLAB;
 
 	sz = obj_exts_alloc_size(s, slab, gfp);
 
@@ -2203,7 +2205,7 @@ int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
 	old_exts = READ_ONCE(slab->obj_exts);
 	handle_failed_objexts_alloc(old_exts, vec, objects);
 
-	if (alloc_flags & SLAB_ALLOC_NEW_SLAB) {
+	if (new_slab) {
 		/*
 		 * If the slab is brand new and nobody can yet access its
 		 * obj_exts, no synchronization is required and obj_exts can