linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4] zram: break the strict dependency from lzo
@ 2020-10-28 11:59 Rui Salvaterra
  2020-10-28 18:59 ` Sergey Senozhatsky
  0 siblings, 1 reply; 6+ messages in thread
From: Rui Salvaterra @ 2020-10-28 11:59 UTC (permalink / raw)
  To: minchan, ngupta, sergey.senozhatsky.work
  Cc: linux-block, linux-kernel, Rui Salvaterra

There's nothing special about zram and lzo. It works just fine without it, so
as long as at least one of the other supported compression algorithms is
selected.

Suggested-by: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Signed-off-by: Rui Salvaterra <rsalvaterra@gmail.com>
---
v4: incorporate Sergey's feedback and fix a small typo.
v3: fix the default selection when lzo isn't present. Rebase against 5.10-rc1.
v2: fix the dependency on CRYPTO.

 drivers/block/zram/Kconfig    |  6 +++++-
 drivers/block/zram/zcomp.c    | 17 +++++++++++++++++
 drivers/block/zram/zcomp.h    |  1 +
 drivers/block/zram/zram_drv.c |  5 +++--
 4 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
index fe7a4b7d30cf..141ce0ebad06 100644
--- a/drivers/block/zram/Kconfig
+++ b/drivers/block/zram/Kconfig
@@ -2,7 +2,6 @@
 config ZRAM
 	tristate "Compressed RAM block device support"
 	depends on BLOCK && SYSFS && ZSMALLOC && CRYPTO
-	select CRYPTO_LZO
 	help
 	  Creates virtual block devices called /dev/zramX (X = 0, 1, ...).
 	  Pages written to these disks are compressed and stored in memory
@@ -14,6 +13,11 @@ config ZRAM
 
 	  See Documentation/admin-guide/blockdev/zram.rst for more information.
 
+config ZRAM_AUTOSEL_ALGO
+	def_bool y
+	depends on ZRAM && !(CRYPTO_LZ4 || CRYPTO_LZ4HC || CRYPTO_842 || CRYPTO_ZSTD)
+	select CRYPTO_LZO
+
 config ZRAM_WRITEBACK
        bool "Write back incompressible or idle page to backing device"
        depends on ZRAM
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 33e3b76c4fa9..4fad177c78c4 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -15,8 +15,10 @@
 #include "zcomp.h"
 
 static const char * const backends[] = {
+#if IS_ENABLED(CONFIG_CRYPTO_LZO)
 	"lzo",
 	"lzo-rle",
+#endif
 #if IS_ENABLED(CONFIG_CRYPTO_LZ4)
 	"lz4",
 #endif
@@ -202,6 +204,21 @@ void zcomp_destroy(struct zcomp *comp)
 	kfree(comp);
 }
 
+const char *default_compressor(void)
+{
+	/*
+	 * Pick the first available one (there should be at least one).
+	 *
+	 * In theory, we can drop all the ifdefs from backends[] and
+	 * just iterate backends array doing crypto_has_comp(comp, 0, 0)
+	 * for each entry and return the first one which is recognized by
+	 * crypto. But crypto_has_comp() modprobes compression drivers,
+	 * so we may end up with extra loaded drivers, when the 'default'
+	 * compressor is not what zram is configured to use.
+	 */
+	return backends[0];
+}
+
 /*
  * search available compressors for requested algorithm.
  * allocate new zcomp and initialize it. return compressing
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index 40f6420f4b2e..f104be9eae9c 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -27,6 +27,7 @@ int zcomp_cpu_dead(unsigned int cpu, struct hlist_node *node);
 ssize_t zcomp_available_show(const char *comp, char *buf);
 bool zcomp_available_algorithm(const char *comp);
 
+const char *default_compressor(void);
 struct zcomp *zcomp_create(const char *comp);
 void zcomp_destroy(struct zcomp *comp);
 
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 1b697208d661..f02ee050c7bf 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -42,7 +42,6 @@ static DEFINE_IDR(zram_index_idr);
 static DEFINE_MUTEX(zram_index_mutex);
 
 static int zram_major;
-static const char *default_compressor = "lzo-rle";
 
 /* Module params (documentation at end) */
 static unsigned int num_devices = 1;
@@ -1960,7 +1959,9 @@ static int zram_add(void)
 	blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, zram->disk->queue);
 	device_add_disk(NULL, zram->disk, zram_disk_attr_groups);
 
-	strlcpy(zram->compressor, default_compressor, sizeof(zram->compressor));
+	strlcpy(zram->compressor,
+		default_compressor(),
+		sizeof(zram->compressor));
 
 	zram_debugfs_register(zram);
 	pr_info("Added device: %s\n", zram->disk->disk_name);
-- 
2.29.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] zram: break the strict dependency from lzo
  2020-10-28 11:59 [PATCH v4] zram: break the strict dependency from lzo Rui Salvaterra
@ 2020-10-28 18:59 ` Sergey Senozhatsky
  2020-11-03 21:28   ` Minchan Kim
  0 siblings, 1 reply; 6+ messages in thread
From: Sergey Senozhatsky @ 2020-10-28 18:59 UTC (permalink / raw)
  To: Minchan Kim, Rui Salvaterra
  Cc: Andrew Morton, ngupta, sergey.senozhatsky.work, linux-block,
	linux-kernel

Cc-ing Andrew

message-id: 20201028115921.848-1-rsalvaterra@gmail.com

On (20/10/28 11:59), Rui Salvaterra wrote:
> There's nothing special about zram and lzo. It works just fine without it, so
> as long as at least one of the other supported compression algorithms is
> selected.
> 
> Suggested-by: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
> Signed-off-by: Rui Salvaterra <rsalvaterra@gmail.com>

Minchan, I'm fine with the change.

Two things from my side:

1) The commit message, probably, can be a bit more informative. Something
like this?

	ZRAM always enables CRYPTO_LZO because lzo-rle is the hardcoded
	fallback compression algorithm, which means that on systems where
	ZRAM always use, for instance, CRYPTO_ZSTD lzo kernel module
	becomes unneeded. This patch removes the hardcoded lzo-lre
	dependency, instead ZRAM picks the first supported CRYPTO
	compression algorithm, should it be ZSTD or LZ4, etc; and only
	forcibly enables CRYPTO_LZO (previous behaviour) if none of the
	alternative algorithms were selected.


2) The ZRAM_AUTOSEL_ALGO allows to deselect CRYPTO_LZO only if
CRYPTO_LZ4/CRYPTO_LZ4HC/CRYPTO_842/CRYPTO_ZSTD are compiled in (=y).
If any of the algorithms is selected as a module (=m) then CRYPTO_LZO
is selected as the default algorithm. Apparently depends on !(CONFIG_FOO)
means depends on !(CONFIG_FOO=y).

It appears that the below change fixes it, but it looks a bit ugly.

---
diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
index 141ce0ebad06..f2fd34de9200 100644
--- a/drivers/block/zram/Kconfig
+++ b/drivers/block/zram/Kconfig
@@ -15,7 +15,7 @@ config ZRAM
 
 config ZRAM_AUTOSEL_ALGO
 	def_bool y
-	depends on ZRAM && !(CRYPTO_LZ4 || CRYPTO_LZ4HC || CRYPTO_842 || CRYPTO_ZSTD)
+	depends on ZRAM && !(CRYPTO_LZ4=m || CRYPTO_LZ4HC=m || CRYPTO_842=m || CRYPTO_ZSTD=m || CRYPTO_LZ4=y || CRYPTO_LZ4HC=y || CRYPTO_842=y || CRYPTO_ZSTD=y)
 	select CRYPTO_LZO
 
 config ZRAM_WRITEBACK
---

	-ss

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] zram: break the strict dependency from lzo
  2020-10-28 18:59 ` Sergey Senozhatsky
