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

Global IUSE=modules-compress support #34513

Closed
wants to merge 8 commits into from

Conversation

mgorny
Copy link
Member

@mgorny mgorny commented Dec 28, 2023

Add a global modules-compress flag to control kernel module compression, update the kernel eclasses to use it and make linux-mod-r1 use it. While at it, fix XZ compression in the legacy eclass.

The rough idea is that sys-kernel/gentoo-kernel (and therefore sys-kernel/gentoo-kernel-bin) are going to support XZ compression of modules starting with the next release but we don't want to force compressed modules on everyone.

@gentoo-bot
Copy link

Pull Request assignment

Submitter: @mgorny
Areas affected: eclasses, profiles
Packages affected: (none)

@gentoo/github

Linked bugs

Bugs linked: 920837

New packages

This Pull Request appears to be introducing new packages only. Due to limited manpower, adding new packages is considered low priority. This does not mean that your Pull Request will not receive any attention, however, it might take quite some time for it to be reviewed. In the meantime, your new ebuild might find a home in the GURU project repository: the ebuild repository maintained collaboratively by Gentoo users. GURU offers your ebuild a place to be reviewed and improved by other Gentoo users, while making it easy for Gentoo users to install it and enjoy the software it adds.


In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added need assignment It was impossible to assign the PR correctly. Please assign it manually. bug linked Bug/Closes found in footer, and cross-linked with the PR. labels Dec 28, 2023
@mgorny
Copy link
Member Author

mgorny commented Dec 28, 2023

CC @gentoo/dist-kernel @gentoo/kernel

Copy link
Member

@AndrewAmmerlaan AndrewAmmerlaan left a comment

Choose a reason for hiding this comment

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

Small cosmetic comment, otherwise LGTM

eclass/linux-mod-r1.eclass Outdated Show resolved Hide resolved
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-12-28 09:27 UTC
Newest commit scanned: 9357168
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/e995704115/output.html

@ionenwks
Copy link
Contributor

