Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Minchan Kim <minchan.kim@gmail.com>
Date:   Wed, 2 Nov 2022 13:15:12 -0700
From:   Minchan Kim <minchan@kernel.org>
To:     Sergey Senozhatsky <senozhatsky@chromium.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Nitin Gupta <ngupta@vflare.org>, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
Subject: Re: [PATCHv4 2/9] zram: Add recompression algorithm sysfs knob
Message-ID: <Y2LP0OWF/WTnkSne@google.com>
References: <20221018045533.2396670-1-senozhatsky@chromium.org>
 <20221018045533.2396670-3-senozhatsky@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221018045533.2396670-3-senozhatsky@chromium.org>
Precedence: bulk

On Tue, Oct 18, 2022 at 01:55:26PM +0900, Sergey Senozhatsky wrote:
> Introduce recomp_algorithm sysfs knob that controls
> secondary algorithm selection used for recompression.
> This device attribute works in a similar way with
> comp_algorithm attribute.
> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  drivers/block/zram/zram_drv.c | 111 +++++++++++++++++++++++++++-------
>  1 file changed, 90 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 770ea3489eb6..a8ef3c0c3dae 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -41,7 +41,12 @@ static DEFINE_IDR(zram_index_idr);
>  static DEFINE_MUTEX(zram_index_mutex);
>  
>  static int zram_major;
> -static const char *default_compressor = CONFIG_ZRAM_DEF_COMP;
> +static const char *default_comp_algs[ZRAM_MAX_ZCOMPS] = {
> +	CONFIG_ZRAM_DEF_COMP,
> +#ifdef CONFIG_ZRAM_MULTI_COMP
> +	"zstd",
> +#endif
> +};
>  
>  /* Module params (documentation at end) */
>  static unsigned int num_devices = 1;
> @@ -1000,31 +1005,37 @@ static ssize_t max_comp_streams_store(struct device *dev,
>  	return len;
>  }
>  
> -static ssize_t comp_algorithm_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)

Do you have any reason to change show and set placement? Otherwise,
please keep the function order to reduce unnecesssary churns.

> +static void comp_algorithm_set(struct zram *zram, u32 idx, const char *alg)
>  {
> -	size_t sz;
> -	struct zram *zram = dev_to_zram(dev);
> +	bool default_alg = false;
> +	int i;
>  
> -	down_read(&zram->init_lock);
> -	sz = zcomp_available_show(zram->comp_algs[ZRAM_PRIMARY_ZCOMP], buf);
> -	up_read(&zram->init_lock);
> +	/* Do not kfree() algs that we didn't allocate, IOW the default ones */
> +	for (i = 0; i < ZRAM_MAX_ZCOMPS; i++) {
> +		if (zram->comp_algs[idx] == default_comp_algs[i]) {
> +			default_alg = true;
> +			break;
> +		}
> +	}
>  
> -	return sz;
> +	if (!default_alg)
> +		kfree(zram->comp_algs[idx]);
> +	zram->comp_algs[idx] = alg;
>  }
>  
> -static void comp_algorithm_set(struct zram *zram, u32 idx, const char *alg)
> +static ssize_t __comp_algorithm_show(struct zram *zram, u32 idx, char *buf)
>  {
> -	/* Do not kfree() algs that we didn't allocate, IOW the default ones */
> -	if (zram->comp_algs[idx] != default_compressor)
> -		kfree(zram->comp_algs[idx]);
> -	zram->comp_algs[idx] = alg;
> +	ssize_t sz;
> +
> +	down_read(&zram->init_lock);
> +	sz = zcomp_available_show(zram->comp_algs[idx], buf);
> +	up_read(&zram->init_lock);
> +
> +	return sz;
>  }
>  
> -static ssize_t comp_algorithm_store(struct device *dev,
> -		struct device_attribute *attr, const char *buf, size_t len)
> +static int __comp_algorithm_store(struct zram *zram, u32 idx, const char *buf)
>  {
> -	struct zram *zram = dev_to_zram(dev);
>  	char *compressor;
>  	size_t sz;
>  
> @@ -1053,11 +1064,55 @@ static ssize_t comp_algorithm_store(struct device *dev,
>  		return -EBUSY;
>  	}
>  
> -	comp_algorithm_set(zram, ZRAM_PRIMARY_ZCOMP, compressor);
> +	comp_algorithm_set(zram, idx, compressor);
>  	up_write(&zram->init_lock);
> -	return len;
> +	return 0;
> +}
> +
> +static ssize_t comp_algorithm_show(struct device *dev,
> +				   struct device_attribute *attr,
> +				   char *buf)
> +{
> +	struct zram *zram = dev_to_zram(dev);
> +
> +	return __comp_algorithm_show(zram, ZRAM_PRIMARY_ZCOMP, buf);
> +}
> +
> +static ssize_t comp_algorithm_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf,
> +				    size_t len)
> +{
> +	struct zram *zram = dev_to_zram(dev);
> +	int ret;
> +
> +	ret = __comp_algorithm_store(zram, ZRAM_PRIMARY_ZCOMP, buf);
> +	return ret ? ret : len;
>  }
>  
> +#ifdef CONFIG_ZRAM_MULTI_COMP
> +static ssize_t recomp_algorithm_show(struct device *dev,
> +				     struct device_attribute *attr,
> +				     char *buf)
> +{
> +	struct zram *zram = dev_to_zram(dev);
> +
> +	return __comp_algorithm_show(zram, ZRAM_SECONDARY_ZCOMP, buf);
> +}

