zram-swap: default to lzo instead of lzo-rle compression

Hi,

I think commit 419f149e482641ddc520f80a7ab2038f7e2ebc8a is not the proper fix for the described issue.

The kernel module lzo-rle is still missing. To solve this, it must be installed on the root-fs:



Since kernel 4.19 isn't longer used, @ge5.1 isn't necessary.

Regards,
Hartmut

On Thu, 17 Sep 2020 at 10:38, e9hack <e9hack@gmail.com> wrote:
>
> Hi,
>
> I think commit 419f149e482641ddc520f80a7ab2038f7e2ebc8a is not the proper fix for the described issue.
>
> The kernel module lzo-rle is still missing. To solve this, it must be installed on the root-fs:
>
> diff --git a/package/kernel/linux/modules/lib.mk b/package/kernel/linux/modules/lib.mk
> index 1289cc1f25..fade8a5cfd 100644
> --- a/package/kernel/linux/modules/lib.mk
> +++ b/package/kernel/linux/modules/lib.mk
> @@ -109,9 +109,10 @@ define KernelPackage/lib-lzo
>    HIDDEN:=1
>    FILES:= \
>         $(LINUX_DIR)/crypto/lzo.ko \
> +       $(LINUX_DIR)/crypto/lzo-rle.ko@ge5.1 \
>         $(LINUX_DIR)/lib/lzo/lzo_compress.ko \
>         $(LINUX_DIR)/lib/lzo/lzo_decompress.ko
> -  AUTOLOAD:=$(call AutoProbe,lzo lzo_compress lzo_decompress)
> +  AUTOLOAD:=$(call AutoProbe,lzo lzo-rle lzo_compress lzo_decompress)
>  endef
>
>  define KernelPackage/lib-lzo/description
>
>
> Since kernel 4.19 isn't longer used, @ge5.1 isn't necessary.
>
> Regards,
> Hartmut

Good catch. You're absolutely right, the lzo-rle is a separate module.
Now, what I don't understand is why the crypto layer advertises
lzo-rle support without the module being present. Sounds like an
upstream bug to me…

Thanks,
Rui

Hi,

> -----Original Message-----
> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
> On Behalf Of e9hack
> Sent: Donnerstag, 17. September 2020 11:39
> To: jo@mein.io; rsalvaterra@gmail.com
> Cc: openwrt-devel@lists.openwrt.org
> Subject: zram-swap: default to lzo instead of lzo-rle compression
> 
> Hi,
> 
> I think commit 419f149e482641ddc520f80a7ab2038f7e2ebc8a is not the
> proper fix for the described issue.
> 
> The kernel module lzo-rle is still missing. To solve this, it must be installed on
> the root-fs:
> 
> diff --git a/package/kernel/linux/modules/lib.mk
> b/package/kernel/linux/modules/lib.mk
> index 1289cc1f25..fade8a5cfd 100644
> --- a/package/kernel/linux/modules/lib.mk
> +++ b/package/kernel/linux/modules/lib.mk
> @@ -109,9 +109,10 @@ define KernelPackage/lib-lzo
>    HIDDEN:=1
>    FILES:= \
>         $(LINUX_DIR)/crypto/lzo.ko \
> +       $(LINUX_DIR)/crypto/lzo-rle.ko@ge5.1 \
>         $(LINUX_DIR)/lib/lzo/lzo_compress.ko \
>         $(LINUX_DIR)/lib/lzo/lzo_decompress.ko
> -  AUTOLOAD:=$(call AutoProbe,lzo lzo_compress lzo_decompress)
> +  AUTOLOAD:=$(call AutoProbe,lzo lzo-rle lzo_compress lzo_decompress)
>  endef
> 
>  define KernelPackage/lib-lzo/description
> 
> 
> Since kernel 4.19 isn't longer used, @ge5.1 isn't necessary.

Kernel 4.19 support is still present in master (for regression testing), so we should properly implement the switches until it is removed entirely.

Best

Adrian

