Re: [PATCH V5 4/8] mm/slab: abstract slabobj_ext access via new slab_obj_ext() helper

[Vlastimil Babka <vbabka@suse.cz>]

On 1/5/26 09:02, Harry Yoo wrote:
> Currently, the slab allocator assumes that slab->obj_exts is a pointer
> to an array of struct slabobj_ext objects. However, to support storage
> methods where struct slabobj_ext is embedded within objects, the slab
> allocator should not make this assumption. Instead of directly
> dereferencing the slabobj_exts array, abstract access to
> struct slabobj_ext via helper functions.
> 
> Introduce a new API slabobj_ext metadata access:
> 
>   slab_obj_ext(slab, obj_exts, index) - returns the pointer to
>   struct slabobj_ext element at the given index.
> 
> Directly dereferencing the return value of slab_obj_exts() is no longer
> allowed. Instead, slab_obj_ext() must always be used to access
> individual struct slabobj_ext objects.
> 
> Convert all users to use these APIs.
> No functional changes intended.
> 

> +/*
> + * slab_obj_ext - get the pointer to the slab object extension metadata
> + * associated with an object in a slab.
> + * @slab: a pointer to the slab struct
> + * @obj_exts: a pointer to the object extension vector
> + * @index: an index of the object
> + *
> + * Returns a pointer to the object extension associated with the object.
> + */
> +static inline struct slabobj_ext *slab_obj_ext(struct slab *slab,
> +					       unsigned long obj_exts,
> +					       unsigned int index)
> +{
> +	struct slabobj_ext *obj_ext;
> +
> +	VM_WARN_ON_ONCE(!slab_obj_exts(slab));
> +	VM_WARN_ON_ONCE(obj_exts != slab_obj_exts(slab));

The first check seems redundant given we have the second one? If we get
passed obj_ext 0 and slab_obj_exts() is also 0, it will blow up quickly anyway.

> +
> +	obj_ext = (struct slabobj_ext *)obj_exts;
> +	return &obj_ext[index];
>  }
>  
>  int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
> @@ -533,7 +558,13 @@ int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
>  
>  #else /* CONFIG_SLAB_OBJ_EXT */
>  
> -static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
> +static inline unsigned long slab_obj_exts(struct slab *slab)
> +{
> +	return 0;
> +}
> +
> +static inline struct slabobj_ext *slab_obj_ext(struct slab *slab,
> +					       unsigned int index)

Hmm this is missing the obj_exts parameter? Either will not compile
!CONFIG_SLAB_OBJ_EXT or isn't reachable in that config anyway?

>  {
>  	return NULL;
>  }
> @@ -550,7 +581,7 @@ static inline enum node_stat_item cache_vmstat_idx(struct kmem_cache *s)
>  bool __memcg_slab_post_alloc_hook(struct kmem_cache *s, struct list_lru *lru,
>  				  gfp_t flags, size_t size, void **p);
>  void __memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab,
> -			    void **p, int objects, struct slabobj_ext *obj_exts);
> +			    void **p, int objects, unsigned long obj_exts);
>  #endif
>  
>  void kvfree_rcu_cb(struct rcu_head *head);
> diff --git a/mm/slub.c b/mm/slub.c
> index 0e32f6420a8a..84bd4f23dc4a 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c

<snip>

> @@ -2176,7 +2178,7 @@ int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
>  
>  static inline void free_slab_obj_exts(struct slab *slab)
>  {
> -	struct slabobj_ext *obj_exts;
> +	unsigned long obj_exts;

I think in this function we could leave it as pointer.

>  	obj_exts = slab_obj_exts(slab);

And do a single cast here.

>  	if (!obj_exts) {
> @@ -2196,11 +2198,11 @@ static inline void free_slab_obj_exts(struct slab *slab)
>  	 * NULL, therefore replace NULL with CODETAG_EMPTY to indicate that
>  	 * the extension for obj_exts is expected to be NULL.
>  	 */
> -	mark_objexts_empty(obj_exts);
> +	mark_objexts_empty((struct slabobj_ext *)obj_exts);
>  	if (unlikely(READ_ONCE(slab->obj_exts) & OBJEXTS_NOSPIN_ALLOC))
> -		kfree_nolock(obj_exts);
> +		kfree_nolock((void *)obj_exts);
>  	else
> -		kfree(obj_exts);
> +		kfree((void *)obj_exts);
>  	slab->obj_exts = 0;
>  }

And avoid those 3 above.
Unless it gets more complicated with later patches...