Message-ID: <ED5FFEDC-A3B7-470E-95AE-B60EB1E6AE64@fb.com>
Date:   Thu, 26 Mar 2020 21:13:54 +0000
From:   Nick Terrell <terrelln@...com>
To:     Petr Malat <oss@...at.biz>
CC:     Nick Terrell <nickrterrell@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Chris Mason <clm@...com>,
        "linux-kbuild@...r.kernel.org" <linux-kbuild@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        Kees Cook <keescook@...omium.org>,
        Kernel Team <Kernel-team@...com>,
        Adam Borowski <kilobyte@...band.pl>,
        Patrick Williams <patrickw3@...com>,
        Michael van der Westhuizen <rmikey@...com>,
        "mingo@...nel.org" <mingo@...nel.org>,
        Patrick Williams <patrick@...cx.xyz>
Subject: Re: [PATCH v3 3/8] lib: add zstd support to decompress



> On Mar 26, 2020, at 1:16 PM, Petr Malat <oss@...at.biz> wrote:
> 
> Hi!
> On Thu, Mar 26, 2020 at 07:03:54PM +0000, Nick Terrell wrote:
>>>> * Add unzstd() and the zstd decompress interface.
>>> Here I do not understand why you limit the window size to 8MB even when
>>> you read a larger value from the header. I do not see a reason why there
>>> should be such a limitation at the first place and if there should be,
>>> why it differs from ZSTD_WINDOWLOG_MAX.
>> 
>> When we are doing streaming decompression (either flush or fill is provided)
>> we have to allocate memory proportional to the window size. We want to
>> bound that memory so we don't accidentally allocate too much memory.
>> When we are doing a single-pass decompression (neither flush nor fill
>> are provided) the window size doesn't matter, and we only have to allocate
>> a fixed amount of memory ~192 KB.
>> 
>> The zstd spec [0] specifies that all decoders should allow window sizes
>> up to 8 MB. Additionally, the zstd CLI won't produce window sizes greater
>> than 8 MB by default. The window size is controlled by the compression
>> level, and can be explicitly set.
> Yes, one needs to pass --ultra option to zstd to produce an incompatible
> archive, but that doesn't justify the reason to limit this in the kernel,
> especially if one is able to read the needed window size from the header
> when allocating the memory. At the time when initramfs is extracted,
> there usually is memory available as it's before any processes are
> started and this memory is reclaimed after the decompression.

I’m happy to increase this limit. I set it to 8 MB to be conservative, but I am
happy to increase it to 128 MB == 1 << ZSTD_WINDOWLOG_MAX. I will
submit a new version with that change.

> If, on the other hand, an user makes an initramfs for a memory constrained
> system, he limits the window size while compressing the archive and
> the small window size will be announced in the header.
> 
> The only scenario where using the hard-coded limit makes sense is in a
> case the window size is not available (I'm not sure if it's mandatory
> to provide it). That's how my code works - if the size is available,
> it uses the provided value, if not it uses 1 << ZSTD_WINDOWLOG_MAX.
> 
> I would also agree a fixed limit would make a sense if a user (or network)
> provided data would be used, but in this case only the system owner is able
> to provide an initramfs. If one is able to change initramfs, he can render
> the system unusable simply by providing a corrupted file. He doesn't have
> to bother making the window bigger than the available memory.

That makes sense to me.

>> I would expect larger window sizes to be beneficial for compression ratio,
>> though there is demising returns. I would expect that for kernel image
>> compression larger window sizes are beneficial, since it is decompressed
>> with a single pass. For initramfs decompression, I would expect that limiting
>> the window size could help decompression speed, since it uses streaming
>> compression, so unzstd() has to allocate a buffer of window size bytes.
> Yes, larger window improves the compression ratio, see here a comparison
> between level 19 and 22 on my testing x86-64 initramfs:
>  30775022 rootfs.cpio.zst-19
>  28755429 rootfs.cpio.zst-22
> These 7% can be noticeable when one has a slow storage, e.g. a flash memory
> on SPI bus. 
> 
>>> I removed that limitation to be able to test it in my environment and I
>>> found the performance is worst than with my patch by roughly 20% (on
>>> i7-3520M), which is a major drawback considering the main motivation
>>> to use zstd is the decompression speed. I will test on arm as well and
>>> share the result tomorrow.
>>> Petr
>> 
>> What do you mean by that? Can you share with me the test you ran?
>> Is this for kernel decompression or initramfs decompression?
> Initramfs - you can apply my v2 patch on v5.5 and try with your test data.
> 
> I have tested your patch also on ARMv7 platform and there the degradation
> was 8%.

Are you comparing the performance of an 8 MB window size vs a 128 MB
window size?

>  Petr

Powered by blists - more mailing lists