@ 2020-11-03 21:28   ` Minchan Kim
  2020-11-04 14:12     ` Rui Salvaterra
  0 siblings, 1 reply; 6+ messages in thread
From: Minchan Kim @ 2020-11-03 21:28 UTC (permalink / raw)
  To: Sergey Senozhatsky
  Cc: Rui Salvaterra, Andrew Morton, ngupta, linux-block, linux-kernel

Hi Sergey and Rui,

On Thu, Oct 29, 2020 at 03:59:27AM +0900, Sergey Senozhatsky wrote:
> Cc-ing Andrew
> 
> message-id: 20201028115921.848-1-rsalvaterra@gmail.com
> 
> On (20/10/28 11:59), Rui Salvaterra wrote:
> > There's nothing special about zram and lzo. It works just fine without it, so
> > as long as at least one of the other supported compression algorithms is
> > selected.
> > 
> > Suggested-by: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
> > Signed-off-by: Rui Salvaterra <rsalvaterra@gmail.com>

Sorry for the late. I am still looking for the time to look into this
patch.

I totally agree with the motivation of Rui. Before that, just a
dumb question.

Can't we just provide choice/endchoice in Kconfig to select default
comp algorithm from admin?

> 
> Minchan, I'm fine with the change.
> 
> Two things from my side:
> 
> 1) The commit message, probably, can be a bit more informative. Something
> like this?
> 
> 	ZRAM always enables CRYPTO_LZO because lzo-rle is the hardcoded
> 	fallback compression algorithm, which means that on systems where
> 	ZRAM always use, for instance, CRYPTO_ZSTD lzo kernel module
> 	becomes unneeded. This patch removes the hardcoded lzo-lre
> 	dependency, instead ZRAM picks the first supported CRYPTO
> 	compression algorithm, should it be ZSTD or LZ4, etc; and only
> 	forcibly enables CRYPTO_LZO (previous behaviour) if none of the
> 	alternative algorithms were selected.
> 
> 
> 2) The ZRAM_AUTOSEL_ALGO allows to deselect CRYPTO_LZO only if
> CRYPTO_LZ4/CRYPTO_LZ4HC/CRYPTO_842/CRYPTO_ZSTD are compiled in (=y).
> If any of the algorithms is selected as a module (=m) then CRYPTO_LZO
> is selected as the default algorithm. Apparently depends on !(CONFIG_FOO)
> means depends on !(CONFIG_FOO=y).
> 
> It appears that the below change fixes it, but it looks a bit ugly.
> 
> ---
> diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
> index 141ce0ebad06..f2fd34de9200 100644
> --- a/drivers/block/zram/Kconfig
> +++ b/drivers/block/zram/Kconfig
> @@ -15,7 +15,7 @@ config ZRAM
>  
>  config ZRAM_AUTOSEL_ALGO
>  	def_bool y
> -	depends on ZRAM && !(CRYPTO_LZ4 || CRYPTO_LZ4HC || CRYPTO_842 || CRYPTO_ZSTD)
> +	depends on ZRAM && !(CRYPTO_LZ4=m || CRYPTO_LZ4HC=m || CRYPTO_842=m || CRYPTO_ZSTD=m || CRYPTO_LZ4=y || CRYPTO_LZ4HC=y || CRYPTO_842=y || CRYPTO_ZSTD=y)
>  	select CRYPTO_LZO
>  
>  config ZRAM_WRITEBACK
> ---
> 
> 	-ss

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] zram: break the strict dependency from lzo
  2020-11-03 21:28   ` Minchan Kim
@ 2020-11-04 14:12     ` Rui Salvaterra
  2020-11-04 20:41       ` Minchan Kim
  0 siblings, 1 reply; 6+ messages in thread
From: Rui Salvaterra @ 2020-11-04 14:12 UTC (permalink / raw)
  To: Minchan Kim
  Cc: Sergey Senozhatsky, Andrew Morton, ngupta, linux-block, linux-kernel

Hi, Minchan,

On Tue, 3 Nov 2020 at 21:29, Minchan Kim <minchan@kernel.org> wrote:
>
> Can't we just provide choice/endchoice in Kconfig to select default
> comp algorithm from admin?

I'm fine with whatever you guys decide, just let me know what works
best for everyone.

Thanks,
Rui

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] zram: break the strict dependency from lzo
  2020-11-04 14:12     ` Rui Salvaterra
