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: enable kcmp() system call #113236

Merged
merged 1 commit into from Feb 18, 2021
Merged

linux: enable kcmp() system call #113236

merged 1 commit into from Feb 18, 2021

Conversation

blitz
Copy link
Contributor

@blitz blitz commented Feb 15, 2021

Motivation for this change

Since 2020, Mesa requires the kcmp() system call to be available for some of its functionality. At the moment, this system call is enabled when CHECKPOINT_RESTORE is enabled in the Linux kernel. This option is also enabled by default in Fedora and Debian.

This PR enables CHECKPOINT_RESTORE by default on all kernel versions where it is not marked as EXPERT, i.e. everything after and including 4.19.

Fixes #112082.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
    • kernel-lts
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@blitz
Copy link
Contributor Author

blitz commented Feb 15, 2021

I've verified that everything <=4.14 has no config changes. The config changes for the Linux >=4.19 default config look like this:

--- config-4.19-old	2021-02-15 23:01:28.626391734 +0100
+++ result	1970-01-01 01:00:01.000000000 +0100
@@ -155,7 +155,7 @@
 CONFIG_USER_NS=y
 CONFIG_PID_NS=y
 CONFIG_NET_NS=y
-# CONFIG_CHECKPOINT_RESTORE is not set
+CONFIG_CHECKPOINT_RESTORE=y
 CONFIG_SCHED_AUTOGROUP=y
 # CONFIG_SYSFS_DEPRECATED is not set
 CONFIG_RELAY=y
@@ -998,6 +998,7 @@
 CONFIG_CLEANCACHE=y
 CONFIG_FRONTSWAP=y
 # CONFIG_CMA is not set
+# CONFIG_MEM_SOFT_DIRTY is not set
 CONFIG_ZSWAP=y
 CONFIG_ZPOOL=y
 CONFIG_ZBUD=y
@@ -8095,7 +8096,7 @@
 CONFIG_PROC_KCORE=y
 CONFIG_PROC_SYSCTL=y
 CONFIG_PROC_PAGE_MONITOR=y
-# CONFIG_PROC_CHILDREN is not set
+CONFIG_PROC_CHILDREN=y
 CONFIG_KERNFS=y
 CONFIG_SYSFS=y
 CONFIG_TMPFS=y

@vcunat
Copy link
Member

vcunat commented Feb 16, 2021

As written in this PR, features.criu doesn't change anything on newer kernels and that seems wrong (by looking at the diff; I don't know anything about criu really).

@blitz
Copy link
Contributor Author

blitz commented Feb 16, 2021

As written in this PR, features.criu doesn't change anything on newer kernels and that seems wrong (by looking at the diff; I don't know anything about criu really).

features.criu doesn't do anything on newer kernels because checkpoint/restore is always available. The diff looks ok from my end, what issue you do you see?

I refrained from enabling checkpoint/restore by default on older kernels, because there it probably was hidden on purpose behind EXPERT and might have issues. People who ask for it using programs.criu.enable still get the support either way.

@vcunat
Copy link
Member

vcunat commented Feb 16, 2021

I said it wrong. I meant features.criu_revert_expert. It did act on older kernels before this PR and that property is gone by this commit without explanation.

@blitz
Copy link
Contributor Author

blitz commented Feb 16, 2021

I said it wrong. I meant features.criu_revert_expert. It did act on older kernels before this PR and that property is gone by this commit without explanation.

This should work exactly as before unless I set the parentheses wrong.

Copy link
Member

@primeos primeos left a comment

Choose a reason for hiding this comment

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

I didn't test this but the diff LGTM :)

The current solution by @blitz looks quite elegant to me:

    criu = if (versionAtLeast version "4.19") then {
      # Unconditionally enabled, because it is required for CRIU and
      # it provides the kcmp() system call that Mesa depends on.
      CHECKPOINT_RESTORE  = yes;
    } else (optionalAttrs (features.criu or false) {
      # For older kernels, CHECKPOINT_RESTORE is hidden behind EXPERT.
      EXPERT              = yes;
      CHECKPOINT_RESTORE  = yes;
    } // optionalAttrs (features.criu_revert_expert or true) {
      RFKILL_INPUT          = option yes;
      HID_PICOLCD_FB        = option yes;
      HID_PICOLCD_BACKLIGHT = option yes;
      HID_PICOLCD_LCD       = option yes;
      HID_PICOLCD_LEDS      = option yes;
      HID_PICOLCD_CIR       = option yes;
      DEBUG_MEMORY_INIT     = option yes;
    });

So basically for Linux kernels >= 4.19 we unconditionally set CHECKPOINT_RESTORE and for older kernels the behaviour should be unchanged.
(Edit: I think I got it now, I'll have another look.)

Since 2020, Mesa requires the kcmp() system call to be available for
some of its functionality. At the moment, this system call is enabled
when CHECKPOINT_RESTORE is enabled in the Linux kernel. This option is
also enabled by default in Fedora and Debian.

This patch enables CHECKPOINT_RESTORE by default on all kernel
versions where it is not marked as EXPERT, i.e. everything after and
including 4.19.

Fixes NixOS#112082.
@primeos primeos changed the base branch from master to staging February 18, 2021 11:34
@primeos
Copy link
Member

primeos commented Feb 18, 2021

I was about to merge this and finally noticed what @vcunat meant (at least I guess that's it - nice catch btw!):

--- a/pkgs/os-specific/linux/kernel/common-config.nix
+++ b/pkgs/os-specific/linux/kernel/common-config.nix
@@ -638,7 +638,7 @@ let
       # Unconditionally enabled, because it is required for CRIU and
       # it provides the kcmp() system call that Mesa depends on.
       CHECKPOINT_RESTORE  = yes;
-    } else (optionalAttrs (features.criu or false) {
+    } else optionalAttrs (features.criu or false) ({
       # For older kernels, CHECKPOINT_RESTORE is hidden behind EXPERT.
       EXPERT              = yes;
       CHECKPOINT_RESTORE  = yes;

That way the (features.criu_revert_expert or true) part, that is added by default, will only be added if (features.criu or false) (which was the old behaviour).

I've verified that everything <=4.14 has no config changes.

I can confirm this. I didn't investigate this further but I assume the (features.criu_revert_expert or true) part matches the Linux kernel defaults (I only tested it on 4.14 though). Anyway, now we should be on the safe side :)

I also switched this PR from master to staging because most of the other PRs I've seen changing the Linux kernel configuration where merged into staging as well (though from the amount of rebuilds master should be fine AFAIK).

Copy link
Member

@primeos primeos left a comment

Choose a reason for hiding this comment

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

Let's give this a try :) Thanks a lot @blitz!

@primeos primeos merged commit 968d7e8 into NixOS:staging Feb 18, 2021
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.

Firefox hardware video acceleration failes due to missing kcmp() system call
3 participants