Hope users that use compression will actually enable it, switching compression on / off has the annoying downside of duplicating modules when rebuilding for the same kernel (one compressed, and one not, due to portage's UNINSTALL_IGNORE)

Then nvidia-drivers gets updated (same kernel), risks loading the wrong (old) one (pretty sure it ignores timestamps), and then their whole DE fails because the modules are mismatching with user space.

dist-kernel setting _XZ would've had the same problematic effect, so the IUSE itself (at least) guards users not using compression from running into that.

@ionenwks
Copy link
Contributor

Guess being exposed as an IUSE is actually making that issue worse, users may more casually flip it on/off. And not thrilled at the idea of trying to delete old modules in pkg_postinst if ever had to go there.

Not that I'm against this IUSE, just annoyed that an annoyance is becoming more annoying.

@AndrewAmmerlaan
Copy link
Member

Guess being exposed as an IUSE is actually making that issue worse, users may more casually flip it on/off. And not thrilled at the idea of trying to delete old modules in pkg_postinst if ever had to go there.

Not that I'm against this IUSE, just annoyed that an annoyance is becoming more annoying.

We could add (another) warning message about this when users switch the flag while keeping the same version. i.e.

if ( use modules-compress && ! has_version "${CATEGORY}/${PF}[modules-compress] ) || ( ! use modules-compress && ! has_version "${CATEGORY}/${PF}[-modules-compress] ) "; then
     ewarn "WARNING: Toggling USE=modules-compress on the same version will leave behind old (un)compressed modules in /lib/modules." 
fi

@mgorny
Copy link
Member Author

mgorny commented Dec 28, 2023

Yeah, I've seen that happen too. I was wondering about having a post*-phase cleanup but that could be messy.

@ionenwks
Copy link
Contributor

Yeah, a warning would be better than nothing. Albeit need to do the warning in pkg_setup given the old ${CATEGORY}/${PN} will be gone in pkg_postinst to compare (and linux-mod-r1 does not have a pkg_preinst nor pretend).

Alternative would be to search /lib/modules in postinst for identical modules while disregarding the compression extensions, and warn that something is off without deleting.

Assume dist-kernel technically have the same problem unless did some special logic (didn't look), it will duplicate all its modules when flipped for a same version -- not that it should lead to failures unless the modules changed (different CONFIG_ settings and such).

@mgorny
Copy link
Member Author

mgorny commented Dec 28, 2023

We could also do pkg_postinst that looks for duplicate modules, and removes the older file. I realize this is "unsafe" but I can't think of a valid reason why the older file would be more correct.

@mgorny
Copy link
Member Author

mgorny commented Dec 28, 2023

That said, perhaps we could move some common logic to dist-kernel-utils.eclass? This eclass doesn't set any metadata, so it should be safe to inherit from linux-mod-r1.

@ionenwks
Copy link
Contributor

That said, perhaps we could move some common logic to dist-kernel-utils.eclass? This eclass doesn't set any metadata, so it should be safe to inherit from linux-mod-r1.

Less code duplication sounds fine to me, also to keep these less error-prone if opt to delete.

Crawling the entire /lib/modules/${KV_FULL} for duplicates with every module install does feel a bit unfortunate given it can be very large though.. but eh, can't be that slow.

@ionenwks
Copy link
Contributor

ionenwks commented Dec 28, 2023

Crawling the entire /lib/modules/${KV_FULL} for duplicates with every module install does feel a bit unfortunate given it can be very large though.. but eh, can't be that slow.

Or err, well, guess it could check the image and check only these files in postinst.

Edit: does require to check this before postinst it though, e.g. set a variable in src_install. And maybe not really worth it.

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-12-28 11:12 UTC
Newest commit scanned: 8278568
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/76d8cb0cf1/output.html

@mgorny
Copy link
Member Author

mgorny commented Dec 28, 2023

Added cleanup logic. Please review again.

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-12-28 11:47 UTC
Newest commit scanned: 58d14f5
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/c590b8f5c3/output.html

Copy link
Contributor

@ionenwks ionenwks left a comment

Choose a reason for hiding this comment

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

Cleanup works as advertised and hasn't destroyed the wrong modules from a quick try.

Slight worry if someone had their clock reset to some old date and the "older" files are actually the good ones, but think I can live with that. Not like modules get constantly duplicated to have good chances to hit this.

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-12-30 05:22 UTC
Newest commit scanned: e0facb6
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/9f64fdaf8f/output.html

Add a global `modules-compress` flag that controls installing compressed
kernel modules.  The primary purpose of this flag is to make it possible
to install uncompressed modules while preserving module decompression
support in the prebuilt dist-kernel.

Signed-off-by: Michał Górny <mgorny@gentoo.org>
Signed-off-by: Michał Górny <mgorny@gentoo.org>
Add a `modules-compress` USE flag to explicitly control kernel module
compression.  When the flag is disabled, modules are installed
uncompressed even if the kernel supports compression (which is going
to be the case for new sys-kernel/gentoo-kernel* builds).  When it is
enabled, the eclass compresses modules using the compressor configured,
or fails if no compression is supported.

Signed-off-by: Michał Górny <mgorny@gentoo.org>
Match xz compression options to the ones used by the kernel,
as the xz decoder used by the kernel supports only a subset of the xz
format.

Bug: https://bugs.gentoo.org/920837
Signed-off-by: Michał Górny <mgorny@gentoo.org>
Signed-off-by: Michał Górny <mgorny@gentoo.org>
Signed-off-by: Michał Górny <mgorny@gentoo.org>
Signed-off-by: Michał Górny <mgorny@gentoo.org>
Signed-off-by: Michał Górny <mgorny@gentoo.org>
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-12-30 13:12 UTC
Newest commit scanned: ed8d768
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/8e76a6f667/output.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug linked Bug/Closes found in footer, and cross-linked with the PR. need assignment It was impossible to assign the PR correctly. Please assign it manually.
Projects
None yet
5 participants