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 16:36, Vlastimil Babka (SUSE) wrote:
>  
>> 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.

It's also not a new hazard I think because while previously gfp flags with
__GFP_NO_OBJ_EXT would could be propagated more thoroughly than alloc_flags
for obj_exts only __alloc_tagging_slab_alloc_hook() looks at them, and
alloc_slab_obj_exts() (from account_slab()) didn't either, so the amount of
(finite) recursion is the same I think.

>> 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
>