[PATCHv4,6/9] zsmalloc: pass limit on pages per-zspage to zs_create_pool()

  Allow zsmalloc pool owner to specify max number of pages
per-zspage (during pool creation), so that different pools
can have different characteristics.

By default we pass ZS_DEFAULT_PAGES_PER_ZSPAGE which is 4
(matches the current order 2 zspages limit).

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 drivers/block/zram/zram_drv.c |  3 ++-
 include/linux/zsmalloc.h      |  2 +-
 mm/zsmalloc.c                 | 11 +++++++----
 3 files changed, 10 insertions(+), 6 deletions(-)
  

On (22/10/31 14:41), Sergey Senozhatsky wrote:
[..]
> -struct zs_pool *zs_create_pool(const char *name)
> +struct zs_pool *zs_create_pool(const char *name, unsigned long num_pages)
>  {
>  	int i;
>  	struct zs_pool *pool;
>  	struct size_class *prev_class = NULL;
> -	unsigned long num_pages;
> +
> +	if (WARN_ON(num_pages < ZS_MIN_PAGES_PER_ZSPAGE ||
> +		    num_pages > ZS_MAX_PAGES_PER_ZSPAGE))
> +		return NULL;

I tend to think that creating `struct zs_tunables` would be better. For
the time being zs_tunables will contain only one member max_zspage_len,
but it can be extended in the future.
  

On Mon, Oct 31, 2022 at 02:41:05PM +0900, Sergey Senozhatsky wrote:
> Allow zsmalloc pool owner to specify max number of pages
> per-zspage (during pool creation), so that different pools
> can have different characteristics.
> 
> By default we pass ZS_DEFAULT_PAGES_PER_ZSPAGE which is 4
> (matches the current order 2 zspages limit).

How could user decide what's the best size for their workload?

> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  drivers/block/zram/zram_drv.c |  3 ++-
>  include/linux/zsmalloc.h      |  2 +-
>  mm/zsmalloc.c                 | 11 +++++++----
>  3 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 90b0c66bbd5b..bec02f636bce 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -1247,7 +1247,8 @@ static bool zram_meta_alloc(struct zram *zram, u64 disksize)
>  	if (!zram->table)
>  		return false;
>  
> -	zram->mem_pool = zs_create_pool(zram->disk->disk_name);
> +	zram->mem_pool = zs_create_pool(zram->disk->disk_name,
> +					ZS_DEFAULT_PAGES_PER_ZSPAGE);
>  	if (!zram->mem_pool) {
>  		vfree(zram->table);
>  		return false;
> diff --git a/include/linux/zsmalloc.h b/include/linux/zsmalloc.h
> index b6b8654a2d45..28f2b9cb1c47 100644
> --- a/include/linux/zsmalloc.h
> +++ b/include/linux/zsmalloc.h
> @@ -50,7 +50,7 @@ struct zs_pool_stats {
>  
>  struct zs_pool;
>  
> -struct zs_pool *zs_create_pool(const char *name);
> +struct zs_pool *zs_create_pool(const char *name, unsigned long num_pages);
>  void zs_destroy_pool(struct zs_pool *pool);
>  
>  unsigned long zs_malloc(struct zs_pool *pool, size_t size, gfp_t flags);
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index d329bd673baa..42987a913f45 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -366,7 +366,7 @@ static void *zs_zpool_create(const char *name, gfp_t gfp,
>  	 * different contexts and its caller must provide a valid
>  	 * gfp mask.
>  	 */
> -	return zs_create_pool(name);
> +	return zs_create_pool(name, ZS_DEFAULT_PAGES_PER_ZSPAGE);
>  }
>  
>  static void zs_zpool_destroy(void *pool)
> @@ -2195,6 +2195,7 @@ static int zs_register_shrinker(struct zs_pool *pool)
>  /**
>   * zs_create_pool - Creates an allocation pool to work from.
>   * @name: pool name to be created
> + * @num_pages: maximum number of pages per-zspage

How about "max_page_chain:"? 

>   *
>   * This function must be called before anything when using
>   * the zsmalloc allocator.
> @@ -2202,18 +2203,20 @@ static int zs_register_shrinker(struct zs_pool *pool)
>   * On success, a pointer to the newly created pool is returned,
>   * otherwise NULL.
>   */
> -struct zs_pool *zs_create_pool(const char *name)
> +struct zs_pool *zs_create_pool(const char *name, unsigned long num_pages)
>  {
>  	int i;
>  	struct zs_pool *pool;
>  	struct size_class *prev_class = NULL;
> -	unsigned long num_pages;
> +
> +	if (WARN_ON(num_pages < ZS_MIN_PAGES_PER_ZSPAGE ||
> +		    num_pages > ZS_MAX_PAGES_PER_ZSPAGE))
> +		return NULL;
>  
>  	pool = kzalloc(sizeof(*pool), GFP_KERNEL);
>  	if (!pool)
>  		return NULL;
>  
> -	num_pages = ZS_DEFAULT_PAGES_PER_ZSPAGE;
>  	/* min_alloc_size must be multiple of ZS_ALIGN */
>  	pool->min_alloc_size = num_pages << PAGE_SHIFT >> OBJ_INDEX_BITS;
>  	pool->min_alloc_size = max(pool->min_alloc_size, ZS_MIN_ALLOC_SIZE);
> -- 
> 2.38.1.273.g43a17bfeac-goog
>
  

On (22/11/10 18:10), Minchan Kim wrote:
> On Mon, Oct 31, 2022 at 02:41:05PM +0900, Sergey Senozhatsky wrote:
> > Allow zsmalloc pool owner to specify max number of pages
> > per-zspage (during pool creation), so that different pools
> > can have different characteristics.
> > 
> > By default we pass ZS_DEFAULT_PAGES_PER_ZSPAGE which is 4
> > (matches the current order 2 zspages limit).
> 
> How could user decide what's the best size for their workload?

[..]

For starters in a similar manner that I showed during our meeting.
They can run tests, gather stats (zsmalloc objects distribution),
analyze where most of the objects sit, how things change when we
have different cluster configurations, and so on.

But more importantly: they need lots of zramX mm_stat data, which is
perfectly traceable and collectable during fleet A/B testing: when a
number of devices get randomly assigned to different experiments and
receive different zspage len configuration, which they feed to zram
sysfs knobs during system startup (when init script configures zram).
And then look at statistically significant improvements or regressions.

This is how things done in ChromeOS and I'm sure in many other places.

In this regard, finding best zspage len value is not any different from
finding what is the best zram disksize, or what is the best compression
algorithm. Exactly same approach - feed different configuration to devices
and then analyze the data. Look at mm_stat-s before and after experiment,
per device class/type.

We can discuss in more details internally.

> >  static void zs_zpool_destroy(void *pool)
> > @@ -2195,6 +2195,7 @@ static int zs_register_shrinker(struct zs_pool *pool)
> >  /**
> >   * zs_create_pool - Creates an allocation pool to work from.
> >   * @name: pool name to be created
> > + * @num_pages: maximum number of pages per-zspage
> 
> How about "max_page_chain:"? 

OK.

Do you dislike idea of creating a `struct zs_tunables` which will hold
all fields that we can tune? And then zsmalloc users can pass that
struct (a pointer to) to zs_create_pool().

There can be various tunables. Like policy changes: do we use static
zspool configuration, or a dynamic one and so on.

On zram side, we can have a generic sysfs knob: allocator_tuning,
which will accept named params, the same way we did it for
recomp_algorithm and recompress.

	echo "tuneable=VAL tunealbe=VAL" > /sys/block/zramX/allocator_tuning
  

On Wed, Nov 09, 2022 at 03:24:43PM +0900, Sergey Senozhatsky wrote:
> On (22/10/31 14:41), Sergey Senozhatsky wrote:
> [..]
> > -struct zs_pool *zs_create_pool(const char *name)
> > +struct zs_pool *zs_create_pool(const char *name, unsigned long num_pages)
> >  {
> >  	int i;
> >  	struct zs_pool *pool;
> >  	struct size_class *prev_class = NULL;
> > -	unsigned long num_pages;
> > +
> > +	if (WARN_ON(num_pages < ZS_MIN_PAGES_PER_ZSPAGE ||
> > +		    num_pages > ZS_MAX_PAGES_PER_ZSPAGE))
> > +		return NULL;
> 
> I tend to think that creating `struct zs_tunables` would be better. For
> the time being zs_tunables will contain only one member max_zspage_len,
> but it can be extended in the future.

+1 zs_tunables if we go that way.