Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

linux kernel: prefer zstd where possible #302300

Merged
merged 2 commits into from
May 8, 2024
Merged

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Apr 7, 2024

Description of changes

Closes #302291
Closes #301536

The following things have changed:

  • For 5.11+: ZRAM compressor uses zstd now.
  • For 5.13+: kernel modules are compressed with zstd instead of xz.
  • For 5.19+: support zstd-compressed firmware.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@Ma27
Copy link
Member Author

Ma27 commented Apr 7, 2024

Going to rebuild my infra on my Hydra and boot into that kernel to see if this has some unexpected side-effects.

@Ma27
Copy link
Member Author

Ma27 commented Apr 7, 2024

First breakage I found is that images from make-disk-image.nix are broken, seeing stuff like this in the buildlog:

[    0.255498] Invalid ELF header magic: != �ELF
[    0.256614] Invalid ELF header magic: != �ELF
insmod: can't insert '/nix/store/aq5nbyxd3zsi51z1bahfq2ja07ssivqx-linux-6.1.84-shrunk/lib/module[    0.258685] Invalid ELF header magic: != �ELF
s/6.1.84/kernel/[    0.259465] Invalid ELF header magic: != �ELF

Taking a look.

EDIT: so the problem appears to be busybox not supporting zstd-compressed modules. Gonna check if there are usable patches out there, otherwise I'll see if I can get kmod into the image builder.

@Ma27
Copy link
Member Author

Ma27 commented Apr 7, 2024

Taking a look.

Have a fix locally now, but I'm too tired now to push, will try to get to it tomorrow.

@alois31
Copy link
Contributor

alois31 commented Apr 9, 2024

