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

[Vlastimil Babka (SUSE)]

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/872bd673-3d45-4111-8a41-31185db3ece5@xxxxxxxxxx
> Signed-off-by: Harry Yoo (Oracle) <harry@xxxxxxxxxx>

LGTM.

Reviewed-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>

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
> @@ -23,11 +23,27 @@
>  #define SLAB_ALLOC_NEW_SLAB	0x02 /* a flag for alloc_slab_obj_exts() */
>  #define SLAB_ALLOC_NO_RECURSE	0x04 /* prevent kmalloc() recursion */
>  
> +#define SLAB_FREE_DEFAULT	0x00 /* no flags */
> +#define SLAB_FREE_NOLOCK	0x01 /* spinning not allowed */
> +
> +static inline unsigned int to_alloc_flags(unsigned int free_flags)
> +{
> +	if (free_flags & SLAB_FREE_NOLOCK)
> +		return SLAB_ALLOC_NOLOCK;
> +	else
> +		return SLAB_ALLOC_DEFAULT;
> +}
> +
>  static inline bool alloc_flags_allow_spinning(const unsigned int alloc_flags)
>  {
>  	return !(alloc_flags & SLAB_ALLOC_NOLOCK);
>  }
>  
> +static inline bool free_flags_allow_spinning(const unsigned int free_flags)
> +{
> +	return !(free_flags & SLAB_FREE_NOLOCK);
> +}
> +
>  void *__kmalloc_flags_noprof(DECL_TOKEN_PARAMS(size, token), gfp_t flags,
>  				  unsigned int alloc_flags, int node)
>  				  __assume_kmalloc_alignment __alloc_size(1);
> @@ -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.
But AFAICS only one caller relies on it - __kfree_rcu_sheaf(). So maybe we
could just do it there?

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

>  {
>  	/*
>  	 * If the sheaf was created with SLAB_ALLOC_NO_RECURSE flag then its
> @@ -2829,11 +2833,20 @@ static void free_empty_sheaf(struct kmem_cache *s, struct slab_sheaf *sheaf)
>  		mark_obj_codetag_empty(sheaf);
>  
>  	VM_WARN_ON_ONCE(sheaf->size > 0);
> -	kfree(sheaf);
> +
> +	if (likely(allow_spin))
> +		kfree(sheaf);
> +	else
> +		kfree_nolock(sheaf);
>  
>  	stat(s, SHEAF_FREE);
>  }
>  
> +static void free_empty_sheaf(struct kmem_cache *s, struct slab_sheaf *sheaf)
> +{
> +	__free_empty_sheaf(s, sheaf, /* allow_spin = */ true);

Would avoid this ugliness.

> +}
> +
>  static unsigned int
>  refill_objects(struct kmem_cache *s, void **p, gfp_t gfp, unsigned int min,
>  	       unsigned int max);