@ 2020-11-04 20:41       ` Minchan Kim
  2020-11-14  0:49         ` Sergey Senozhatsky
  0 siblings, 1 reply; 6+ messages in thread
From: Minchan Kim @ 2020-11-04 20:41 UTC (permalink / raw)
  To: Rui Salvaterra
  Cc: Sergey Senozhatsky, Andrew Morton, ngupta, linux-block, linux-kernel

On Wed, Nov 04, 2020 at 02:12:35PM +0000, Rui Salvaterra wrote:
> Hi, Minchan,
> 
> On Tue, 3 Nov 2020 at 21:29, Minchan Kim <minchan@kernel.org> wrote:
> >
> > Can't we just provide choice/endchoice in Kconfig to select default
> > comp algorithm from admin?
> 
> I'm fine with whatever you guys decide, just let me know what works
> best for everyone.

Thanks. Sergey, if you don't mined, I'd like to approach more explict
way to select default compressor algorithm in Kconfig.

Do you have any suggestion?

Thank you.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] zram: break the strict dependency from lzo
  2020-11-04 20:41       ` Minchan Kim
@ 2020-11-14  0:49         ` Sergey Senozhatsky
  0 siblings, 0 replies; 6+ messages in thread
From: Sergey Senozhatsky @ 2020-11-14  0:49 UTC (permalink / raw)
  To: Minchan Kim
  Cc: Rui Salvaterra, Sergey Senozhatsky, Andrew Morton, ngupta,
	linux-block, linux-kernel

On (20/11/04 12:41), Minchan Kim wrote:
> On Wed, Nov 04, 2020 at 02:12:35PM +0000, Rui Salvaterra wrote:
> > Hi, Minchan,
> > 
> > On Tue, 3 Nov 2020 at 21:29, Minchan Kim <minchan@kernel.org> wrote:
> > >
> > > Can't we just provide choice/endchoice in Kconfig to select default
> > > comp algorithm from admin?
> > 
> > I'm fine with whatever you guys decide, just let me know what works
> > best for everyone.
> 
> Thanks. Sergey, if you don't mined, I'd like to approach more explict
> way to select default compressor algorithm in Kconfig.
> 
> Do you have any suggestion?

No objections. Some people tend to dislike new Kconfig options,
but we probably can live with that.

	-ss

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-11-14  0:49 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-28 11:59 [PATCH v4] zram: break the strict dependency from lzo Rui Salvaterra
2020-10-28 18:59 ` Sergey Senozhatsky
2020-11-03 21:28   ` Minchan Kim
2020-11-04 14:12     ` Rui Salvaterra
2020-11-04 20:41       ` Minchan Kim
2020-11-14  0:49         ` Sergey Senozhatsky

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).