Has the impact on the size been measured for the kernel modules? The "comparable" compression ratio of zstd (as compared to LZMA) can be quite a bit worse (double digit percentage) depending on the levels and specifics of the uncompressed data. (This concern should not apply to the other two points; zram changes from a worse compressor and for the firmware it's only adding support.)

@Ma27
Copy link
Member Author

Ma27 commented Apr 9, 2024

Has the impact on the size been measured for the kernel modules

fwiw the stage1 modules-shrunk is 7.6M (xz) vs 9.4M (zstd) on my laptop.
the kernel-modules path of the same machine is 305M (xz) vs 339M (zstd).

Currently preparing a patch that provides zstd compression of the firmware. Here we have 492.6M (zstd) vs 440.3M (xz) though I'm playing around with different levels here.

Will probably document the exact impact in the commit message and perhaps also in the release notes.

@Ma27
Copy link
Member Author

Ma27 commented Apr 10, 2024

Pushed an improved version of the original patch that fixes the VM image builder. Pushed another commit which actually uses zstd to compress firmware.

Will rebuild my infra once again with these two patches to see if I find more problems and how well it works out.

There's a ~11% increase (440M -> 490M) in linux firmware now. Perhaps we want to make the behavior configurable here?
I played around a bit with the flags for zstd, but with a very high compression level (15) I only got it down to 483M.

Will write something to the release notes later.

Also cc @fpletz.

@alois31
Copy link
Contributor

alois31 commented Apr 10, 2024

I'd personally be more worried about the modules, since some of them go into the initrd. If I understand correctly, this is where you observed the increase from 7.6MB to 9.4MB, which seems quite bad.

@aviallon
Copy link
Contributor

Pushed an improved version of the original patch that fixes the VM image builder. Pushed another commit which actually uses zstd to compress firmware.

Will rebuild my infra once again with these two patches to see if I find more problems and how well it works out.

There's a ~11% increase (440M -> 490M) in linux firmware now. Perhaps we want to make the behavior configurable here? I played around a bit with the flags for zstd, but with a very high compression level (15) I only got it down to 483M.

Will write something to the release notes later.

Also cc @fpletz.

You could try the --long option, which should work.

zstd = {
ext = "zst";
nativeBuildInputs = [ zstd ];
cmd = file: target: ''zstd -T1 -6 --check -f "${file}" -o "${target}"'';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cmd = file: target: ''zstd -T1 -6 --check -f "${file}" -o "${target}"'';
cmd = file: target: ''zstd -T1 -19 --long --check -f "${file}" -o "${target}"'';

You can go as high as level 19, and --long should work too. This is consistent with what is configured for xz.

Copy link
Member

@lheckemann lheckemann Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could reuse/expand the data from pkgs/build-support/kernel/initrd-compressor-meta.nix?

("no, too much bikeshedding" is an acceptable answer)

@fpletz
Copy link
Member

fpletz commented Apr 11, 2024

Personally, I'ld love to see zstd support. But as others mentioned xz generally seems to have a better compression ratio than zstd. I hope the suggestions from @aviallon can make a difference.

The hit on the firmware is IMHO pretty bad since most people running on hardware will need at least one binary blob from the standard linux nonfree firmware package.

I was thinking about splitting it because some of the bigger blobs from unpopular vendors are used very rarely anyway:

[…]
   2.58 MiB result/lib/firmware/bnx2x
   3.94 MiB result/lib/firmware/radeon
   4.57 MiB result/lib/firmware/cxgb4
   4.80 MiB result/lib/firmware/liquidio
   6.08 MiB result/lib/firmware/cypress
   6.20 MiB result/lib/firmware/cirrus
   6.52 MiB result/lib/firmware/ath12k
   7.58 MiB result/lib/firmware/rtw89
   8.67 MiB result/lib/firmware/ti-connectivity
  11.99 MiB result/lib/firmware/brcm
  14.74 MiB result/lib/firmware/dpaa2
  19.44 MiB result/lib/firmware/ath10k
  20.43 MiB result/lib/firmware/qed
  25.87 MiB result/lib/firmware/i915
  34.94 MiB result/lib/firmware/mediatek
  43.37 MiB result/lib/firmware/ath11k
  58.95 MiB result/lib/firmware/intel
  60.94 MiB result/lib/firmware/amdgpu
  64.19 MiB result/lib/firmware/nvidia
  80.99 MiB result/lib/firmware/mrvl
  81.63 MiB result/lib/firmware/mellanox
 140.34 MiB result/lib/firmware/netronome
 151.34 MiB result/lib/firmware/qcom
   1.05 GiB total

For instance, we could split it loosely based on how Debian is currently doing it: https://packages.debian.org/search?suite=sid&searchon=names&keywords=firmware

@Ma27
Copy link
Member Author

Ma27 commented Apr 11, 2024

I was thinking about splitting it because some of the bigger blobs from unpopular vendors are used very rarely anyway:

fwiw that works pretty well. I use a thinclient with 4GB emmc as bluetooth/pulse/librespot-connected speaker which is why I didn't want to install linux-firmware on it. Instead, I built a derivation that only copies the relevant bluetooth parts over and called it a day.

Personally, I'ld love to see zstd support. But as others mentioned xz generally seems to have a better compression ratio than zstd. I hope the suggestions from @aviallon can make a difference.

Agreed.
I hope I'll get to it some time this week :)

@Ma27
Copy link
Member Author

Ma27 commented Apr 11, 2024

Perhaps I misremembered something: I somehow had in mind that high compression levels also affect decompression performance in zstd which is why I stopped after -15. However, -19 --long may slow down the build-time significantly (though I don't care much about that), but its decompression speed is still way faster (with a naïve timing measurement from pkgs.time at least I got 0.02s wallclock time for zstd vs. 0.27s for xz for a 13M blob with 125% (zstd) CPU vs 99% CPU (xz)). I can live with these stats.

The total difference between xz and zstd compressed firmware is now 440M vs 460M, i.e. a size increase of 4.5% (vs 11% before).

@Ma27
Copy link
Member Author

Ma27 commented Apr 12, 2024

So, with -19 --long for kernel modules (messed around in the relevant makefile fromt he kernel sourcetree) I got 10% increase of linuxKernel.kernels.linux_6_1.
Then I compressed the .ko files manually with a dictionary trained on these files before. Then I got it down to a 4% increase (though I'll need to dig a bit into it why it's not even less). That being said, I'll need to understand how the kernel does zstd decompression actually and how we can inject a dictionary into it.

@aviallon
Copy link
Contributor

It's a pity there is no Kconfig option for setting compressor's parameters. Perhaps I could try to tease upstream about that (though I have a feeling it'll get rejected).
In any case, thank you very much @Ma27 for your work 😃

pkgs/build-support/vm/default.nix Show resolved Hide resolved
pkgs/os-specific/linux/kernel/common-config.nix Outdated Show resolved Hide resolved
pkgs/build-support/kernel/modules-closure.sh Show resolved Hide resolved
@Ma27
Copy link
Member Author

Ma27 commented Apr 14, 2024

So, I need a bit of feedback on how to resume here:

zram/zswap

ZRAM_MULTI_COMP was enabled in #303821 (comment), so we need Z3FOLD here and are done with it.

firmware

I think we're good with 4.4% increase. If nobody disagrees, I think this can go to master (along woth the zram/zswap stuff).

kernel modules

I think we all agree that the state I have pushed here isn't really acceptable (almost 30% increase of linuxKernel.kernels.linux_6_1). There are three options forward:

  • Accept 11% increase we get by patching in -19 --long (as we do it for firmware blobs) via
            substituteInPlace scripts/Makefile.modinst \
            --replace-fail '$(ZSTD) -T0' '$(ZSTD) -T0 -19 --long'
    
    That'd be the by far quickest solution, but also the dirtiest.
  • Let zstd train a dictionary on build that's used to decompress kernel modules. I'm not sure if there's a reasonable way for third-party modules to benefit from that though. However, the zstd implementation of the Linux kernel appears to support dictionaries, it's not used for the module loader however. If we consider this a viable way forward I'd probably try to write a kernel patch for that.
  • (Caution: I talk about zstd not in the context of .ko files, but in the context of NARs): zstd provides seekable frames (i.e. the ability to decompress portions of a file without decompressing all of it). That could be used under the assumption that between most kernel releases a lot of bytes inside the packages don't change and as a result, Nix could be patched to only fetch ranges of a zstd-compressed NAR that have changed (e.g. by exchanging the hashes of ranges and determining what needs to be re-fetched). That would address the concerns about amount of stuff to download, storage of a binary cache, however it won't account for the disk space of a system.
    (credits to @RaitoBezarius how pointed me towards this idea)

I need a few more opinions from other people because I'm not sure what's the best option to proceed.

Opinions?
cc @lheckemann @K900 @alyssais @RaitoBezarius @fpletz @aviallon @alois31

@K900
Copy link
Contributor

K900 commented Apr 14, 2024

I don't think adding delta updates to Nix is realistic at least in the short term. I also think the size increase shouldn't be a huge deal.

@oxalica
Copy link
Contributor

oxalica commented Apr 15, 2024

zstd has the (security?) advantage of being maintained by a large company.

I agree it is better maintained than xz, but I have reservation about that general opinion. I'm not surprised by a company claiming they "forced to make data accessible by gov to satisfy a local law endorsement", though it's unlikely to do so in a compression format.

@lheckemann
Copy link
Member

but its decompression speed is still way faster

The main issue with the really high compression levels isn't speed, but memory usage -- including for decompression -- as far as I understand the manpage.

That aside, I like this!

Closes NixOS#302291
Closes NixOS#301536

The following things have changed:

* For 5.7+: ZSWAP compressor uses zstd now.
* For 5.11+: ZRAM compressor uses zstd now.
* For 5.13+: kernel modules are compressed with zstd instead of xz.
* For 5.19+: support zstd-compressed firmware.

The modules-closure functionality needed explicit support for copying
over `.zst` files. Also, the VM image builder used busybox's `insmod`
before which doesn't support zstd. Switched to `kmod` and added xz/zstd
as dependencies for it, similar to how it's done for the actual stage1
in d33e52b. The use of `kmod` here
doesn't seem to be such a big deal since it's only a build-time
dependency.
Closes NixOS#267442

    $ nix path-info -Sh /nix/store/qj1dm7wfw5m3mxf1gn3fdm0az9y1h5ny-linux-firmware-20240312-xz
    /nix/store/qj1dm7wfw5m3mxf1gn3fdm0az9y1h5ny-linux-firmware-20240312-xz	440.3M
    $ nix path-info -Sh /nix/store/c3szcjxb3g990dbiz7llwmkaf0bi98j2-linux-firmware-20240312-zstd
    /nix/store/c3szcjxb3g990dbiz7llwmkaf0bi98j2-linux-firmware-20240312-zstd	460.6M

This is an increase of 4.4%, but OTOH zstd has a significantly higher
decompression speed[1].

[1] https://gregoryszorc.com/blog/2017/03/07/better-compression-with-zstandard/
Copy link
Contributor

@Shawn8901 Shawn8901 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

running that since some days, no issues with it, lgtm

@lheckemann lheckemann merged commit a10842c into NixOS:master May 8, 2024
22 checks passed
lheckemann added a commit to lheckemann/nixpkgs that referenced this pull request May 8, 2024
The change I merged too hastily in NixOS#302300 increases the size by
~30%. This could be improved upon, but in the meantime let's go back
to xz while keeping zstd-compressed firmware (only 4.4% larger) and
_support_ for zstd-compressed modules.
lheckemann added a commit to lheckemann/nixpkgs that referenced this pull request May 8, 2024
The change I merged too hastily in NixOS#302300 increases the size by
~30%. This could be improved upon, but in the meantime let's go back
to xz while keeping zstd-compressed firmware (only 4.4% larger) and
_support_ for zstd-compressed modules.
@lheckemann
Copy link
Member

lheckemann commented May 8, 2024

I didn't mean to merge the make-modules-30%-bigger change, reverting in #310110. The rest should be fine IMO, it's an improvement overall and leaves less open WIP.

@oxalica
Copy link
Contributor

oxalica commented May 18, 2024

Could we reopen this since the change was reverted?

@Ma27
Copy link
Member Author

Ma27 commented May 20, 2024

done.

Ma27 pushed a commit to Ma27/nixpkgs that referenced this pull request May 26, 2024
The change I merged too hastily in NixOS#302300 increases the size by
~30%. This could be improved upon, but in the meantime let's go back
to xz while keeping zstd-compressed firmware (only 4.4% larger) and
_support_ for zstd-compressed modules.

(cherry picked from commit 2f04c5f)
chuangzhu added a commit to chuangzhu/mobile-nixos that referenced this pull request Jul 4, 2024
Since NixOS/nixpkgs#302300 is merged,
hardware.firmware are now compressed with zstd. On my oneplus-enchilada,
this is causing the WLAN interface disappearing. This PR fixes this by
copying the changes of kernel config from NixOS/nixpkgs#302300
chuangzhu added a commit to chuangzhu/mobile-nixos that referenced this pull request Jul 4, 2024
Since NixOS/nixpkgs#302300 is merged,
hardware.firmware are now compressed with zstd. On my oneplus-enchilada,
this is causing the WLAN interface disappearing. This PR fixes this by
copying the changes of kernel config from NixOS/nixpkgs#302300
chuangzhu added a commit to chuangzhu/mobile-nixos that referenced this pull request Jul 4, 2024
Since NixOS/nixpkgs#302300 is merged,
hardware.firmware are now compressed with zstd. On my oneplus-enchilada,
this is causing the WLAN interface disappearing. This PR fixes that by
copying the changes of kernel config from NixOS/nixpkgs#302300
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use zstd for linux kernel modules Add zstd compressed linux-firmware support