| Message ID | 20200402123712.338-1-brice@waegenei.re |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#40274,v5] gnu: Add kernel-module-loader-service. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
Hi Brice, I wonder how common it is to pass arguments to the modules explicitly in normal operation. I haven't done it often even in other distributions--and I'm a kernel hacker. See also https://linux.die.net/man/5/modprobe.d for an alternative. I'm not necessarily against doing it like you do it, but just want to bring up the possibility of just omitting the functionality and let it be someone-else's-problem, possibly another guix service that prepares /etc/modprobe.d with module options and other things (aliases, installation and removal invocations). That's also important because Linux tries to (lazy-)autoload modules whenever possible (via invoking modprobe). In that case, the argument handling would be inconsistent between if it was lazy-autoloaded compared to if it was loaded by your loader. (I even wonder if it were better for kernel-module-loader-service to read the modprobe to use from /proc/sys/kernel/modprobe in order to make the situations a little more consistent) For example let's say the following happened: (1) Linux boots up. (2) Someone accesses some device so "modprobe foo" is invoked by Linux. (3) foo is loaded, gets options from /etc/modprobe.d (usually none). [Time passes, other stuff happens] (4) Your kernel-module-loader-service is started, invokes "modprobe foo x=2". (5) x=2 is not passed to the Linux kernel module ever. I'm just saying maybe not invite this kind of trouble in the first place. I don't think it fits Guix's declarative configuration style to do that either. Also, when reconfiguring the Guix system, kernel-module-loader-service won't unload the kernel modules and thus also wouldn't load it with new options. Also, it could happen that two different guix services require the same module but with different options. That's an insane problem to have and I wouldn't try to support it. (I've reviewed your patch, otherwise OK!)
Hello Brice, Thanks for this new version. > + (guard (c ((message-condition? c) > + (display (condition-message c)) Maybe `(format (current-error-port) "~a~%" (condition-message c))' instead of display. Thanks, Mathieu
On 2020-04-02 13:56, Danny Milosavljevic wrote: > Hi Brice, > > I wonder how common it is to pass arguments to the modules explicitly > in normal > operation. > > I haven't done it often even in other distributions--and I'm a kernel > hacker. > > See also https://linux.die.net/man/5/modprobe.d for an alternative. > > I'm not necessarily against doing it like you do it, but just want to > bring up > the possibility of just omitting the functionality and let it be > someone-else's-problem, possibly another guix service that prepares > /etc/modprobe.d with module options and other things (aliases, > installation and > removal invocations). > > That's also important because Linux tries to (lazy-)autoload modules > whenever > possible (via invoking modprobe). In that case, the argument handling > would > be inconsistent between if it was lazy-autoloaded compared to if it was > loaded > by your loader. > > (I even wonder if it were better for kernel-module-loader-service to > read the > modprobe to use from /proc/sys/kernel/modprobe in order to make the > situations > a little more consistent) > > For example let's say the following happened: > > (1) Linux boots up. > (2) Someone accesses some device so "modprobe foo" is invoked by Linux. > (3) foo is loaded, gets options from /etc/modprobe.d (usually none). > [Time passes, other stuff happens] > (4) Your kernel-module-loader-service is started, invokes "modprobe foo > x=2". > (5) x=2 is not passed to the Linux kernel module ever. > > I'm just saying maybe not invite this kind of trouble in the first > place. > > I don't think it fits Guix's declarative configuration style to do that > either. > > Also, when reconfiguring the Guix system, kernel-module-loader-service > won't > unload the kernel modules and thus also wouldn't load it with new > options. > > Also, it could happen that two different guix services require the same > module > but with different options. That's an insane problem to have and I > wouldn't > try to support it. > > (I've reviewed your patch, otherwise OK!) Hello Danny, Thank for taking the time to review this patch. Since I'm definitely *not* a kernel hacker --just a casual user-- I wasn't aware of the uselessness of specifying the module arguments to modprobe in such service. I wrote this patch just to load this pesky non auto-loading `ddcci-backlight` module and I have no current use of specifying module arguments. I just thought it *could* be useful, to some, to pass arguments to modprobe since it is present in its API; but the edge-cases you brought up show that it wasn't a good idea after all. Should I just go back to the first format, with just a list of module names, and we merge this patch? Or would it be better, regarding the user interface, to start this patch anew by using `modprobe.d` API as a base. By that I mean defining a `modprobe-service-type` which populates `/etc/modprobe.d/` and can manually load a module at boot if needed (like kernel-module-loader does)? Would it be overkill? Following is an example of what such service could look like: #+begin_src scheme (service modprobe-service-type (list (modprobe-entry (module "ddcci") (load? #t) (options '("dyndbg" "delay=120")) (alises '("ddc/ci")) (install "") ; default (remove "")) ; default (modprobe-entry (module "acpi-call") (blacklist? #t)))) #+end_src - Brice
Hi Brice, On Thu, 02 Apr 2020 17:13:05 +0000 Brice Waegeneire <brice@waegenei.re> wrote: > *could* be useful, to some, to pass arguments to modprobe since it is > present in its API; Definitely could be useful. The question is whether the complexity justifies it and whether it can be reliable. > Should I just go back to the first format, with just a list of module > names, and we merge this patch? I would like that more since it's very improbable to ever need changing. >Or would it be better, regarding the > user > interface, to start this patch anew by using `modprobe.d` API as a base. > By that I mean defining a `modprobe-service-type` which populates > `/etc/modprobe.d/` and can manually load a module at boot if needed > (like > kernel-module-loader does)? Would it be overkill? Following is an > example of > what such service could look like: I think the two things (loading a module and configuring a module) can be seperate, so I wouldn't mix it. For example why would you load a module before the service that needs it actually starts up (if ever)? That just increases the attack surface for no reason. On the other hand, module configuration will probably be done as part of some guix service presence--used or not. That said, maybe there are upsides to--among other things easier-understandable configuration if it's central. > #+begin_src scheme > (service modprobe-service-type > (list (modprobe-entry > (module "ddcci") > (load? #t) > (options '("dyndbg" "delay=120")) > (alises '("ddc/ci")) > (install "") ; default > (remove "")) ; default > (modprobe-entry > (module "acpi-call") > (blacklist? #t)))) A service to do that would still be nice. But maybe "kernel-module-configuration-service" ? (Please don't think I have all the answers, let's see which way is better)
Furthermore, many modules could be built-into the kernel (instead of loadable), too. Then the "module" options somehow have to make it into the kernel command line as "module_name.parameter_name=parameter_value".
Hello, On 2020-04-03 10:20, Danny Milosavljevic wrote: > On Thu, 02 Apr 2020 17:13:05 +0000 > Brice Waegeneire <brice@waegenei.re> wrote: >> #+begin_src scheme >> (service modprobe-service-type >> (list (modprobe-entry >> (module "ddcci") >> (load? #t) >> (options '("dyndbg" "delay=120")) >> (alises '("ddc/ci")) >> (install "") ; default >> (remove "")) ; default >> (modprobe-entry >> (module "acpi-call") >> (blacklist? #t)))) > > A service to do that would still be nice. But maybe > "kernel-module-configuration-service" ? A lengthy one but a better one for sure. > (Please don't think I have all the answers, let's see which way is > better) I don't but you seems to have more answers than I have on this specific subject. We are on the same page here, the best interface should be the one that get merged! As you suggested, I choose to go back to the list of string format. It simplified the boilerplate a lot: no record and no helper function. Yet it still allows passing arguments to modules by extending `etc-service-type` to create "/etc/modprobe.d/*.conf" files, and avoids all the pitfalls of the previous method you made me aware off. As `kernel-module-loader-service-type` is extensible it would be possible, in an other patch series, if the need arises, to create a `kernel-module-configuration-service` centralizing loadable modules configuration. But like you noted it would be specific to loadable modules and not built-in ones since `kernel-arguments` is just an OS field, which AFAIK can't be influenced by a service. Thank you for your help Danny. - Brice
Hi Brice, >But like you noted it would > be specific > to loadable modules and not built-in ones since `kernel-arguments` is > just an OS > field, which AFAIK can't be influenced by a service. There's no one stopping us from extending operating-system-kernel-arguments, which already exists as a helper procedure, to also call operating-system-services and check those for the command line arguments to add (see gnu/system.scm).
On 2020-04-04 18:31, Danny Milosavljevic wrote: >> But like you noted it would >> be specific >> to loadable modules and not built-in ones since `kernel-arguments` is >> just an OS >> field, which AFAIK can't be influenced by a service. > > There's no one stopping us from extending > operating-system-kernel-arguments, > which already exists as a helper procedure, to also call > operating-system-services and check those for the command line > arguments > to add (see gnu/system.scm). Thank you for pointing this to me, I didn't thought it was working this way. So writing such a service is totally doable, interesting.
diff --git a/doc/guix.texi b/doc/guix.texi index 8cb85fe62c..ea1d363efc 100644 --- a/doc/guix.texi +++ b/doc/guix.texi @@ -76,6 +76,7 @@ Copyright @copyright{} 2020 Damien Cassou@* Copyright @copyright{} 2020 Jakub Kądziołka@* Copyright @copyright{} 2020 Jack Hill@* Copyright @copyright{} 2020 Naga Malleswari@* +Copyright @copyright{} 2020 Brice Waegeneire@* Permission is granted to copy, distribute and/or modify this document under the terms of the GNU Free Documentation License, Version 1.3 or @@ -25383,6 +25384,58 @@ notifications. @end table @end deftp +@cindex modprobe +@cindex kernel module loader +@subsubsection Kernel Module Loader Service + +The @code{kernel-module-loader-service} provides a service to load +kernel modules at boot. This is especially useful for modules that +don't autoload and need to be manually loaded, like it's the case with +@code{ddcci} for example. + +@deffn {Scheme Procedure} kernel-module-loader-service @var{modules} +Return a service that runs @command{modprobe} for each given module. +For example loading the drivers provided by @code{ddci-driver-linux} +with @code{ddcci} in debugging mode can be done as follow: + +@lisp +(use-modules (gnu)) +(use-package-modules linux) +(use-service-modules linux) +(operating-system + ... + (services (cons* (kernel-module-loader-service + '(("ddcci" "dyndbg") + ("ddcci_backlight"))) + %base-services)) + (kernel-loadable-modules (list ddcci-driver-linux))) +@end lisp +@end deffn + +@deffn {Scheme Variable} kernel-module-loader-service-type +The service type for loading kernel modules at boot with +@command{modprobe}, for modules that can't autoload. Its value must be +a @code{kernel-module-loader-configuration} object, described below. +@end deffn + +@deftp {Data Type} kernel-module-loader-configuration +The data type representing the modules to be loaded by +@command{modprobe}. +@end deftp + +@deftp {Data Type} kernel-module-loader-configuration +This is the configuration record for the +@code{kernel-module-loader-service-type}. + +@table @asis +@item @code{modprobe} (default: @code{(file-append kmod "/bin/modprobe"}) +The @command{modprobe} executable to use. + +@item @code{modules} (default: @code{'()}) +A list of list of strings specifying the modules to load with their +optional parameters. +@end table +@end deftp @node Miscellaneous Services @subsection Miscellaneous Services diff --git a/gnu/services/linux.scm b/gnu/services/linux.scm index caa0326c31..e3942f693d 100644 --- a/gnu/services/linux.scm +++ b/gnu/services/linux.scm @@ -1,5 +1,6 @@ ;;; GNU Guix --- Functional package management for GNU ;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com> +;;; Copyright © 2020 Brice Waegeneire <brice@waegenei.re> ;;; ;;; This file is part of GNU Guix. ;;; @@ -25,6 +26,8 @@ #:use-module (gnu packages linux) #:use-module (srfi srfi-1) #:use-module (srfi srfi-26) + #:use-module (srfi srfi-34) + #:use-module (srfi srfi-35) #:use-module (ice-9 match) #:export (earlyoom-configuration earlyoom-configuration? @@ -37,7 +40,14 @@ earlyoom-configuration-ignore-positive-oom-score-adj? earlyoom-configuration-show-debug-messages? earlyoom-configuration-send-notification-command - earlyoom-service-type)) + earlyoom-service-type + + kernel-module-loader-configuration + kernel-module-loader-configuration? + kernel-module-loader-configuration-modprobe + kernel-module-loader-configuration-modules + kernel-module-loader-service-type + kernel-module-loader-service)) ;;; @@ -123,3 +133,63 @@ representation." (list (service-extension shepherd-root-service-type (compose list earlyoom-shepherd-service)))) (description "Run @command{earlyoom}, the Early OOM daemon."))) + + +;;; +;;; Kernel module loader. +;;; + +(define-record-type* <kernel-module-loader-configuration> + kernel-module-loader-configuration make-kernel-module-loader-configuration + kernel-module-loader-configuration? + ;; path of the 'modprobe' command + (modprobe kernel-module-loader-configuration-modprobe + (default (file-append kmod "/bin/modprobe"))) + ;; list of lists of strings + (modules kernel-module-loader-configuration-modules + (default '()))) + +(define kernel-module-loader-shepherd-service + (match-lambda + (($ <kernel-module-loader-configuration> modprobe kernel-modules) + (list + (shepherd-service + (documentation "Load kernel modules.") + (provision '(kernel-module-loader)) + (respawn? #f) + (one-shot? #t) + (modules `((srfi srfi-34) + (srfi srfi-35) + ,@%default-modules)) + (start + #~(lambda _ + (guard (c ((message-condition? c) + (display (condition-message c)) + #f)) + (map (lambda (module-with-parameters) + (apply invoke/quiet #$modprobe "--" + module-with-parameters)) + '#$kernel-modules))))))))) + +(define kernel-module-loader-service-type + (service-type + (name 'kernel-module-loader) + (description "Load kernel modules.") + (extensions + (list + (service-extension shepherd-root-service-type + kernel-module-loader-shepherd-service))) + (compose concatenate) + (extend (lambda (config modules) + (kernel-module-loader-configuration + (inherit config) + (modules (append + (kernel-module-loader-configuration-modules config) + modules))))) + (default-value (kernel-module-loader-configuration)))) + +(define* (kernel-module-loader-service modules) + "Return a service that loads kernel MODULES." + (service kernel-module-loader-service-type + (kernel-module-loader-configuration + (modules modules)))) diff --git a/gnu/tests/linux-modules.scm b/gnu/tests/linux-modules.scm index 39e11587c6..2532850d34 100644 --- a/gnu/tests/linux-modules.scm +++ b/gnu/tests/linux-modules.scm @@ -1,6 +1,7 @@ ;;; GNU Guix --- Functional package management for GNU ;;; Copyright © 2019 Jakob L. Kreuze <zerodaysfordays@sdf.org> ;;; Copyright © 2020 Danny Milosavljevic <dannym@scratchpost.org> +;;; Copyright © 2020 Brice Waegeneire <brice@waegenei.re> ;;; ;;; This file is part of GNU Guix. ;;; @@ -19,6 +20,7 @@ (define-module (gnu tests linux-modules) #:use-module (gnu packages linux) + #:use-module (gnu services linux) #:use-module (gnu system) #:use-module (gnu system vm) #:use-module (gnu tests) @@ -37,25 +39,31 @@ ;;; ;;; Code: -(define* (module-loader-program os modules) - "Return an executable store item that, upon being evaluated, will dry-run -load MODULES." +(define* (modules-loaded?-program os modules) + "Return an executable store item that, upon being evaluated, will verify +that MODULES are actually loaded." (program-file "load-kernel-modules.scm" - (with-imported-modules (source-module-closure '((guix build utils))) - #~(begin - (use-modules (guix build utils)) - (for-each (lambda (module) - (invoke (string-append #$kmod "/bin/modprobe") "-n" "--" - module)) - '#$modules))))) + #~(begin + (use-modules (ice-9 rdelim) + (ice-9 popen)) + (let* ((port (open-input-pipe (string-append #$kmod "/bin/lsmod"))) + (output (read-string port)) + (status (close-pipe port))) + (and-map (lambda (module) (string-contains output module)) + '#$modules))))) -(define* (run-loadable-kernel-modules-test module-packages module-names) - "Run a test of an OS having MODULE-PACKAGES, and modprobe MODULE-NAMES." +(define* (run-loadable-kernel-modules-test module-packages + modules-with-parameters) + "Run a test of an OS having MODULE-PACKAGES, and verify that +MODULES-WITH-PARAMETERS are loaded in memory." (define os (marionette-operating-system (operating-system (inherit (simple-operating-system)) + (services (cons (kernel-module-loader-service modules-with-parameters) + (operating-system-user-services + (simple-operating-system)))) (kernel-loadable-modules module-packages)) #:imported-modules '((guix combinators)))) (define vm (virtual-machine os)) @@ -75,7 +83,9 @@ load MODULES." marionette)) (test-end) (exit (= (test-runner-fail-count (test-runner-current)) 0))))) - (gexp->derivation "loadable-kernel-modules" (test (module-loader-program os module-names)))) + (gexp->derivation + "loadable-kernel-modules" + (test (modules-loaded?-program os (map car modules-with-parameters))))) (define %test-loadable-kernel-modules-0 (system-test @@ -88,16 +98,17 @@ with no extra modules.") (system-test (name "loadable-kernel-modules-1") (description "Tests loadable kernel modules facility of <operating-system> -with one extra module.") +with one extra module with some parameters.") (value (run-loadable-kernel-modules-test (list ddcci-driver-linux) - '("ddcci"))))) + '(("ddcci" "dyndbg" "delay=120")))))) (define %test-loadable-kernel-modules-2 (system-test (name "loadable-kernel-modules-2") (description "Tests loadable kernel modules facility of <operating-system> -with two extra modules.") +with two extra modules without any parameters.") (value (run-loadable-kernel-modules-test (list acpi-call-linux-module ddcci-driver-linux) - '("acpi_call" "ddcci"))))) + '(("acpi_call") + ("ddcci"))))))