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

["Vlastimil Babka (SUSE)" <vbabka@kernel.org>]

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.

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