> 
> Regards,
> Hartmut
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Am Donnerstag, 17. September 2020, 11:38:42 CEST schrieb e9hack:
>The kernel module lzo-rle is still missing. To solve this, it must be
> installed on the root-fs:
> 
> diff --git a/package/kernel/linux/modules/lib.mk
> b/package/kernel/linux/modules/lib.mk index 1289cc1f25..fade8a5cfd 100644
> --- a/package/kernel/linux/modules/lib.mk
> +++ b/package/kernel/linux/modules/lib.mk
> @@ -109,9 +109,10 @@ define KernelPackage/lib-lzo
>    HIDDEN:=1
>    FILES:= \
>         $(LINUX_DIR)/crypto/lzo.ko \
> +       $(LINUX_DIR)/crypto/lzo-rle.ko@ge5.1 \
>         $(LINUX_DIR)/lib/lzo/lzo_compress.ko \
>         $(LINUX_DIR)/lib/lzo/lzo_decompress.ko
> -  AUTOLOAD:=$(call AutoProbe,lzo lzo_compress lzo_decompress)
> +  AUTOLOAD:=$(call AutoProbe,lzo lzo-rle lzo_compress lzo_decompress)
>  endef
> 

Hartmut,

I had a short test wit zram-swap these days and also saw this. 
I fixed this by changing the default compression-algo to lzo via etc/config/
system. My intention was not to use an additional kernel-module which will 
consume additional RAM which I just won by using zram ... (Indeed I did not 
check the numbers and only assumed)


Sven

Am Donnerstag, 17. September 2020, 12:11:19 CET schrieben Sie:
> On Thu, 17 Sep 2020 at 10:38, e9hack <e9hack at gmail.com> wrote:
> > Hi,
> > 
> > I think commit 419f149e482641ddc520f80a7ab2038f7e2ebc8a is not the proper
> > fix for the described issue.
> > 
> > The kernel module lzo-rle is still missing. To solve this, it must be
> > installed on the root-fs:
> > 
> 
> Good catch. You're absolutely right, the lzo-rle is a separate module.
> Now, what I don't understand is why the crypto layer advertises
> lzo-rle support without the module being present. Sounds like an
> upstream bug to me?
> 

Rui, not sure if to call it a bug. At the end there is a hardcoded default 
algo in the module, that is used initially when creating the device. The check 
for the valid algo is done later at device-activation.
I spend some time in this code and have a patch ready, which checks for algos 
before announcing them.

Sven

Hi, Sven,

On Sun, 28 Nov 2021 at 01:40, Sven Roederer <devel-sven@geroedel.de> wrote:
>
> Rui, not sure if to call it a bug. At the end there is a hardcoded default
> algo in the module, that is used initially when creating the device. The check
> for the valid algo is done later at device-activation.
> I spend some time in this code and have a patch ready, which checks for algos
> before announcing them.

It's not a bug, but it's also not exactly an unsurprising behaviour.
This is the real issue:

https://elixir.bootlin.com/linux/v5.10.82/source/crypto/Makefile#L153

obj-$(CONFIG_CRYPTO_LZO) += lzo.o lzo-rle.o

Even though they're built as separate modules, they depend on a single
kconfig symbol. Moreover, lzo-rle uses most of the original lzo
functions (adding just RLE on top), so they should arguably just be
merged. I don't know how receptive upstream is to that idea, but it
seems logical to me.

Thanks,
Rui

Am Montag, 29. November 2021, 10:57:37 CET schrieb Rui Salvaterra:
> Hi, Sven,
> 
> On Sun, 28 Nov 2021 at 01:40, Sven Roederer <devel-sven@geroedel.de> wrote:
> > Rui, not sure if to call it a bug. At the end there is a hardcoded default
> > algo in the module, that is used initially when creating the device. The
> > check for the valid algo is done later at device-activation.
> > I spend some time in this code and have a patch ready, which checks for
> > algos before announcing them.
> 
> It's not a bug, but it's also not exactly an unsurprising behaviour.
> This is the real issue:
> 
> https://elixir.bootlin.com/linux/v5.10.82/source/crypto/Makefile#L153
> 
> obj-$(CONFIG_CRYPTO_LZO) += lzo.o lzo-rle.o
> 
> Even though they're built as separate modules, they depend on a single
> kconfig symbol. Moreover, lzo-rle uses most of the original lzo
> functions (adding just RLE on top), so they should arguably just be
> merged. I don't know how receptive upstream is to that idea, but it
> seems logical to me.
> 

