[PATCHv4,2/9] zram: Add recompression algorithm sysfs knob

  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(-)
  

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
>
  

On (22/11/02 13:15), Minchan Kim wrote:
[..]
> >  /* 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.

I don't change their placement. It's just show and store for primary and
secondary algorithms use the same __store and __show functions, which
are static and are placed ahead of store and show.

[..]
> 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

What is the threshold here? My design approach is that ZRAM doesn't do
recompression on its own, so no magic is happening automatically. It's
the user-space that triggers recompression for selected pages when
user-space thinks it's time to. This allows us to have various flexible
policies and consider things that ZRAM is not even aware of: battery level,
free memory, CPU load average, etc. E.g. no recompression when all CPUs
are busy rendering video game, or when we are draining battery too fast,
etc.

> "cat /zram/recomp_algo" shows the list
> 
> echo "C" > /zram/remove_recomp_algo
> will remove the C algorithm in stack.

What is the use case for removal of a secondary algorithm?

> 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?

So, as far as I understand, we don't have reason to add remove_recomp_algo
right now. And existing recomp_algo does not enforce any particular format,
it can be extended. Right now we accept "$name" but can do something like
"$name:$priority". The only thing that we probably need to do is rename
recomp_algo to either add_recomp_algo or register_recomp_algo?

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

#ifdef-s are banned in the new C-code, as far as I know. IS_ENABLED is
what we should use.
  

On (22/11/03 12:05), Sergey Senozhatsky wrote:
> [..]
> > 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

As a side note:
The way it's implemented currently is that comps is an array, so we
can store more comps there. I sort of was thinking that we probably
can have more than two algos at some point the in the future (hence
the MULTI_COMPRESS config option).
  

On (22/11/03 12:05), Sergey Senozhatsky wrote:
> What is the use case for removal of a secondary algorithm?
> 
> > 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?
> 
> So, as far as I understand, we don't have reason to add remove_recomp_algo
> right now. And existing recomp_algo does not enforce any particular format,
> it can be extended. Right now we accept "$name" but can do something like
> "$name:$priority".

Or with keywords:

	name=STRING priority=INT

and the only legal priority for now is 1.
  

On (22/11/03 13:09), Sergey Senozhatsky wrote:
> On (22/11/03 12:05), Sergey Senozhatsky wrote:
> > What is the use case for removal of a secondary algorithm?
> > 
> > > 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?
> > 
> > So, as far as I understand, we don't have reason to add remove_recomp_algo
> > right now. And existing recomp_algo does not enforce any particular format,
> > it can be extended. Right now we accept "$name" but can do something like
> > "$name:$priority".
> 
> Or with keywords:
> 
> 	name=STRING priority=INT
> 
> and the only legal priority for now is 1.


E.g. recomp_algorithm support for algorithms name= and optional
integer priority=.

I sort of like the recomp_algorithm name so far, but we can change it.

---

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index cf9d3474b80c..9a614253de07 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1102,9 +1102,38 @@ static ssize_t recomp_algorithm_store(struct device *dev,
 				      size_t len)
 {
 	struct zram *zram = dev_to_zram(dev);
+	int prio = ZRAM_SECONDARY_ZCOMP;
+	char *args, *param, *val;
+	char *alg = NULL;
 	int ret;
 
-	ret = __comp_algorithm_store(zram, ZRAM_SECONDARY_ZCOMP, buf);
+	args = skip_spaces(buf);
+	while (*args) {
+		args = next_arg(args, &param, &val);
+
+		if (!*val)
+			return -EINVAL;
+
+		if (!strcmp(param, "name")) {
+			alg = val;
+			continue;
+		}
+
+		if (!strcmp(param, "priority")) {
+			ret = kstrtoint(val, 10, &prio);
+			if (ret)
+				return ret;
+			continue;
+		}
+	}
+
+	if (!alg)
+		return -EINVAL;
+
+	if (prio < ZRAM_SECONDARY_ZCOMP || prio >= ZRAM_MAX_ZCOMPS)
+		return -EINVAL;
+
+	ret = __comp_algorithm_store(zram, prio, alg);
 	return ret ? ret : len;
 }
 #endif
  

On Thu, Nov 03, 2022 at 12:05:06PM +0900, Sergey Senozhatsky wrote:

< snip >

> > 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
> 
> What is the threshold here? My design approach is that ZRAM doesn't do

As your term, watermark but yeah, priority you suggested would be good
for me.

> recompression on its own, so no magic is happening automatically. It's
> the user-space that triggers recompression for selected pages when
> user-space thinks it's time to. This allows us to have various flexible
> policies and consider things that ZRAM is not even aware of: battery level,
> free memory, CPU load average, etc. E.g. no recompression when all CPUs
> are busy rendering video game, or when we are draining battery too fast,
> etc.
> 
> > "cat /zram/recomp_algo" shows the list
> > 
> > echo "C" > /zram/remove_recomp_algo
> > will remove the C algorithm in stack.
> 
> What is the use case for removal of a secondary algorithm?

Without the interface, How can we modify the selection if admin want to
change the order of second algorithms?

> 
> > 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?
> 
> So, as far as I understand, we don't have reason to add remove_recomp_algo
> right now. And existing recomp_algo does not enforce any particular format,
> it can be extended. Right now we accept "$name" but can do something like
> "$name:$priority". The only thing that we probably need to do is rename
> recomp_algo to either add_recomp_algo or register_recomp_algo?

Yeah, I like the name and priority format.

Only question is how we could support algorithm selection change
under considering multiple secondary algorithms.
  

On Thu, Nov 03, 2022 at 12:54:43PM +0900, Sergey Senozhatsky wrote:
> On (22/11/03 12:05), Sergey Senozhatsky wrote:
> > [..]
> > > 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
> 
> As a side note:
> The way it's implemented currently is that comps is an array, so we
> can store more comps there. I sort of was thinking that we probably
> can have more than two algos at some point the in the future (hence
> the MULTI_COMPRESS config option).

Sure.
  

On Thu, Nov 03, 2022 at 01:09:49PM +0900, Sergey Senozhatsky wrote:
> On (22/11/03 12:05), Sergey Senozhatsky wrote:
> > What is the use case for removal of a secondary algorithm?
> > 
> > > 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?
> > 
> > So, as far as I understand, we don't have reason to add remove_recomp_algo
> > right now. And existing recomp_algo does not enforce any particular format,
> > it can be extended. Right now we accept "$name" but can do something like
> > "$name:$priority".
> 
> Or with keywords:
> 
> 	name=STRING priority=INT
> 
> and the only legal priority for now is 1.

I like it.
  

On (22/11/03 09:34), Minchan Kim wrote:
> > > 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?
> > 
> > So, as far as I understand, we don't have reason to add remove_recomp_algo
> > right now. And existing recomp_algo does not enforce any particular format,
> > it can be extended. Right now we accept "$name" but can do something like
> > "$name:$priority". The only thing that we probably need to do is rename
> > recomp_algo to either add_recomp_algo or register_recomp_algo?
> 
> Yeah, I like the name and priority format.
> 
> Only question is how we could support algorithm selection change
> under considering multiple secondary algorithms.

So what I was thinking about, and I'm still in the mental model that
re-compression is a user-space event, just like writeback, extension
of recompress sysfs knob with "algo_index" (or something similar) which
will mirror algorithm priority.

Example:

Configure 2 alternative algos, with priority 1 and 2

	echo "name=lz4 priority=1" > recomp_algo
	echo "name=lz5 priority=2" > recomp_algo

Recompress pages using algo 1 and algo 2

	echo "type=huge threshold=3000 algo_idx=1" > recompress
	echo "type=idle threshold=2000 algo_idx=2" > recompress

Maybe we can even pass algo name instead of idx.
  

On (22/11/04 12:18), Sergey Senozhatsky wrote:
> On (22/11/03 09:34), Minchan Kim wrote:
> > Yeah, I like the name and priority format.
> > 
> > Only question is how we could support algorithm selection change
> > under considering multiple secondary algorithms.
> 
> So what I was thinking about, and I'm still in the mental model that
> re-compression is a user-space event, just like writeback, extension
> of recompress sysfs knob with "algo_index" (or something similar) which
> will mirror algorithm priority.
> 
> Example:
> 
> Configure 2 alternative algos, with priority 1 and 2
> 
> 	echo "name=lz4 priority=1" > recomp_algo
> 	echo "name=lz5 priority=2" > recomp_algo
> 
> Recompress pages using algo 1 and algo 2
> 
> 	echo "type=huge threshold=3000 algo_idx=1" > recompress
> 	echo "type=idle threshold=2000 algo_idx=2" > recompress
> 
> Maybe we can even pass algo name instead of idx.

Or pass priority= so that interface that uses algorithms has the
same keyword that the interface that configures those algorithms.

I still don't see many use-cases for "delete algorithm", to be honest.
ZRAM is configured by scripts in 99.99999% of cases and it is
quite static once it has been configured. So we probably can use
the "don't setup algorithms that you don't need" approach, to keep
things simpler.
  

On Fri, Nov 04, 2022 at 12:18:43PM +0900, Sergey Senozhatsky wrote:
> On (22/11/03 09:34), Minchan Kim wrote:
> > > > 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?
> > > 
> > > So, as far as I understand, we don't have reason to add remove_recomp_algo
> > > right now. And existing recomp_algo does not enforce any particular format,
> > > it can be extended. Right now we accept "$name" but can do something like
> > > "$name:$priority". The only thing that we probably need to do is rename
> > > recomp_algo to either add_recomp_algo or register_recomp_algo?
> > 
> > Yeah, I like the name and priority format.
> > 
> > Only question is how we could support algorithm selection change
> > under considering multiple secondary algorithms.
> 
> So what I was thinking about, and I'm still in the mental model that
> re-compression is a user-space event, just like writeback, extension
> of recompress sysfs knob with "algo_index" (or something similar) which
> will mirror algorithm priority.
> 
> Example:
> 
> Configure 2 alternative algos, with priority 1 and 2
> 
> 	echo "name=lz4 priority=1" > recomp_algo
> 	echo "name=lz5 priority=2" > recomp_algo
> 
> Recompress pages using algo 1 and algo 2
> 
> 	echo "type=huge threshold=3000 algo_idx=1" > recompress
> 	echo "type=idle threshold=2000 algo_idx=2" > recompress
> 
> Maybe we can even pass algo name instead of idx.

Let's use name rather than index.
  

On Fri, Nov 04, 2022 at 01:53:11PM +0900, Sergey Senozhatsky wrote:
> On (22/11/04 12:18), Sergey Senozhatsky wrote:
> > On (22/11/03 09:34), Minchan Kim wrote:
> > > Yeah, I like the name and priority format.
> > > 
> > > Only question is how we could support algorithm selection change
> > > under considering multiple secondary algorithms.
> > 
> > So what I was thinking about, and I'm still in the mental model that
> > re-compression is a user-space event, just like writeback, extension
> > of recompress sysfs knob with "algo_index" (or something similar) which
> > will mirror algorithm priority.
> > 
> > Example:
> > 
> > Configure 2 alternative algos, with priority 1 and 2
> > 
> > 	echo "name=lz4 priority=1" > recomp_algo
> > 	echo "name=lz5 priority=2" > recomp_algo
> > 
> > Recompress pages using algo 1 and algo 2
> > 
> > 	echo "type=huge threshold=3000 algo_idx=1" > recompress
> > 	echo "type=idle threshold=2000 algo_idx=2" > recompress
> > 
> > Maybe we can even pass algo name instead of idx.
> 
> Or pass priority= so that interface that uses algorithms has the
> same keyword that the interface that configures those algorithms.

Hmm, why do we need algo_idx here if we already set up every
fields at algorithm setup time?

My understaind(assuming default(i.e., primary) algo is lzo) is

    echo "name=lz4 priority=1" > recomp_algo
    echo "name=lz5 priority=2" > recomp_algo

    echo "type=huge threshold=3000" > recompress

It will try compress every objects which greater than 3000B with lz4 first
and then lz5 if it's stillgreater or equal than 3000(or same size class).

> 
> I still don't see many use-cases for "delete algorithm", to be honest.
> ZRAM is configured by scripts in 99.99999% of cases and it is

For the development time in the local side, people usually type in
until they will have solid script version. If we asks resetting
to zram to modify it, it's not good and consistent with other
sysfs knobs we could overwrite it to change it. How about supporting
overwritting to chage it over priority?

    echo "name=lz4 priority=1" > recomp_algo
    echo "name=lz5 priority=2" > recomp_algo

    # or I realized to change lz5 to lz7 so
    echo "name=lz6 priority=2" > recomp_algo

> quite static once it has been configured. So we probably can use
> the "don't setup algorithms that you don't need" approach, to keep
> things simpler.
  

On (22/11/04 09:34), Minchan Kim wrote:
> > > > So, as far as I understand, we don't have reason to add remove_recomp_algo
> > > > right now. And existing recomp_algo does not enforce any particular format,
> > > > it can be extended. Right now we accept "$name" but can do something like
> > > > "$name:$priority". The only thing that we probably need to do is rename
> > > > recomp_algo to either add_recomp_algo or register_recomp_algo?
> > > 
> > > Yeah, I like the name and priority format.
> > > 
> > > Only question is how we could support algorithm selection change
> > > under considering multiple secondary algorithms.
> > 
> > So what I was thinking about, and I'm still in the mental model that
> > re-compression is a user-space event, just like writeback, extension
> > of recompress sysfs knob with "algo_index" (or something similar) which
> > will mirror algorithm priority.
> > 
> > Example:
> > 
> > Configure 2 alternative algos, with priority 1 and 2
> > 
> > 	echo "name=lz4 priority=1" > recomp_algo
> > 	echo "name=lz5 priority=2" > recomp_algo
> > 
> > Recompress pages using algo 1 and algo 2
> > 
> > 	echo "type=huge threshold=3000 algo_idx=1" > recompress
> > 	echo "type=idle threshold=2000 algo_idx=2" > recompress
> > 
> > Maybe we can even pass algo name instead of idx.
> 
> Let's use name rather than index.

OK. Any preference on the keyword? "name="? "algo="? "algorithm="?
"compressor="? "comp="?

I want use the same keyword for recomp_algo. I sort of like "algo=",
but not sure.
  

On Sat, Nov 05, 2022 at 08:25:44AM +0900, Sergey Senozhatsky wrote:
> On (22/11/04 09:34), Minchan Kim wrote:
> > > > > So, as far as I understand, we don't have reason to add remove_recomp_algo
> > > > > right now. And existing recomp_algo does not enforce any particular format,
> > > > > it can be extended. Right now we accept "$name" but can do something like
> > > > > "$name:$priority". The only thing that we probably need to do is rename
> > > > > recomp_algo to either add_recomp_algo or register_recomp_algo?
> > > > 
> > > > Yeah, I like the name and priority format.
> > > > 
> > > > Only question is how we could support algorithm selection change
> > > > under considering multiple secondary algorithms.
> > > 
> > > So what I was thinking about, and I'm still in the mental model that
> > > re-compression is a user-space event, just like writeback, extension
> > > of recompress sysfs knob with "algo_index" (or something similar) which
> > > will mirror algorithm priority.
> > > 
> > > Example:
> > > 
> > > Configure 2 alternative algos, with priority 1 and 2
> > > 
> > > 	echo "name=lz4 priority=1" > recomp_algo
> > > 	echo "name=lz5 priority=2" > recomp_algo
> > > 
> > > Recompress pages using algo 1 and algo 2
> > > 
> > > 	echo "type=huge threshold=3000 algo_idx=1" > recompress
> > > 	echo "type=idle threshold=2000 algo_idx=2" > recompress
> > > 
> > > Maybe we can even pass algo name instead of idx.
> > 
> > Let's use name rather than index.
> 
> OK. Any preference on the keyword? "name="? "algo="? "algorithm="?
> "compressor="? "comp="?
> 
> I want use the same keyword for recomp_algo. I sort of like "algo=",
> but not sure.

+1 with algo
  

On (22/11/04 10:43), Minchan Kim wrote:
> > > Configure 2 alternative algos, with priority 1 and 2
> > > 
> > > 	echo "name=lz4 priority=1" > recomp_algo
> > > 	echo "name=lz5 priority=2" > recomp_algo
> > > 
> > > Recompress pages using algo 1 and algo 2
> > > 
> > > 	echo "type=huge threshold=3000 algo_idx=1" > recompress
> > > 	echo "type=idle threshold=2000 algo_idx=2" > recompress
> > > 
> > > Maybe we can even pass algo name instead of idx.
> > 
> > Or pass priority= so that interface that uses algorithms has the
> > same keyword that the interface that configures those algorithms.
> 
> Hmm, why do we need algo_idx here if we already set up every
> fields at algorithm setup time?
> 
> My understaind(assuming default(i.e., primary) algo is lzo) is
> 
>     echo "name=lz4 priority=1" > recomp_algo
>     echo "name=lz5 priority=2" > recomp_algo
> 
>     echo "type=huge threshold=3000" > recompress
> 
> It will try compress every objects which greater than 3000B with lz4 first
> and then lz5 if it's stillgreater or equal than 3000(or same size class).

One can be SW one can be HW. So I thought about having flexibility here.
Instead of doing

	for (idx = 1; idx < MAX_IDX; idx++) {
		len = zcomp_compress(zram->comps[idx]);
		if (len <= threshold)
			break;
	}

We would just directly use the suggested algo.

But we probably don't need that param at all and can use
the loop instead?

[..]
>     echo "name=lz4 priority=1" > recomp_algo
>     echo "name=lz5 priority=2" > recomp_algo
>
>     # or I realized to change lz5 to lz7 so
>     echo "name=lz6 priority=2" > recomp_algo

So the latter should delete lz5 at idx 2 and put lz6 there?
I can add that.
  

On (22/11/04 16:40), Minchan Kim wrote:
> > > > Configure 2 alternative algos, with priority 1 and 2
> > > > 
> > > > 	echo "name=lz4 priority=1" > recomp_algo
> > > > 	echo "name=lz5 priority=2" > recomp_algo
> > > > 
> > > > Recompress pages using algo 1 and algo 2
> > > > 
> > > > 	echo "type=huge threshold=3000 algo_idx=1" > recompress
> > > > 	echo "type=idle threshold=2000 algo_idx=2" > recompress
> > > > 
> > > > Maybe we can even pass algo name instead of idx.
> > > 
> > > Let's use name rather than index.
> > 
> > OK. Any preference on the keyword? "name="? "algo="? "algorithm="?
> > "compressor="? "comp="?
> > 
> > I want use the same keyword for recomp_algo. I sort of like "algo=",
> > but not sure.
> 
> +1 with algo

Minchan, I'm sorry I'm getting a bit confused (didn't sleep last night).
I just saw your other email and you suggested there that we don't need
any idx or name in recompress. Or did I misunderstand it?
  

On (22/11/05 08:41), Sergey Senozhatsky wrote:
> One can be SW one can be HW. So I thought about having flexibility here.
> Instead of doing
> 
> 	for (idx = 1; idx < MAX_IDX; idx++) {
> 		len = zcomp_compress(zram->comps[idx]);
> 		if (len <= threshold)
> 			break;
> 	}
> 
> We would just directly use the suggested algo.
> 
> But we probably don't need that param at all and can use
> the loop instead?

My idea was that recompress does not loop through the algos (what
on the fly recompression can do for instance), but instead
recompress only does one thing.

Because we can have, for instance, something like this

	algo=zstd priority=1
	algo=deflate priority=2

And we recompress with algo=zstd only first. Without going
into the slowest, most CPU and power consuming one. If the
memory pressure keeps increasing then we might do algo=deflate
recompress, as the last resort before we do writeback. But we
may skip algo=deflate altogether and go straight to writeback,
for instance because we have less than 30% of battery left.

So the reason I suggested algo= in recompress was, basically,
for that for having exact control of what recompression does.
  

On Sat, Nov 05, 2022 at 08:41:37AM +0900, Sergey Senozhatsky wrote:
> On (22/11/04 10:43), Minchan Kim wrote:
> > > > Configure 2 alternative algos, with priority 1 and 2
> > > > 
> > > > 	echo "name=lz4 priority=1" > recomp_algo
> > > > 	echo "name=lz5 priority=2" > recomp_algo
> > > > 
> > > > Recompress pages using algo 1 and algo 2
> > > > 
> > > > 	echo "type=huge threshold=3000 algo_idx=1" > recompress
> > > > 	echo "type=idle threshold=2000 algo_idx=2" > recompress
> > > > 
> > > > Maybe we can even pass algo name instead of idx.
> > > 
> > > Or pass priority= so that interface that uses algorithms has the
> > > same keyword that the interface that configures those algorithms.
> > 
> > Hmm, why do we need algo_idx here if we already set up every
> > fields at algorithm setup time?
> > 
> > My understaind(assuming default(i.e., primary) algo is lzo) is
> > 
> >     echo "name=lz4 priority=1" > recomp_algo
> >     echo "name=lz5 priority=2" > recomp_algo
> > 
> >     echo "type=huge threshold=3000" > recompress
> > 
> > It will try compress every objects which greater than 3000B with lz4 first
> > and then lz5 if it's stillgreater or equal than 3000(or same size class).
> 
> One can be SW one can be HW. So I thought about having flexibility here.
> Instead of doing
> 
> 	for (idx = 1; idx < MAX_IDX; idx++) {
> 		len = zcomp_compress(zram->comps[idx]);
> 		if (len <= threshold)
> 			break;
> 	}
> 
> We would just directly use the suggested algo.
> 
> But we probably don't need that param at all and can use
> the loop instead?

I don't understand what param you are saying. I expected
the zram->comps array already has sorted algoritm based on the
priority so the loop will try compression as expected so loop is fine.
Are we on same page?

> 
> [..]
> >     echo "name=lz4 priority=1" > recomp_algo
> >     echo "name=lz5 priority=2" > recomp_algo
> >
> >     # or I realized to change lz5 to lz7 so
> >     echo "name=lz6 priority=2" > recomp_algo
> 
> So the latter should delete lz5 at idx 2 and put lz6 there?
> I can add that.

Yub.
  

On Sat, Nov 05, 2022 at 08:44:46AM +0900, Sergey Senozhatsky wrote:
> On (22/11/04 16:40), Minchan Kim wrote:
> > > > > Configure 2 alternative algos, with priority 1 and 2
> > > > > 
> > > > > 	echo "name=lz4 priority=1" > recomp_algo
> > > > > 	echo "name=lz5 priority=2" > recomp_algo
> > > > > 
> > > > > Recompress pages using algo 1 and algo 2
> > > > > 
> > > > > 	echo "type=huge threshold=3000 algo_idx=1" > recompress
> > > > > 	echo "type=idle threshold=2000 algo_idx=2" > recompress
> > > > > 
> > > > > Maybe we can even pass algo name instead of idx.
> > > > 
> > > > Let's use name rather than index.
> > > 
> > > OK. Any preference on the keyword? "name="? "algo="? "algorithm="?
> > > "compressor="? "comp="?
> > > 
> > > I want use the same keyword for recomp_algo. I sort of like "algo=",
> > > but not sure.
> > 
> > +1 with algo
> 
> Minchan, I'm sorry I'm getting a bit confused (didn't sleep last night).
> I just saw your other email and you suggested there that we don't need
> any idx or name in recompress. Or did I misunderstand it?
> 

I should have more clear. Sorry for that.

I meant if you need some reason, I prefer "algo=' to make review
proceed. If you agree we don't need it, then, yeah, we are all good.
  

On (22/11/04 17:01), Minchan Kim wrote:
> > One can be SW one can be HW. So I thought about having flexibility here.
> > Instead of doing
> > 
> > 	for (idx = 1; idx < MAX_IDX; idx++) {
> > 		len = zcomp_compress(zram->comps[idx]);
> > 		if (len <= threshold)
> > 			break;
> > 	}
> > 
> > We would just directly use the suggested algo.
> > 
> > But we probably don't need that param at all and can use
> > the loop instead?
> 
> I don't understand what param you are saying. I expected
> the zram->comps array already has sorted algoritm based on the
> priority so the loop will try compression as expected so loop is fine.
> Are we on same page?

I'll try to implement both loop and a specific algorithm selection.
  

On Sat, Nov 05, 2022 at 09:00:33AM +0900, Sergey Senozhatsky wrote:
> On (22/11/05 08:41), Sergey Senozhatsky wrote:
> > One can be SW one can be HW. So I thought about having flexibility here.
> > Instead of doing
> > 
> > 	for (idx = 1; idx < MAX_IDX; idx++) {
> > 		len = zcomp_compress(zram->comps[idx]);
> > 		if (len <= threshold)
> > 			break;
> > 	}
> > 
> > We would just directly use the suggested algo.
> > 
> > But we probably don't need that param at all and can use
> > the loop instead?
> 
> My idea was that recompress does not loop through the algos (what
> on the fly recompression can do for instance), but instead
> recompress only does one thing.
> 
> Because we can have, for instance, something like this
> 
> 	algo=zstd priority=1
> 	algo=deflate priority=2
> 
> And we recompress with algo=zstd only first. Without going
> into the slowest, most CPU and power consuming one. If the
> memory pressure keeps increasing then we might do algo=deflate
> recompress, as the last resort before we do writeback. But we
> may skip algo=deflate altogether and go straight to writeback,
> for instance because we have less than 30% of battery left.
> 
> So the reason I suggested algo= in recompress was, basically,
> for that for having exact control of what recompression does.


I am thinking like this:

* Without recomp_algo setup, user can do whatever they want on the fly


    echo "type=idle threshold=3000 algo=zstd" > recompress

Later they could do

    echo "type=idle threshold=3000 algo=deflate" > recompress

or
    
    writeback to backing device

* With recomp_algo setup like this,

    echo "algo=zstd priority=1" > recomp_algo
    echo "algo=deflate priority=2" > recomp_algo
    ..
    ..
    echo "type=idle threshold=3000" > recompress

If zstd fails, it will continue to work with deflate transparently.

IOW, "algo=" in *recompress* interface will override the global policy
in the *recomp_algo* setup. Otherwise, the recomp_algo's set up will
work.
  

On (22/11/07 11:08), Minchan Kim wrote:
[..]
> I am thinking like this:
> 
> * Without recomp_algo setup, user can do whatever they want on the fly
> 
> 
>     echo "type=idle threshold=3000 algo=zstd" > recompress
> 
> Later they could do
> 
>     echo "type=idle threshold=3000 algo=deflate" > recompress

By "without recomp_algo setup" you mean that user doesn't configure
anything before `echo XG > zramX/disksize`? Currently algorithm and
recomp algorithm need to be selected at the same time - before zram
device is initialised, because we use the same code and same approaches:
we need to have zcomp back-ends in per-CPU data in zram read/write/recompress.

Creating per-CPU zcomps on the fly is probably going to be a little bit
intrusive to zram.



What I currently have is as follows.

A copy paste from my test script:

- init device
        echo "algo=lz4 priority=1" > /sys/block/zram0/recomp_algorithm
        echo "algo=zstd priority=2" > /sys/block/zram0/recomp_algorithm
        echo "algo=deflate priority=3" > /sys/block/zram0/recomp_algorithm
        echo 5G > /sys/block/zram0/disksize


Various recompression use cases:

- recompress huge pages using all secondary algos in order of priority
        echo "type=huge" > /sys/block/zram0/recompress

- recompress huge pages using zstd only
        echo "type=huge algo=zstd" > /sys/block/zram0/recompress

- recompress all pages using lz4
        echo "algo=lz4" > /sys/block/zram0/recompress

- recompress idle pages, use all algos in priority order, with threshold
        echo "type=idle threshold=3000" > /sys/block/zram0/recompress

- recompress idle pages, using zstd only, with threshold
        echo "algo=zstd type=idle threshold=2000" > /sys/block/zram0/recompress