Re: [PATCH 3/7] zsmalloc: switch handle type to struct zs_handle

[Yosry Ahmed <yosry@kernel.org>]

[..]
> diff --git a/include/linux/zsmalloc.h b/include/linux/zsmalloc.h
> index 5b7298a5026b..693c9484b638 100644
> --- a/include/linux/zsmalloc.h
> +++ b/include/linux/zsmalloc.h
> @@ -22,14 +22,15 @@ struct zs_pool_stats {
>  };
>  
>  struct zs_pool;
> +struct zs_handle;
>  struct scatterlist;
>  
>  struct zs_pool *zs_create_pool(const char *name);
>  void zs_destroy_pool(struct zs_pool *pool);
>  
> -unsigned long zs_malloc(struct zs_pool *pool, size_t size, gfp_t flags,
> -			const int nid);
> -void zs_free(struct zs_pool *pool, unsigned long obj);
> +struct zs_handle *zs_malloc(struct zs_pool *pool, size_t size, gfp_t flags,
> +			    const int nid);
> +void zs_free(struct zs_pool *pool, struct zs_handle *handle);
>  
>  size_t zs_huge_class_size(struct zs_pool *pool);
>  
> @@ -40,10 +41,10 @@ unsigned int zs_lookup_class_index(struct zs_pool *pool, unsigned int size);
>  
>  void zs_pool_stats(struct zs_pool *pool, struct zs_pool_stats *stats);
>  
> -void zs_obj_read_sg_begin(struct zs_pool *pool, unsigned long handle,
> +void zs_obj_read_sg_begin(struct zs_pool *pool, struct zs_handle *handle,
>  			  struct scatterlist *sg, size_t mem_len);
> -void zs_obj_read_sg_end(struct zs_pool *pool, unsigned long handle);
> -void zs_obj_write(struct zs_pool *pool, unsigned long handle,
> +void zs_obj_read_sg_end(struct zs_pool *pool, struct zs_handle *handle);
> +void zs_obj_write(struct zs_pool *pool, struct zs_handle *handle,
>  		  void *handle_mem, size_t mem_len);

I would squash patch 7's constification here.

>  
>  extern const struct movable_operations zsmalloc_mops;
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 7ef80e0da626..7d8aab56f5cd 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -264,6 +264,10 @@ struct link_free {
>  	};
>  };
>  
> +struct zs_handle {
> +	unsigned long __private obj;
> +};
> +
>  static struct kmem_cache *handle_cachep;
>  static struct kmem_cache *zspage_cachep;
>  
> @@ -438,16 +442,16 @@ static void init_deferred_free(struct zs_pool *pool) {}
>  static void SetZsPageMovable(struct zs_pool *pool, struct zspage *zspage) {}
>  #endif
>  
> -static unsigned long cache_alloc_handle(gfp_t gfp)
> +static struct zs_handle *cache_alloc_handle(gfp_t gfp)
>  {
>  	gfp = gfp & ~(__GFP_HIGHMEM | __GFP_MOVABLE);
>  
> -	return (unsigned long)kmem_cache_alloc(handle_cachep, gfp);
> +	return kmem_cache_alloc(handle_cachep, gfp);
>  }
>  
> -static void cache_free_handle(unsigned long handle)
> +static void cache_free_handle(struct zs_handle *handle)
>  {
> -	kmem_cache_free(handle_cachep, (void *)handle);
> +	kmem_cache_free(handle_cachep, handle);
>  }
>  
>  static struct zspage *cache_alloc_zspage(gfp_t gfp)
> @@ -466,9 +470,9 @@ static void cache_free_zspage(struct zspage *zspage)
>   * Pairs with READ_ONCE() in handle_to_obj(): zs_free() may read the
>   * handle locklessly, so prevent store tearing here.
>   */
> -static void record_obj(unsigned long handle, unsigned long obj)
> +static void record_obj(struct zs_handle *handle, unsigned long obj)
>  {
> -	WRITE_ONCE(*(unsigned long *)handle, obj);
> +	WRITE_ONCE(ACCESS_PRIVATE(handle, obj), obj);
>  }
>  
>  /* Protected by class->lock */
> @@ -817,13 +821,13 @@ static unsigned long location_to_obj(struct zpdesc *zpdesc, unsigned int obj_idx
>  	return obj;
>  }
>  
> -static unsigned long handle_to_obj(unsigned long handle)
> +static unsigned long handle_to_obj(struct zs_handle *handle)
>  {
> -	return READ_ONCE(*(unsigned long *)handle);
> +	return READ_ONCE(ACCESS_PRIVATE(handle, obj));
>  }
>  
>  static inline bool obj_allocated(struct zpdesc *zpdesc, void *obj,
> -				 unsigned long *phandle)
> +				 struct zs_handle **phandle)
>  {
>  	unsigned long handle;
>  	struct zspage *zspage = get_zspage(zpdesc);
> @@ -839,7 +843,7 @@ static inline bool obj_allocated(struct zpdesc *zpdesc, void *obj,
>  		return false;
>  
>  	/* Clear all tags before returning the handle */
> -	*phandle = handle & ~OBJ_TAG_MASK;
> +	*phandle = (struct zs_handle *)(handle & ~OBJ_TAG_MASK);
>  	return true;
>  }

Would it be cleaerer if obj_allocated() just returned the handle here? I
believe this can be done separately in a prep patch.