Rui, 

during my work I also had the impression, that both files probably share a lot 
of common code. Based on your comment I had a closer look ...
Back in 2019 (v5.1) the linux guys explicitly split the code into separate 
files to avoid potential data-corrution. 
https://github.com/torvalds/linux/commit/45ec975efb527625629d123f305

So I don't expect joining the code again will be accepted upstream.

Back to the initial issue, I just send my patch of the selection-algorithm to 
the list for tests and comments.
When it's proven to work, I'm fine with trying to push upstream.

Sven

On 2021-12-01 00:27, Sven Roederer wrote:
> Am Montag, 29. November 2021, 10:57:37 CET schrieb Rui Salvaterra:
>> Hi, Sven,
>> 
>> On Sun, 28 Nov 2021 at 01:40, Sven Roederer <devel-sven@geroedel.de> wrote:
>> > Rui, not sure if to call it a bug. At the end there is a hardcoded default
>> > algo in the module, that is used initially when creating the device. The
>> > check for the valid algo is done later at device-activation.
>> > I spend some time in this code and have a patch ready, which checks for
>> > algos before announcing them.
>> 
>> It's not a bug, but it's also not exactly an unsurprising behaviour.
>> This is the real issue:
>> 
>> https://elixir.bootlin.com/linux/v5.10.82/source/crypto/Makefile#L153
>> 
>> obj-$(CONFIG_CRYPTO_LZO) += lzo.o lzo-rle.o
>> 
>> Even though they're built as separate modules, they depend on a single
>> kconfig symbol. Moreover, lzo-rle uses most of the original lzo
>> functions (adding just RLE on top), so they should arguably just be
>> merged. I don't know how receptive upstream is to that idea, but it
>> seems logical to me.
>> 
> 
> Rui,
> 
> during my work I also had the impression, that both files probably share a lot
> of common code. Based on your comment I had a closer look ...
> Back in 2019 (v5.1) the linux guys explicitly split the code into separate
> files to avoid potential data-corrution.
> https://github.com/torvalds/linux/commit/45ec975efb527625629d123f305
> 
> So I don't expect joining the code again will be accepted upstream.
> 
> Back to the initial issue, I just send my patch of the selection-algorithm to
> the list for tests and comments.
> When it's proven to work, I'm fine with trying to push upstream.
Maybe upstream would accept a simple makefile change that merges both 
into a single module without changing any of the source files.

- Felix

Hi, Felix,

On Wed, 1 Dec 2021 at 12:45, Felix Fietkau <nbd@nbd.name> wrote:
>
> Maybe upstream would accept a simple makefile change that merges both
> into a single module without changing any of the source files.

That's the most logical course of action, yes. Otherwise, this patch
[1] I sent last year seems the next best idea.

Thanks,
Rui

[1] https://patchwork.ozlabs.org/project/openwrt/patch/20201210132825.2342-1-rsalvaterra@gmail.com/

Am Mittwoch, 1. Dezember 2021, 14:08:15 CET schrieb Rui Salvaterra:
> Hi, Felix,
> 
> On Wed, 1 Dec 2021 at 12:45, Felix Fietkau <nbd@nbd.name> wrote:
> > Maybe upstream would accept a simple makefile change that merges both
> > into a single module without changing any of the source files.
> 
> That's the most logical course of action, yes. Otherwise, this patch
> [1] I sent last year seems the next best idea.
> 
> Thanks,
> Rui
> 

As mentioned in my other mail, forcfully combining both compressors will 
result in an additional kernel-module, that gets loaded. In my case that will 
be counterproductive, as I already include "lzo" for other purpose and having 
to include "lzo-rle" too results in unwanted "memory bloat". That's why I 
think the kernel should not insist on a specific compressor by default.

Sven