| Message ID | 20201026085141.1179-1-rsalvaterra@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] zram: break the strict dependency from lzo | expand |
On (20/10/26 08:51), Rui Salvaterra wrote: > static const char * const backends[] = { > +#if IS_ENABLED(CONFIG_CRYPTO_LZO) > "lzo", > "lzo-rle", > +#endif > #if IS_ENABLED(CONFIG_CRYPTO_LZ4) > "lz4", > #endif [..] > +static const char *default_compressor = > +#if IS_ENABLED(CONFIG_CRYPTO_LZO) > + "lzo-rle"; > +#elif IS_ENABLED(CONFIG_CRYPTO_LZ4) > + "lz4"; > +#elif IS_ENABLED(CONFIG_CRYPTO_LZ4HC) > + "lz4hc"; > +#elif IS_ENABLED(CONFIG_CRYPTO_842) > + "842"; > +#elif IS_ENABLED(CONFIG_CRYPTO_ZSTD) > + "zstd"; > +#endif Honestly, I'm not entirely excited. lzo is a fallback compression algorithm. If you want to use zram with something else thenconfigure zram to use something else. What do all these #if/#elif buy us? -ss
Hi, Sergey, On Tue, 27 Oct 2020 at 01:22, Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote: > > Honestly, I'm not entirely excited. lzo is a fallback compression > algorithm. If you want to use zram with something else thenconfigure > zram to use something else. What do all these #if/#elif buy us? The idea is to allow us to select a single compression algorithm at build time, if we're sure to use something other than lzo. The status quo only allows us to select additional algorithms, as lzo is a hard dependency. I dislike the "iffery" as much as the next guy, but in this case the default selection stops being static (as lzo may not be available at run time), so we have to fall back to an algorithm which is enabled, otherwise zram won't work out of the box (we'd always need to choose the algorithm manually in sysfs). Personally, I always use zram with zstd, and the only lzo dependency I have is zram. Disabling lzo saves me about 3 kiB in the final (xz-compressed) vmlinuz image. It's not much, for sure, but when your total storage is 4 MiB (and your RAM is 32 MiB), every bit counts. :) Thanks, Rui
Hi, On (20/10/27 08:39), Rui Salvaterra wrote: > Personally, I always use zram with zstd, and the only lzo dependency I > have is zram. Disabling lzo saves me about 3 kiB in the final > (xz-compressed) vmlinuz image. It's not much, for sure, but when your > total storage is 4 MiB (and your RAM is 32 MiB), every bit counts. :) Can the following work then? Completely untested. ===8<=== diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig index fe7a4b7d30cf..f93eed40e155 100644 --- a/drivers/block/zram/Kconfig +++ b/drivers/block/zram/Kconfig @@ -2,7 +2,7 @@ config ZRAM tristate "Compressed RAM block device support" depends on BLOCK && SYSFS && ZSMALLOC && CRYPTO - select CRYPTO_LZO + depends on (CRYPTO_LZO || CRYPTO_LZ4 || CRYPTO_LZ4HC || CRYPTO_842 || CRYPTO_ZSTD) help Creates virtual block devices called /dev/zramX (X = 0, 1, ...). Pages written to these disks are compressed and stored in memory diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c index 33e3b76c4fa9..98c7c46c9c3a 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 endup 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);
Hi, Sergey, On Wed, 28 Oct 2020 at 10:19, Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote: > > Can the following work then? Almost! See below. :) > Completely untested. > > ===8<=== > > diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig > index fe7a4b7d30cf..f93eed40e155 100644 > --- a/drivers/block/zram/Kconfig > +++ b/drivers/block/zram/Kconfig > @@ -2,7 +2,7 @@ > config ZRAM > tristate "Compressed RAM block device support" > depends on BLOCK && SYSFS && ZSMALLOC && CRYPTO > - select CRYPTO_LZO > + depends on (CRYPTO_LZO || CRYPTO_LZ4 || CRYPTO_LZ4HC || CRYPTO_842 || CRYPTO_ZSTD) This reverses the dependency order, as now we have to select a supported compression algorithm in order for zram to be visible in the block device drivers list. This is why I wrote the ZRAM_AUTOSEL_ALGO kconfig symbol, which automatically selects lzo as a fallback. If the user chooses to select another supported algorithm, he will then be allowed to deselect lzo. We thus follow the principle of least surprise, IMHO. > help > Creates virtual block devices called /dev/zramX (X = 0, 1, ...). > Pages written to these disks are compressed and stored in memory > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c > index 33e3b76c4fa9..98c7c46c9c3a 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 endup with extra loaded drivers, when the 'default' > + * compressor is not what zram is configured to use. > + */ > + return backends[0]; > +} This is much more elegant, indeed, and I completely agree with just returning the first one. I'll incorporate your feedback and send a v4 soon. Thanks a lot, Rui
Hi, On (20/10/28 11:25), Rui Salvaterra wrote: > > diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig > > index fe7a4b7d30cf..f93eed40e155 100644 > > --- a/drivers/block/zram/Kconfig > > +++ b/drivers/block/zram/Kconfig > > @@ -2,7 +2,7 @@ > > config ZRAM > > tristate "Compressed RAM block device support" > > depends on BLOCK && SYSFS && ZSMALLOC && CRYPTO > > - select CRYPTO_LZO > > + depends on (CRYPTO_LZO || CRYPTO_LZ4 || CRYPTO_LZ4HC || CRYPTO_842 || CRYPTO_ZSTD) > > This reverses the dependency order, as now we have to select a > supported compression algorithm in order for zram to be visible in the > block device drivers list. Right, but well we also need to select ZSMALLOC and CRYPTO for zram to become visible (the thing that I found out recently is that you can always check the hidden/blocked items by hitting 'z' in menuconfig). > This is why I wrote the ZRAM_AUTOSEL_ALGO > kconfig symbol, which automatically selects lzo as a fallback. If the > user chooses to select another supported algorithm, he will then be > allowed to deselect lzo. We thus follow the principle of least > surprise, IMHO. OK, makes sense. -ss
Hi, again, On Wed, 28 Oct 2020 at 18:22, Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote: > > Right, but well we also need to select ZSMALLOC and CRYPTO for > zram to become visible (the thing that I found out recently is > that you can always check the hidden/blocked items by hitting > 'z' in menuconfig). Sure, I can fix that too. Should I do it now, or wait for Andrew's/Minchan's feedback? And that 'z' key… wow. Did you read the kconfig code? I have no idea how you found that one out, but it sure deserves to be documented somewhere, it's too useful to be true. Thanks, Rui
Hi, On (20/10/28 23:11), Rui Salvaterra wrote: > Hi, again, > > On Wed, 28 Oct 2020 at 18:22, Sergey Senozhatsky > <sergey.senozhatsky.work@gmail.com> wrote: > > > > Right, but well we also need to select ZSMALLOC and CRYPTO for > > zram to become visible (the thing that I found out recently is > > that you can always check the hidden/blocked items by hitting > > 'z' in menuconfig). > > Sure, I can fix that too. Should I do it now, or wait for > Andrew's/Minchan's feedback? Let's give Minchan and Andrew some time. It's still -rc1, so we have plenty of time to land this patch. > And that 'z' key… wow. Did you read the kconfig code? By accident, "fat fingers" lol -ss
diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig index fe7a4b7d30cf..14b2b098d662 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 @@ -37,3 +36,8 @@ config ZRAM_MEMORY_TRACKING /sys/kernel/debug/zram/zramX/block_state. 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 diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c index 33e3b76c4fa9..052aa3f65514 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 diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index 1b697208d661..2ae09561ba79 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -42,7 +42,19 @@ static DEFINE_IDR(zram_index_idr); static DEFINE_MUTEX(zram_index_mutex); static int zram_major; -static const char *default_compressor = "lzo-rle"; + +static const char *default_compressor = +#if IS_ENABLED(CONFIG_CRYPTO_LZO) + "lzo-rle"; +#elif IS_ENABLED(CONFIG_CRYPTO_LZ4) + "lz4"; +#elif IS_ENABLED(CONFIG_CRYPTO_LZ4HC) + "lz4hc"; +#elif IS_ENABLED(CONFIG_CRYPTO_842) + "842"; +#elif IS_ENABLED(CONFIG_CRYPTO_ZSTD) + "zstd"; +#endif /* Module params (documentation at end) */ static unsigned int num_devices = 1;
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. Signed-off-by: Rui Salvaterra <rsalvaterra@gmail.com> --- 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 | 2 ++ drivers/block/zram/zram_drv.c | 14 +++++++++++++- 3 files changed, 20 insertions(+), 2 deletions(-)