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

nixos/ksm: rewrite using systemd-tmpfiles #114464

Merged
merged 2 commits into from
Feb 26, 2021
Merged

nixos/ksm: rewrite using systemd-tmpfiles #114464

merged 2 commits into from
Feb 26, 2021

Conversation

rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Feb 26, 2021

Motivation for this change

This is to get rid of the ugly systemd-udev-settle dependency.
See #73095 for more information.

Things done
  • Tested via one or more NixOS test (nixosTests.ksm)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Feb 26, 2021
@flokli
Copy link
Contributor

flokli commented Feb 26, 2021

How would moving this to tmpfiles fix the problem? This seems to be a script writing to /sys, possibly requiring these files to be present (and underlying hardware to be appropriately initialized), hence the dependency on udevadm-settle.

Nothing guarantees systemd-tmpfiles runs "late enough" for this to has happened.

Can this be some udev rule instead?

@flokli flokli requested a review from aanderse February 26, 2021 16:45
@flokli
Copy link
Contributor

flokli commented Feb 26, 2021

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Feb 26, 2021

How would moving this to tmpfiles fix the problem?

It changes nothing in this regard, but I don't think there is a problem to begin with, at least not anymore. I don't see why /sys/kernel/mm/ksm/ shouldn't be available as soon as the kernel is executed.

Try building a system with:

boot.initrd.preDeviceCommands = ''
 echo 1 > /sys/kernel/mm/ksm/run; cat /sys/kernel/mm/ksm/run
'';

this runs in stage 1, before udev is even started and succeeds.

Btw, I stole the tmpfiles rule from https://wiki.archlinux.org/index.php/QEMU#Enabling_KSM.

@flokli
Copy link
Contributor

flokli commented Feb 26, 2021

In that case, I'd prefer if this could stay a shell script, and we just drop the dependency. If we do that, worst case the script will fail (and people will realize). If this is switched to a tmpfiles rule, it might do the right thing every second time, and every other time it'll try to create the underlying file structure by itself ;-)

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Feb 26, 2021

and every other time it'll try to create the underlying file structure by itself ;-)

According to tmpfiles.d(5) this should never happen: if the file does not exist it should simply fail, but ok I'll keep the script.

Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

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

don't usepkgs.lib, but lib directly

nixos/tests/ksm.nix Outdated Show resolved Hide resolved
The sysfs file /sys/kernel/mm/ksm/run seems to be available as soon as
the kernel has started, so no point in waiting for udev to "settle". If
for some reason it doesn't, we let the unit fail explicitly.
@flokli flokli merged commit d52d0a1 into NixOS:master Feb 26, 2021
@rnhmjoj rnhmjoj deleted the ksm branch July 10, 2023 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants