Re: [PATCHv7 10/24] zram: add zlib compression backend support

Re: [PATCHv7 10/24] zram: add zlib compression backend support

[Zaslonko Mikhail]

Hello Sergey,

On 02.09.2024 12:55, Sergey Senozhatsky wrote:
> Add s/w zlib (inflate/deflate) compression.
> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---

> diff --git a/drivers/block/zram/backend_deflate.c b/drivers/block/zram/backend_deflate.c
> new file mode 100644
> index 000000000000..acefb86701b9
> --- /dev/null
> +++ b/drivers/block/zram/backend_deflate.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +#include <linux/zlib.h>
> +
> +#include "backend_deflate.h"
> +
> +/* Use the same value as crypto API */
> +#define DEFLATE_DEF_WINBITS		11
> +#define DEFLATE_DEF_MEMLEVEL		MAX_MEM_LEVEL
> +

I was trying to use a zram device with a deflate compression algorithm tuned for s390 hardware acceleration. While I was able to set the compression level via algorithm_params sysfs attribute, I figured out that the window size used by zram zlib compression is limited to 4K since windowBits parameter is hardcoded to DEFLATE_DEF_WINBITS. In order to utilize s390 hardware deflate acceleration, the maximum window size is needed (windowBits of 15), which is also a zlib default. Thus, I'm wondering why windowBits value of 11 was picked for zram zlib compression backend support. The comment line says 'Use the same value as crypto API'... could you please clarify here? Are there some memory constraints?

I tried to build the kernel with DEFLATE_DEF_WINBITS set to 15 and verified that s390 hardware deflate acceleration works for zram devices with a deflate compression.

Thanks,
Mikhail

Re: [PATCHv7 10/24] zram: add zlib compression backend support

[Sergey Senozhatsky]

Hello,

On (25/05/08 16:19), Zaslonko Mikhail wrote:
[..]
> > +#include "backend_deflate.h"
> > +
> > +/* Use the same value as crypto API */
> > +#define DEFLATE_DEF_WINBITS		11
> > +#define DEFLATE_DEF_MEMLEVEL		MAX_MEM_LEVEL
> > +
[..]
> The comment line says 'Use the same value as crypto API'...
> could you please clarify here? Are there some memory constraints?

When zram transitioned from Crypto API (scomp) to custom compression
API I picked the CryptoAPI deflate DEFLATE_DEF_WINBITS value:

crypto/deflate.c: DEFLATE_DEF_WINBITS	11

which is then passed to zlib_deflateInit2() and zlib_inflateInit2().

> I tried to build the kernel with DEFLATE_DEF_WINBITS set to 15 and
> verified that s390 hardware deflate acceleration works for zram devices
> with a deflate compression.

If we define it as 15 on non-s390 machines, will there be any
consequences?  Increased memory usage?  By how much?

Re: [PATCHv7 10/24] zram: add zlib compression backend support

[Zaslonko Mikhail]

Hello,

On 09.05.2025 03:38, Sergey Senozhatsky wrote:
> Hello,
> 
> On (25/05/08 16:19), Zaslonko Mikhail wrote:
> [..]
>>> +#include "backend_deflate.h"
>>> +
>>> +/* Use the same value as crypto API */
>>> +#define DEFLATE_DEF_WINBITS		11
>>> +#define DEFLATE_DEF_MEMLEVEL		MAX_MEM_LEVEL
>>> +
> [..]
>> The comment line says 'Use the same value as crypto API'...
>> could you please clarify here? Are there some memory constraints?
> 
> When zram transitioned from Crypto API (scomp) to custom compression
> API I picked the CryptoAPI deflate DEFLATE_DEF_WINBITS value:
> 
> crypto/deflate.c: DEFLATE_DEF_WINBITS	11
> 
> which is then passed to zlib_deflateInit2() and zlib_inflateInit2().
> 
>> I tried to build the kernel with DEFLATE_DEF_WINBITS set to 15 and
>> verified that s390 hardware deflate acceleration works for zram devices
>> with a deflate compression.
> 
> If we define it as 15 on non-s390 machines, will there be any
> consequences?  Increased memory usage?  By how much?

On s390, setting windowBits to 15 would lead to zlib workarea size
increased by 120K (0x24dc8 -> 0x42dc8) per compression stream,
i.e. per online CPU. 
On non-s390 machine, that impact will be about 115K per stream. 
Increasing window size should improve deflate compression,
although the compression speed might be affected. Couldn't find any
relevant zlib benchmarks though.

Not sure what other consequences might there be for zram. Do you see any?

> 
Thanks,
Mikhail

Re: [PATCHv7 10/24] zram: add zlib compression backend support

[Sergey Senozhatsky]

Sorry for the delay,

On (25/05/09 17:18), Zaslonko Mikhail wrote:
> > When zram transitioned from Crypto API (scomp) to custom compression
> > API I picked the CryptoAPI deflate DEFLATE_DEF_WINBITS value:
> > 
> > crypto/deflate.c: DEFLATE_DEF_WINBITS	11
> > 
> > which is then passed to zlib_deflateInit2() and zlib_inflateInit2().
> > 
> >> I tried to build the kernel with DEFLATE_DEF_WINBITS set to 15 and
> >> verified that s390 hardware deflate acceleration works for zram devices
> >> with a deflate compression.
> > 
> > If we define it as 15 on non-s390 machines, will there be any
> > consequences?  Increased memory usage?  By how much?
> 
> On s390, setting windowBits to 15 would lead to zlib workarea size
> increased by 120K (0x24dc8 -> 0x42dc8) per compression stream,
> i.e. per online CPU. 
> On non-s390 machine, that impact will be about 115K per stream. 
> Increasing window size should improve deflate compression,
> although the compression speed might be affected. Couldn't find any
> relevant zlib benchmarks though.
> 
> Not sure what other consequences might there be for zram. Do you see any?

The increased per-CPU memory usage is the only thing I can think of.
I guess for zram we could turn this into a run-time parameter, but for
Crypto API compile-time is the only option, I guess.

Can you send a patch series (for zram and Crypto API) that sets
windowBits to 15?

Re: [PATCHv7 10/24] zram: add zlib compression backend support

[Zaslonko Mikhail]

Hello,

On 13.05.2025 07:41, Sergey Senozhatsky wrote:
> Sorry for the delay,
> 
> On (25/05/09 17:18), Zaslonko Mikhail wrote:
>>> When zram transitioned from Crypto API (scomp) to custom compression
>>> API I picked the CryptoAPI deflate DEFLATE_DEF_WINBITS value:
>>>
>>> crypto/deflate.c: DEFLATE_DEF_WINBITS	11
>>>
>>> which is then passed to zlib_deflateInit2() and zlib_inflateInit2().
>>>
>>>> I tried to build the kernel with DEFLATE_DEF_WINBITS set to 15 and
>>>> verified that s390 hardware deflate acceleration works for zram devices
>>>> with a deflate compression.
>>>
>>> If we define it as 15 on non-s390 machines, will there be any
>>> consequences?  Increased memory usage?  By how much?
>>
>> On s390, setting windowBits to 15 would lead to zlib workarea size
>> increased by 120K (0x24dc8 -> 0x42dc8) per compression stream,
>> i.e. per online CPU. 
>> On non-s390 machine, that impact will be about 115K per stream. 
>> Increasing window size should improve deflate compression,
>> although the compression speed might be affected. Couldn't find any
>> relevant zlib benchmarks though.
>>
>> Not sure what other consequences might there be for zram. Do you see any?
> 
> The increased per-CPU memory usage is the only thing I can think of.
> I guess for zram we could turn this into a run-time parameter, but for
> Crypto API compile-time is the only option, I guess.

With 'run-time parameter' you mean adding 'windowBits' as another deflate compression
algorithm parameter for zram? Guess we could do this, using default value of 15 then.

> 
> Can you send a patch series (for zram and Crypto API) that sets
> windowBits to 15?

I can do it for zram. Not sure if Crypto should be changed as well. Or is it
supposed to have the same compression defaults as zram?

Thanks,
Mikhail

Re: [PATCHv7 10/24] zram: add zlib compression backend support

[Sergey Senozhatsky]

On (25/05/13 14:58), Zaslonko Mikhail wrote:
> On 13.05.2025 07:41, Sergey Senozhatsky wrote:
> > Sorry for the delay,
> > 
> > On (25/05/09 17:18), Zaslonko Mikhail wrote:
> >>> When zram transitioned from Crypto API (scomp) to custom compression
> >>> API I picked the CryptoAPI deflate DEFLATE_DEF_WINBITS value:
> >>>
> >>> crypto/deflate.c: DEFLATE_DEF_WINBITS	11
> >>>
> >>> which is then passed to zlib_deflateInit2() and zlib_inflateInit2().
> >>>
> >>>> I tried to build the kernel with DEFLATE_DEF_WINBITS set to 15 and
> >>>> verified that s390 hardware deflate acceleration works for zram devices
> >>>> with a deflate compression.
> >>>
> >>> If we define it as 15 on non-s390 machines, will there be any
> >>> consequences?  Increased memory usage?  By how much?
> >>
> >> On s390, setting windowBits to 15 would lead to zlib workarea size
> >> increased by 120K (0x24dc8 -> 0x42dc8) per compression stream,
> >> i.e. per online CPU. 
> >> On non-s390 machine, that impact will be about 115K per stream. 
> >> Increasing window size should improve deflate compression,
> >> although the compression speed might be affected. Couldn't find any
> >> relevant zlib benchmarks though.
> >>
> >> Not sure what other consequences might there be for zram. Do you see any?
> > 
> > The increased per-CPU memory usage is the only thing I can think of.
> > I guess for zram we could turn this into a run-time parameter, but for
> > Crypto API compile-time is the only option, I guess.
> 
> With 'run-time parameter' you mean adding 'windowBits' as another deflate compression
> algorithm parameter for zram? Guess we could do this, using default value of 15 then.

I sent a simple patch set [1] that adds deflate.winbits parameter
support, so that it can be configured at runtime.  E.g.:

	echo "priority=1 deflate.winbits=15" > /sys/block/zram0/algorithm_params

Please take a look.

> > Can you send a patch series (for zram and Crypto API) that sets
> > windowBits to 15?
> 
> I can do it for zram. Not sure if Crypto should be changed as well. Or is it
> supposed to have the same compression defaults as zram?

Crypto API uses a hard-coded value of -11, which wouldn't work in your
case.  However, if you don't use crypto API (e.g. zswap) in your setup
then it probably doesn't need to be patched.  Cc-ed Herbert from the
crypto API side, just in case.

[1] https://lore.kernel.org/linux-kernel/20250514024825.1745489-1-senozhatsky@chromium.org

Re: [PATCHv7 10/24] zram: add zlib compression backend support

[Herbert Xu]

On Wed, May 14, 2025 at 11:56:38AM +0900, Sergey Senozhatsky wrote:
>
> Crypto API uses a hard-coded value of -11, which wouldn't work in your
> case.  However, if you don't use crypto API (e.g. zswap) in your setup
> then it probably doesn't need to be patched.  Cc-ed Herbert from the
> crypto API side, just in case.
> 
> [1] https://lore.kernel.org/linux-kernel/20250514024825.1745489-1-senozhatsky@chromium.org

Once we have parameter support in acomp you should be able to set
this as an optional parameter.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt