Re: [PATCH slab/for-next v4 2/8] mm/slab: handle the !allow_spin case in kfree_rcu_sheaf()

On 7/21/26 7:07 PM, Vlastimil Babka (SUSE) wrote:
> On 7/20/26 14:44, Harry Yoo (Oracle) wrote:
>> Teach kfree_rcu_sheaf() how to handle the !allow_spin case. Try to get
>> an empty sheaf from pcs->spare or the barn even when spinning is not
>> allowed. Unlike __pcs_replace_full_main(), try harder to allocate
>> an empty sheaf because the fallback path will be more expensive than
>> kfree_nolock().
>>
>> Now that slab has internal alloc_flags to describe context, introduce
>> free_flags analogously and convert free_flags to alloc_flags when
>> allocating memory in the free path. alloc_empty_sheaf() now strips
>> __GFP_RECLAIM when SLAB_ALLOC_NOLOCK is specified.
>>
>> When trylock fails or the kernel observes non-NULL pcs->rcu_free after
>> lock acquisition, free the sheaf instead of putting it to the barn.
>> This is rare and not worth complicating the code.
>>
>> Since call_rcu() cannot be called in an unknown context,
>> kfree_rcu_sheaf() fails when the rcu sheaf becomes full.
>>
>> Link: https://lore.kernel.org/linux-mm/[email protected]
>> Signed-off-by: Harry Yoo (Oracle) <[email protected]>
> 
> LGTM.
> 
> Reviewed-by: Vlastimil Babka (SUSE) <[email protected]>

Thanks a lot for reviewing, Vlastimil!

> Nits below:
> 
>> ---
>>  mm/slab.h        | 18 +++++++++++++++++-
>>  mm/slab_common.c |  2 +-
>>  mm/slub.c        | 36 ++++++++++++++++++++++++++++--------
>>  3 files changed, 46 insertions(+), 10 deletions(-)
>>
>> diff --git a/mm/slab.h b/mm/slab.h
>> index 281a65233795..85ef2ebc9812 100644
>> --- a/mm/slab.h
>> +++ b/mm/slab.h
>> @@ -429,7 +445,7 @@ static inline bool is_kmalloc_normal(struct kmem_cache *s)
>>  	return !(s->flags & (SLAB_CACHE_DMA|SLAB_ACCOUNT|SLAB_RECLAIM_ACCOUNT));
>>  }
>>  
>> -bool __kfree_rcu_sheaf(struct kmem_cache *s, void *obj);
>> +bool __kfree_rcu_sheaf(struct kmem_cache *s, void *obj, unsigned int free_flags);
>>  void flush_all_rcu_sheaves(void);
>>  void flush_rcu_sheaves_on_cache(struct kmem_cache *s);
>>  
>> diff --git a/mm/slab_common.c b/mm/slab_common.c
>> index b6426d7ceec9..e07b4e6d6679 100644
>> --- a/mm/slab_common.c
>> +++ b/mm/slab_common.c
>> @@ -1605,7 +1605,7 @@ static bool kfree_rcu_sheaf(void *obj)
>>  
>>  	s = slab->slab_cache;
>>  	if (likely(!IS_ENABLED(CONFIG_NUMA) || slab_nid(slab) == numa_mem_id()))
>> -		return __kfree_rcu_sheaf(s, obj);
>> +		return __kfree_rcu_sheaf(s, obj, SLAB_FREE_DEFAULT);
>>  
>>  	return false;
>>  }
>> diff --git a/mm/slub.c b/mm/slub.c
>> index e32a68677537..0c350274fbff 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -2814,10 +2814,14 @@ static inline struct slab_sheaf *alloc_empty_sheaf(struct kmem_cache *s,
>>  
>>  	gfp &= ~OBJCGS_CLEAR_MASK;
>>  
>> +	if (alloc_flags & SLAB_ALLOC_NOLOCK)
>> +		gfp &= ~__GFP_RECLAIM;
> 
> So in general we expect gfp and alloc flags to be compatible and warn if
> they are not. This now performs an auto-adjustment, which makes it unusual.

That's fair.

> But AFAICS only one caller relies on it - __kfree_rcu_sheaf(). So maybe we
> could just do it there?

Will do. I don't have strong preference on this.

>>  	return __alloc_empty_sheaf(s, gfp, alloc_flags, s->sheaf_capacity);
>>  }
>>  
>> -static void free_empty_sheaf(struct kmem_cache *s, struct slab_sheaf *sheaf)
>> +static void __free_empty_sheaf(struct kmem_cache *s, struct slab_sheaf *sheaf,
>> +			       bool allow_spin)
> 
> Why not free_flags instead of allow_spin? Since you already introduced them.

Indeed I tried that but gave up on doing that as part of series
after realizing free_empty_sheaf() alone has 12 callers :)

But I think it's worth teaching those functions (including
free_empty_sheaf()) to handle SLAB_ALLOC_* and SLAB_FREE_* flags
rather than propagating allow_spin.

-- 
Cheers,
Harry / Hyeonggon

-----BEGIN PGP SIGNATURE-----

iHUEARYKAB0WIQQQ1ub6gR5ogjaKRmOGXBN6rc5S1gUCamBxLQAKCRCGXBN6rc5S
1lmSAP9lYDgvsDB/HErz8AaTnBiLvXlODJbuM/n6AnzN174nqQD/e4Fyvp2AXL7v
i1a62GaFBxE4ZzdiDFmrY9G+9EoQZgs=
=AE3u
-----END PGP SIGNATURE-----