Just open question(I might be too paranoid?)

I am thinking someone want to add third comp algorithm in future
to balance decompression and memory efficiency.

If it's not too crazy idea, let's think about the interface.
Maybe, could we make the recomp knobs works like list?

# A primary comp
echo "A" > /zram/comp_algo

# Multiple secondary comps
echo "B threshold" > /zram/add_recomp_algo
echo "C threshold" > /zram/add_recomp_algo
echo "D threshold" > /zram/add_recomp_algo

"cat /zram/recomp_algo" shows the list

echo "C" > /zram/remove_recomp_algo
will remove the C algorithm in stack.

My point is that we don't need to implement it atm but makes the
interface to open the possibility for future extension.

What do you think?

> +
> +static ssize_t recomp_algorithm_store(struct device *dev,
> +				      struct device_attribute *attr,
> +				      const char *buf,
> +				      size_t len)
> +{
> +	struct zram *zram = dev_to_zram(dev);
> +	int ret;
> +
> +	ret = __comp_algorithm_store(zram, ZRAM_SECONDARY_ZCOMP, buf);
> +	return ret ? ret : len;
> +}
> +#endif
> +
>  static ssize_t compact_store(struct device *dev,
>  		struct device_attribute *attr, const char *buf, size_t len)
>  {
> @@ -1762,7 +1817,11 @@ static void zram_reset_device(struct zram *zram)
>  	memset(&zram->stats, 0, sizeof(zram->stats));
>  	reset_bdev(zram);
>  
> -	comp_algorithm_set(zram, ZRAM_PRIMARY_ZCOMP, default_compressor);
> +	comp_algorithm_set(zram, ZRAM_PRIMARY_ZCOMP,
> +			   default_comp_algs[ZRAM_PRIMARY_ZCOMP]);
> +	if (IS_ENABLED(CONFIG_ZRAM_MULTI_COMP))

Dumb question:

Why do you use IS_ENABLED instead of ifdef?


> +		comp_algorithm_set(zram, ZRAM_SECONDARY_ZCOMP,
> +				   default_comp_algs[ZRAM_SECONDARY_ZCOMP]);
>  	up_write(&zram->init_lock);
>  }
>  
> @@ -1895,6 +1954,9 @@ static DEVICE_ATTR_WO(writeback);
>  static DEVICE_ATTR_RW(writeback_limit);
>  static DEVICE_ATTR_RW(writeback_limit_enable);
>  #endif
> +#ifdef CONFIG_ZRAM_MULTI_COMP
> +static DEVICE_ATTR_RW(recomp_algorithm);
> +#endif
>  
>  static struct attribute *zram_disk_attrs[] = {
>  	&dev_attr_disksize.attr,
> @@ -1918,6 +1980,9 @@ static struct attribute *zram_disk_attrs[] = {
>  	&dev_attr_bd_stat.attr,
>  #endif
>  	&dev_attr_debug_stat.attr,
> +#ifdef CONFIG_ZRAM_MULTI_COMP
> +	&dev_attr_recomp_algorithm.attr,
> +#endif
>  	NULL,
>  };
>  
> @@ -1997,7 +2062,11 @@ static int zram_add(void)
>  	if (ret)
>  		goto out_cleanup_disk;
>  
> -	zram->comp_algs[ZRAM_PRIMARY_ZCOMP] = default_compressor;
> +	zram->comp_algs[ZRAM_PRIMARY_ZCOMP] =
> +		default_comp_algs[ZRAM_PRIMARY_ZCOMP];
> +	if (IS_ENABLED(CONFIG_ZRAM_MULTI_COMP))
> +		zram->comp_algs[ZRAM_SECONDARY_ZCOMP] =
> +			default_comp_algs[ZRAM_SECONDARY_ZCOMP];
>  
>  	zram_debugfs_register(zram);
>  	pr_info("Added device: %s\n", zram->disk->disk_name);
> -- 
> 2.38.0.413.g74048e4d9e-goog
>