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

On 7/22/26 4:37 PM, Vlastimil Babka (SUSE) wrote:
> On 7/22/26 09:28, Harry Yoo wrote:
>>> 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.
> 
> Thanks!
> 
>>>>  	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 :)
> 
> Yeah but I meant __free_empty_sheaf() only. It's fine to leave the wrapper
> without the new parameter for the most common case, like you did.

Ah, I see!
Right, that's reasonable and will do.

>> 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.
> 
> It doesn't have to be now :)

Yeah :)

Thanks!

-- 
Cheers,
Harry / Hyeonggon

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

iHUEARYKAB0WIQQQ1ub6gR5ogjaKRmOGXBN6rc5S1gUCamBzygAKCRCGXBN6rc5S
1mj6AQCEDuXSXD84vwoWMnvh3SAH6pQ7Ngs8xt8xR+bK8mwudgEAue6ODAgbBFy7
JAlMoRPioGz3QAzAlg9IcFHlo0egHwM=
=MCiF
-----END PGP SIGNATURE-----