unofficial mirror of guix-patches@gnu.org 
 help / color / Atom feed
* [bug#42123] [PATCH] linux-libre: Enable module compression.
@ 2020-06-29 14:24 Mathieu Othacehe
  2020-06-30  7:31 ` Mathieu Othacehe
  2020-07-02 10:23 ` Ludovic Courtès
  0 siblings, 2 replies; 8+ messages in thread
From: Mathieu Othacehe @ 2020-06-29 14:24 UTC (permalink / raw)
  To: 42123; +Cc: Mathieu Othacehe

This commit enables GZIP compression for linux-libre kernel modules, reducing
the size of linux-libre by 63% (165MB). The initrd modules are kept
uncompressed as the initrd is already compressed as a whole.

The linux-libre kernel also supports XZ compression, but as Guix does not have
any available bindings for now, and the compression time is far more
significant, GZIP seems to be a better option.

* gnu/packages/aux-files/linux-libre/5.4-arm.conf: Enable GZ compression.
* gnu/packages/aux-files/linux-libre/5.4-arm64.conf: Ditto.
* gnu/packages/aux-files/linux-libre/5.4-i686.conf: Ditto.
* gnu/packages/aux-files/linux-libre/5.4-x86_64.conf: Ditto.
* gnu/build/linux-modules.scm (modinfo-section-contents): Use
'call-with-gzip-input-port' to read from a module file using '.gz' extension,
(strip-extension): new procedure,
(dot-ko): adapt to support compression,
(ensure-dot-ko): ditto,
(file-name->module-name): ditto,
(find-module-file): ditto,
(load-linux-module*): ditto,
(module-name->file-name/guess): ditto,
(module-name-lookup): ditto,
(write-module-name-database): ditto,
(write-module-alias-database): ditto,
(write-module-device-database): ditto.
* gnu/system/linux-initrd.scm (flat-linux-module-directory): Make sure that
zlib bindings are available because they may be used in
'write-module-device-database'. Also make sure that the initrd only contains
uncompressed module files.
---
Hello,

I think the commit message pretty much describes this change. Just wanted to
add that it passes the loadable-kernel-modules-X tests.

Thanks,

Mathieu

 gnu/build/linux-modules.scm                   | 102 ++++++++++++------
 .../aux-files/linux-libre/5.4-arm.conf        |   4 +-
 .../aux-files/linux-libre/5.4-arm64.conf      |   4 +-
 .../aux-files/linux-libre/5.4-i686.conf       |   4 +-
 .../aux-files/linux-libre/5.4-x86_64.conf     |   4 +-
 gnu/system/linux-initrd.scm                   |  38 +++++--
 6 files changed, 111 insertions(+), 45 deletions(-)

diff --git a/gnu/build/linux-modules.scm b/gnu/build/linux-modules.scm
index aa1c7cfeae..7c6945a881 100644
--- a/gnu/build/linux-modules.scm
+++ b/gnu/build/linux-modules.scm
@@ -21,6 +21,7 @@
 (define-module (gnu build linux-modules)
   #:use-module (guix elf)
   #:use-module (guix glob)
+  #:use-module (guix zlib)
   #:use-module (guix build syscalls)
   #:use-module ((guix build utils) #:select (find-files invoke))
   #:use-module (guix build union)
@@ -97,7 +98,16 @@ string list."
 (define (modinfo-section-contents file)
   "Return the contents of the '.modinfo' section of FILE as a list of
 key/value pairs.."
-  (let* ((bv      (call-with-input-file file get-bytevector-all))
+  (define (get-bytevector file)
+    (cond
+     ((string-contains file ".ko.gz")
+      (call-with-input-file file
+        (lambda (port)
+          (call-with-gzip-input-port port get-bytevector-all))))
+     (else
+      (call-with-input-file file get-bytevector-all))))
+
+  (let* ((bv      (get-bytevector file))
          (elf     (parse-elf bv))
          (section (elf-section-by-name elf ".modinfo"))
          (modinfo (section-contents elf section)))
@@ -110,7 +120,7 @@ key/value pairs.."
 (define (module-formal-name file)
   "Return the module name of FILE as it appears in its info section.  Usually
 the module name is the same as the base name of FILE, modulo hyphens and minus
-the \".ko\" extension."
+the \".ko[.gz|.xz]\" extension."
   (match (assq 'name (modinfo-section-contents file))
     (('name . name) name)
     (#f #f)))
@@ -171,14 +181,25 @@ modules that can be postloaded, of the soft dependencies of module FILE."
                  (_ #f))
                 (modinfo-section-contents file))))
 
-(define dot-ko
-  (cut string-append <> ".ko"))
-
-(define (ensure-dot-ko name)
-  "Return NAME with a '.ko' prefix appended, unless it already has it."
-  (if (string-suffix? ".ko" name)
+(define (strip-extension filename)
+  (let ((extension (string-index filename #\.)))
+    (if extension
+        (string-take filename extension)
+        filename)))
+
+(define (dot-ko name compression)
+  (let ((suffix (match compression
+                  ('xz   ".ko.xz")
+                  ('gzip ".ko.gz")
+                  (else  ".ko"))))
+    (string-append name suffix)))
+
+(define (ensure-dot-ko name compression)
+  "Return NAME with a '.ko[.gz|.xz]' suffix appended, unless it already has
+it."
+  (if (string-contains name ".ko")
       name
-      (dot-ko name)))
+      (dot-ko name compression)))
 
 (define (normalize-module-name module)
   "Return the \"canonical\" name for MODULE, replacing hyphens with
@@ -191,9 +212,9 @@ underscores."
               module))
 
 (define (file-name->module-name file)
-  "Return the module name corresponding to FILE, stripping the trailing '.ko'
-and normalizing it."
-  (normalize-module-name (basename file ".ko")))
+  "Return the module name corresponding to FILE, stripping the trailing
+'.ko[.gz|.xz]' and normalizing it."
+  (normalize-module-name (strip-extension (basename file))))
 
 (define (find-module-file directory module)
   "Lookup module NAME under DIRECTORY, and return its absolute file name.
@@ -208,19 +229,19 @@ whereas file names often, but not always, use hyphens.  Examples:
     ;; List of possible file names.  XXX: It would of course be cleaner to
     ;; have a database that maps module names to file names and vice versa,
     ;; but everyone seems to be doing hacks like this one.  Oh well!
-    (map ensure-dot-ko
-         (delete-duplicates
-          (list module
-                (normalize-module-name module)
-                (string-map (lambda (chr) ;converse of 'normalize-module-name'
-                              (case chr
-                                ((#\_) #\-)
-                                (else chr)))
-                            module)))))
+    (delete-duplicates
+     (list module
+           (normalize-module-name module)
+           (string-map (lambda (chr) ;converse of 'normalize-module-name'
+                         (case chr
+                           ((#\_) #\-)
+                           (else chr)))
+                       module))))
 
   (match (find-files directory
                      (lambda (file stat)
-                       (member (basename file) names)))
+                       (member (strip-extension
+                                (basename file)) names)))
     ((file)
      file)
     (()
@@ -290,8 +311,8 @@ not a file name."
                              (recursive? #t)
                              (lookup-module dot-ko)
                              (black-list (module-black-list)))
-  "Load Linux module from FILE, the name of a '.ko' file; return true on
-success, false otherwise.  When RECURSIVE? is true, load its dependencies
+  "Load Linux module from FILE, the name of a '.ko[.gz|.xz]' file; return true
+on success, false otherwise.  When RECURSIVE? is true, load its dependencies
 first (à la 'modprobe'.)  The actual files containing modules depended on are
 obtained by calling LOOKUP-MODULE with the module name.  Modules whose name
 appears in BLACK-LIST are not loaded."
@@ -523,16 +544,27 @@ are required to access DEVICE."
 ;;; Module databases.
 ;;;
 
-(define (module-name->file-name/guess directory name)
+(define* (module-name->file-name/guess directory name
+                                       #:key compression)
   "Guess the file name corresponding to NAME, a module name.  That doesn't
 always work because sometimes underscores in NAME map to hyphens (e.g.,
 \"input-leds.ko\"), sometimes not (e.g., \"mac_hid.ko\")."
-  (string-append directory "/" (ensure-dot-ko name)))
+  (string-append directory "/" (ensure-dot-ko name compression)))
 
 (define (module-name-lookup directory)
   "Return a one argument procedure that takes a module name (e.g.,
 \"input_leds\") and returns its absolute file name (e.g.,
 \"/.../input-leds.ko\")."
+  (define (guess-file-name name)
+    (let ((names (list
+                  (module-name->file-name/guess directory name)
+                  (module-name->file-name/guess directory name
+                                                #:compression 'xz)
+                  (module-name->file-name/guess directory name
+                                                #:compression 'gzip))))
+      (or (find file-exists? names)
+          (first names))))
+
   (catch 'system-error
     (lambda ()
       (define mapping
@@ -541,23 +573,23 @@ always work because sometimes underscores in NAME map to hyphens (e.g.,
 
       (lambda (name)
         (or (assoc-ref mapping name)
-            (module-name->file-name/guess directory name))))
+            (guess-file-name name))))
     (lambda args
       (if (= ENOENT (system-error-errno args))
-          (cut module-name->file-name/guess directory <>)
+          (cut guess-file-name <>)
           (apply throw args)))))
 
 (define (write-module-name-database directory)
   "Write a database that maps \"module names\" as they appear in the relevant
-ELF section of '.ko' files, to actual file names.  This format is
+ELF section of '.ko[.gz|.xz]' files, to actual file names.  This format is
 Guix-specific.  It aims to deal with inconsistent naming, in particular
 hyphens vs. underscores."
   (define mapping
     (map (lambda (file)
            (match (module-formal-name file)
-             (#f   (cons (basename file ".ko") file))
+             (#f   (cons (strip-extension (basename file)) file))
              (name (cons name file))))
-         (find-files directory "\\.ko$")))
+         (find-files directory "\\.ko.*$")))
 
   (call-with-output-file (string-append directory "/modules.name")
     (lambda (port)
@@ -569,12 +601,12 @@ hyphens vs. underscores."
       (pretty-print mapping port))))
 
 (define (write-module-alias-database directory)
-  "Traverse the '.ko' files in DIRECTORY and create the corresponding
+  "Traverse the '.ko[.gz|.xz]' files in DIRECTORY and create the corresponding
 'modules.alias' file."
   (define aliases
     (map (lambda (file)
            (cons (file-name->module-name file) (module-aliases file)))
-         (find-files directory "\\.ko$")))
+         (find-files directory "\\.ko.*$")))
 
   (call-with-output-file (string-append directory "/modules.alias")
     (lambda (port)
@@ -616,7 +648,7 @@ are found, return a tuple (DEVNAME TYPE MAJOR MINOR), otherwise return #f."
   (char-set-complement (char-set #\-)))
 
 (define (write-module-device-database directory)
-  "Traverse the '.ko' files in DIRECTORY and create the corresponding
+  "Traverse the '.ko[.gz|.xz]' files in DIRECTORY and create the corresponding
 'modules.devname' file.  This file contains information about modules that can
 be loaded on-demand, such as file system modules."
   (define aliases
@@ -624,7 +656,7 @@ be loaded on-demand, such as file system modules."
                   (match (aliases->device-tuple (module-aliases file))
                     (#f #f)
                     (tuple (cons (file-name->module-name file) tuple))))
-                (find-files directory "\\.ko$")))
+                (find-files directory "\\.ko.*$")))
 
   (call-with-output-file (string-append directory "/modules.devname")
     (lambda (port)
diff --git a/gnu/packages/aux-files/linux-libre/5.4-arm.conf b/gnu/packages/aux-files/linux-libre/5.4-arm.conf
index a54228643b..7c9ab94719 100644
--- a/gnu/packages/aux-files/linux-libre/5.4-arm.conf
+++ b/gnu/packages/aux-files/linux-libre/5.4-arm.conf
@@ -880,7 +880,9 @@ CONFIG_MODULE_FORCE_UNLOAD=y
 CONFIG_MODVERSIONS=y
 # CONFIG_MODULE_SRCVERSION_ALL is not set
 # CONFIG_MODULE_SIG is not set
-# CONFIG_MODULE_COMPRESS is not set
+CONFIG_MODULE_COMPRESS=y
+CONFIG_MODULE_COMPRESS_GZIP=y
+# CONFIG_MODULE_COMPRESS_XZ is not set
 # CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS is not set
 # CONFIG_UNUSED_SYMBOLS is not set
 # CONFIG_TRIM_UNUSED_KSYMS is not set
diff --git a/gnu/packages/aux-files/linux-libre/5.4-arm64.conf b/gnu/packages/aux-files/linux-libre/5.4-arm64.conf
index fabd25c6a4..6520d1ddf2 100644
--- a/gnu/packages/aux-files/linux-libre/5.4-arm64.conf
+++ b/gnu/packages/aux-files/linux-libre/5.4-arm64.conf
@@ -781,7 +781,9 @@ CONFIG_MODVERSIONS=y
 CONFIG_ASM_MODVERSIONS=y
 # CONFIG_MODULE_SRCVERSION_ALL is not set
 # CONFIG_MODULE_SIG is not set
-# CONFIG_MODULE_COMPRESS is not set
+CONFIG_MODULE_COMPRESS=y
+CONFIG_MODULE_COMPRESS_GZIP=y
+# CONFIG_MODULE_COMPRESS_XZ is not set
 # CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS is not set
 # CONFIG_UNUSED_SYMBOLS is not set
 # CONFIG_TRIM_UNUSED_KSYMS is not set
diff --git a/gnu/packages/aux-files/linux-libre/5.4-i686.conf b/gnu/packages/aux-files/linux-libre/5.4-i686.conf
index 50a41f6cc9..3727f9d486 100644
--- a/gnu/packages/aux-files/linux-libre/5.4-i686.conf
+++ b/gnu/packages/aux-files/linux-libre/5.4-i686.conf
@@ -850,7 +850,9 @@ CONFIG_MODVERSIONS=y
 CONFIG_ASM_MODVERSIONS=y
 CONFIG_MODULE_SRCVERSION_ALL=y
 # CONFIG_MODULE_SIG is not set
-# CONFIG_MODULE_COMPRESS is not set
+CONFIG_MODULE_COMPRESS=y
+CONFIG_MODULE_COMPRESS_GZIP=y
+# CONFIG_MODULE_COMPRESS_XZ is not set
 # CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS is not set
 CONFIG_UNUSED_SYMBOLS=y
 CONFIG_MODULES_TREE_LOOKUP=y
diff --git a/gnu/packages/aux-files/linux-libre/5.4-x86_64.conf b/gnu/packages/aux-files/linux-libre/5.4-x86_64.conf
index 30fdc4e86a..be7a603af1 100644
--- a/gnu/packages/aux-files/linux-libre/5.4-x86_64.conf
+++ b/gnu/packages/aux-files/linux-libre/5.4-x86_64.conf
@@ -849,7 +849,9 @@ CONFIG_MODVERSIONS=y
 CONFIG_ASM_MODVERSIONS=y
 CONFIG_MODULE_SRCVERSION_ALL=y
 # CONFIG_MODULE_SIG is not set
-# CONFIG_MODULE_COMPRESS is not set
+CONFIG_MODULE_COMPRESS=y
+CONFIG_MODULE_COMPRESS_GZIP=y
+# CONFIG_MODULE_COMPRESS_XZ is not set
 # CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS is not set
 CONFIG_UNUSED_SYMBOLS=y
 CONFIG_MODULES_TREE_LOOKUP=y
diff --git a/gnu/system/linux-initrd.scm b/gnu/system/linux-initrd.scm
index 0971ec29e2..99ec82246b 100644
--- a/gnu/system/linux-initrd.scm
+++ b/gnu/system/linux-initrd.scm
@@ -111,11 +111,29 @@ the derivations referenced by EXP are automatically copied to the initrd."
 (define (flat-linux-module-directory linux modules)
   "Return a flat directory containing the Linux kernel modules listed in
 MODULES and taken from LINUX."
+  (define zlib
+    (module-ref (resolve-interface '(gnu packages compression)) 'zlib))
+
+  (define config.scm
+    (scheme-file "config.scm"
+                 #~(begin
+                     (define-module (guix config)
+                       #:export (%libz))
+
+                     (define %libz
+                       #+(file-append zlib "/lib/libz")))))
+
+  (define imported-modules
+    (cons `((guix config) => ,config.scm)
+          (delete '(guix config)
+                  (source-module-closure '((gnu build linux-modules)
+                                           (guix build utils))))))
+
   (define build-exp
-    (with-imported-modules (source-module-closure
-                            '((gnu build linux-modules)))
+    (with-imported-modules imported-modules
       #~(begin
           (use-modules (gnu build linux-modules)
+                       (guix build utils)
                        (srfi srfi-1)
                        (srfi srfi-26))
 
@@ -129,12 +147,20 @@ MODULES and taken from LINUX."
                       (recursive-module-dependencies modules
                                                      #:lookup-module lookup))))
 
+          (define (maybe-uncompress file)
+            ;; If FILE is a compressed module, uncompress it, as the initrd is
+            ;; already gzipped as a whole.
+            (cond
+             ((string-contains file ".ko.gz")
+              (invoke #+(file-append gzip "/bin/gunzip") file))))
+
           (mkdir #$output)
           (for-each (lambda (module)
-                      (format #t "copying '~a'...~%" module)
-                      (copy-file module
-                                 (string-append #$output "/"
-                                                (basename module))))
+                      (let ((out-module
+                             (string-append #$output "/" (basename module))))
+                        (format #t "copying '~a'...~%" module)
+                        (copy-file module out-module)
+                        (maybe-uncompress out-module)))
                     (delete-duplicates modules))
 
           ;; Hyphen or underscore?  This database tells us.
-- 
2.26.2





^ permalink raw reply	[flat|nested] 8+ messages in thread

* [bug#42123] [PATCH] linux-libre: Enable module compression.
  2020-06-29 14:24 [bug#42123] [PATCH] linux-libre: Enable module compression Mathieu Othacehe
@ 2020-06-30  7:31 ` Mathieu Othacehe
  2020-07-02 10:23 ` Ludovic Courtès
  1 sibling, 0 replies; 8+ messages in thread
From: Mathieu Othacehe @ 2020-06-30  7:31 UTC (permalink / raw)
  To: 42123


Hello,

There's at least one issue with this one. When running the
"installation-os" test, I have this backtrace:

--8<---------------cut here---------------start------------->8---
+ guix system init /mnt/etc/config.scm /mnt --no-substitutes
The following derivations will be built:
   /gnu/store/rlq93i0ahllyvmjxk14w3snmqsv7bsvg-system.drv
   /gnu/store/28cfjshgch8jyixmwv3q0x1ybniwlvf5-parameters.drv
   /gnu/store/vk3rw7qwm7p0ldy34cadzgczxx2hc5xf-raw-initrd.drv
   /gnu/store/w219h1ia74bkplvr622f0b8qxyi9xw7y-provenance.drv
   /gnu/store/z56pp4bw5hif9mh856cckqvfnvykaybh-profile.drv
   /gnu/store/xalv7fm73cpyyi3zyvy1zlb26nfi1fb6-module-import-compiled.drv
   /gnu/store/rmpwh65ypvv1805zwmcir8gk9b53c4gz-module-import-compiled.drv
   /gnu/store/l9ixrby1zjqvdc719lyvfp9n9n4hb5pv-grub.cfg.drv
The following profile hook will be built:
   /gnu/store/92giwj2f8jyq90l20q19vcvds18v9xf4-linux-module-database.drv
building /gnu/store/w219h1ia74bkplvr622f0b8qxyi9xw7y-provenance.drv...
building /gnu/store/k4m2pj7sadz4vv5aaz40f68gaa4mw36b-Python-3.5.9.tar.xz.drv...
\builder for `/gnu/store/k4m2pj7sadz4vv5aaz40f68gaa4mw36b-Python-3.5.9.tar.xz.drv' failed to produce output path `/gnu/store/f99fblkzb6ip268sg096shhs7wzjyp55-Python-3.5.9.tar.xz'
build of /gnu/store/k4m2pj7sadz4vv5aaz40f68gaa4mw36b-Python-3.5.9.tar.xz.drv failed
View build log at '/var/log/guix/drvs/k4/m2pj7sadz4vv5aaz40f68gaa4mw36b-Python-3.5.9.tar.xz.drv.bz2'.
cannot build derivation `/gnu/store/9l09n8d6ick1nsjvchysys3hdgq7ynfr-Python-3.5.9.tar.xz.drv': 1 dependencies couldn't be built
building /gnu/store/wd9giqyfcfm1wgc6rscl3m9i30hg6rcs-bash-2.05b.tar.gz.drv...
cannot build derivation `/gnu/store/66q0r5cr3j1cbwckx5zvi0wv4cp3kxgl-python-minimal-3.5.9.drv': 1 dependencies couldn't be built
cannot build derivation `/gnu/store/c2ly303cbslks8hx3811wa91wahqr295-glibc-2.31.drv': 1 dependencies couldn't be built
building /gnu/store/h1p3962y3bmv1zgwsng1gb4c7caryj82-config.scm.drv...
cannot build derivation `/gnu/store/97vna9jgn19hyfx24s2kd6c3wywg22wl-e2fsprogs-1.45.6.drv': 1 dependencies couldn't be built
cannot build derivation `/gnu/store/r4dg2iypidr067kyddg90z07arrxp3h6-e2fsck-static-1.45.6.drv': 1 dependencies couldn't be built
cannot build derivation `/gnu/store/64f1y8njbd8bppl61fm6dhljnlgmh3h2-init.drv': 1 dependencies couldn't be built
building /gnu/store/6v5f1ry0pbhm2a1v5v8b773qpncnf6rr-module-import-compiled.drv...
cannot build derivation `/gnu/store/vk3rw7qwm7p0ldy34cadzgczxx2hc5xf-raw-initrd.drv': 1 dependencies couldn't be built
cannot build derivation `/gnu/store/l9ixrby1zjqvdc719lyvfp9n9n4hb5pv-grub.cfg.drv': 1 dependencies couldn't be built
guix system: error: build of `/gnu/store/l9ixrby1zjqvdc719lyvfp9n9n4hb5pv-grub.cfg.drv' failed
environment variable `PATH' set to `/gnu/store/a2b10x6h8j8qgsrcqip06xhnbssa0k25-qemu-minimal-5.0.0/bin'
--8<---------------cut here---------------end--------------->8---

For some reason, something triggers a build of the initrd (instead of using
the gcrooted one). I guess it's related to the addition of "zlib" and
"gzip" to the initrd inputs, but I don't understand why.

Someone?

Thanks,

Mathieu




^ permalink raw reply	[flat|nested] 8+ messages in thread

* [bug#42123] [PATCH] linux-libre: Enable module compression.
  2020-06-29 14:24 [bug#42123] [PATCH] linux-libre: Enable module compression Mathieu Othacehe
  2020-06-30  7:31 ` Mathieu Othacehe
@ 2020-07-02 10:23 ` Ludovic Courtès
  2020-07-06  8:48   ` Mathieu Othacehe
  1 sibling, 1 reply; 8+ messages in thread
From: Ludovic Courtès @ 2020-07-02 10:23 UTC (permalink / raw)
  To: Mathieu Othacehe; +Cc: Mathieu Othacehe, 42123

Hi!

Mathieu Othacehe <m.othacehe@gmail.com> skribis:

> This commit enables GZIP compression for linux-libre kernel modules, reducing
> the size of linux-libre by 63% (165MB). The initrd modules are kept
> uncompressed as the initrd is already compressed as a whole.
>
> The linux-libre kernel also supports XZ compression, but as Guix does not have
> any available bindings for now, and the compression time is far more
> significant, GZIP seems to be a better option.
>
> * gnu/packages/aux-files/linux-libre/5.4-arm.conf: Enable GZ compression.
> * gnu/packages/aux-files/linux-libre/5.4-arm64.conf: Ditto.
> * gnu/packages/aux-files/linux-libre/5.4-i686.conf: Ditto.
> * gnu/packages/aux-files/linux-libre/5.4-x86_64.conf: Ditto.
> * gnu/build/linux-modules.scm (modinfo-section-contents): Use
> 'call-with-gzip-input-port' to read from a module file using '.gz' extension,
> (strip-extension): new procedure,
> (dot-ko): adapt to support compression,
> (ensure-dot-ko): ditto,
> (file-name->module-name): ditto,
> (find-module-file): ditto,
> (load-linux-module*): ditto,
> (module-name->file-name/guess): ditto,
> (module-name-lookup): ditto,
> (write-module-name-database): ditto,
> (write-module-alias-database): ditto,
> (write-module-device-database): ditto.
> * gnu/system/linux-initrd.scm (flat-linux-module-directory): Make sure that
> zlib bindings are available because they may be used in
> 'write-module-device-database'. Also make sure that the initrd only contains
> uncompressed module files.

Nice!

I do think that gzip is more appropriate than xz here, also in terms of
memory requirements.

Perhaps you can do this in two patches: first the linux-initrd bits, and
2nd the linux-libre changes.

> diff --git a/gnu/build/linux-modules.scm b/gnu/build/linux-modules.scm
> index aa1c7cfeae..7c6945a881 100644
> --- a/gnu/build/linux-modules.scm
> +++ b/gnu/build/linux-modules.scm
> @@ -21,6 +21,7 @@
>  (define-module (gnu build linux-modules)
>    #:use-module (guix elf)
>    #:use-module (guix glob)
> +  #:use-module (guix zlib)
>    #:use-module (guix build syscalls)
>    #:use-module ((guix build utils) #:select (find-files invoke))
>    #:use-module (guix build union)
> @@ -97,7 +98,16 @@ string list."
>  (define (modinfo-section-contents file)
>    "Return the contents of the '.modinfo' section of FILE as a list of
>  key/value pairs.."
> -  (let* ((bv      (call-with-input-file file get-bytevector-all))
> +  (define (get-bytevector file)
> +    (cond
> +     ((string-contains file ".ko.gz")

‘string-suffix?’ would be more accurate.

> +      (call-with-input-file file
> +        (lambda (port)
> +          (call-with-gzip-input-port port get-bytevector-all))))

‘call-with-input-file’ creates a buffered port, which could be
problematic, although ‘make-gzip-input-port’ checks that.

To be safe, you’d do (open-file file "r0") with a dynwind.

> +(define* (module-name->file-name/guess directory name
> +                                       #:key compression)
>    "Guess the file name corresponding to NAME, a module name.  That doesn't
>  always work because sometimes underscores in NAME map to hyphens (e.g.,
>  \"input-leds.ko\"), sometimes not (e.g., \"mac_hid.ko\")."

Please mention COMPRESSION in the docstring.

>  (define (write-module-alias-database directory)
> -  "Traverse the '.ko' files in DIRECTORY and create the corresponding
> +  "Traverse the '.ko[.gz|.xz]' files in DIRECTORY and create the corresponding
>  'modules.alias' file."
>    (define aliases
>      (map (lambda (file)
>             (cons (file-name->module-name file) (module-aliases file)))
> -         (find-files directory "\\.ko$")))
> +         (find-files directory "\\.ko.*$")))

Should we refine this regexp (there are a couple of places like this)?

There other Scheme bits LGTM!

(I don’t really know about Linux-libre but you do!)

Ludo’.




^ permalink raw reply	[flat|nested] 8+ messages in thread

* [bug#42123] [PATCH] linux-libre: Enable module compression.
  2020-07-02 10:23 ` Ludovic Courtès
@ 2020-07-06  8:48   ` Mathieu Othacehe
  2020-07-06 12:20     ` Ludovic Courtès
  0 siblings, 1 reply; 8+ messages in thread
From: Mathieu Othacehe @ 2020-07-06  8:48 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 42123


Hey Ludo,

> There other Scheme bits LGTM!
>
> (I don’t really know about Linux-libre but you do!)

Thanks a lot for reviewing :) I took all your remarks into
account. Regarding the test issue I'm describing in this thread, I now
have an understanding of the situation.

Adding (guix zlib) as a dependency of (gnu build linux-modules) also
drags (guix config). That's an issue because there are a lot of
derivations including (gnu build linux-modules) without deleting/mocking
(guix config).

That's for instance the case with "expression->initrd", and it explains
why the installation test is trying to rebuild the initrd.

I see two solutions here:

* Delete/mock (guix config) in every derivation including (gnu build
linux-modules) or any other build module that includes it. Not ideal
because there are quite numerous.

* Do not use (guix zlib) but a raw "gzip -cd" pipe in (gnu build
linux-modules). It works but it way slower.
  
A third solution would be nice here :)

WDYT?

Thanks,

Mathieu




^ permalink raw reply	[flat|nested] 8+ messages in thread

* [bug#42123] [PATCH] linux-libre: Enable module compression.
  2020-07-06  8:48   ` Mathieu Othacehe
@ 2020-07-06 12:20     ` Ludovic Courtès
  2020-07-06 14:23       ` Mathieu Othacehe
  0 siblings, 1 reply; 8+ messages in thread
From: Ludovic Courtès @ 2020-07-06 12:20 UTC (permalink / raw)
  To: Mathieu Othacehe; +Cc: 42123

Hello!

Mathieu Othacehe <othacehe@gnu.org> skribis:

> Adding (guix zlib) as a dependency of (gnu build linux-modules) also
> drags (guix config). That's an issue because there are a lot of
> derivations including (gnu build linux-modules) without deleting/mocking
> (guix config).
>
> That's for instance the case with "expression->initrd", and it explains
> why the installation test is trying to rebuild the initrd.
>
> I see two solutions here:
>
> * Delete/mock (guix config) in every derivation including (gnu build
> linux-modules) or any other build module that includes it. Not ideal
> because there are quite numerous.
>
> * Do not use (guix zlib) but a raw "gzip -cd" pipe in (gnu build
> linux-modules). It works but it way slower.

I don’t have other ideas, but both solutions sound good to me.  Using
(guix zlib) is slightly more “elegant” IMO, but no big deal.  I don’t
expect any significant difference from the use of in-process
decompression, unless we really have to go and decompress many modules
in a row.

Thanks,
Ludo’.




^ permalink raw reply	[flat|nested] 8+ messages in thread

* [bug#42123] [PATCH] linux-libre: Enable module compression.
  2020-07-06 12:20     ` Ludovic Courtès
@ 2020-07-06 14:23       ` Mathieu Othacehe
  2020-07-06 20:13         ` Ludovic Courtès
  0 siblings, 1 reply; 8+ messages in thread
From: Mathieu Othacehe @ 2020-07-06 14:23 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 42123

[-- Attachment #1: Type: text/plain, Size: 648 bytes --]


Hey,

> I don’t have other ideas, but both solutions sound good to me.  Using
> (guix zlib) is slightly more “elegant” IMO, but no big deal.  I don’t
> expect any significant difference from the use of in-process
> decompression, unless we really have to go and decompress many modules
> in a row.

Creating the initrd implies to create the module name database, and it
ends-up decompressing every single module. Using the in-process method
it takes 2 seconds, using the second method 30 seconds.

So, I opted for the first solution as you suggested. Here's an attached
patch that fixes the situation.

Thanks,

Mathieu

[-- Attachment #2: import.patch --]
[-- Type: text/x-diff, Size: 10330 bytes --]

From 8bbf343510091fad4a08758e0115a70410c1c8d7 Mon Sep 17 00:00:00 2001
From: Mathieu Othacehe <othacehe@gnu.org>
Date: Mon, 6 Jul 2020 16:04:21 +0200
Subject: [PATCH] self: Add with-imported-modules+config and use it.

Introduce "with-imported-modules+config" and use it to replace every call to
"with-imported-modules" that would trigger an import of (guix config) module.

* guix/self.scm (not-config?): New procedure,
(with-imported-modules+config): new macro.
* guix/profiles.scm (linux-module-database): Replace with-imported-modules by
with-imported-modules+config.
* gnu/system/shadow.scm (account-shepherd-service): Ditto.
* gnu/system/linux-initrd.scm (raw-initrd): Ditto.
* gnu/services/base.scm (default-serial-port): Ditto,
(file-system-shepherd-service): ditto,
(udev-shepherd-service): ditto.
* gnu/services.scm (activation-script): Ditto.
* gnu/machine/ssh.scm (machine-check-initrd-modules): Ditto.
---
 gnu/machine/ssh.scm         |  8 ++++----
 gnu/services.scm            |  6 +++---
 gnu/services/base.scm       | 11 +++++------
 gnu/system/linux-initrd.scm | 12 ++++++------
 gnu/system/shadow.scm       |  6 +++---
 guix/profiles.scm           |  6 +++---
 guix/self.scm               | 24 ++++++++++++++++++++++++
 7 files changed, 48 insertions(+), 25 deletions(-)

diff --git a/gnu/machine/ssh.scm b/gnu/machine/ssh.scm
index 4148639292..7369eb2136 100644
--- a/gnu/machine/ssh.scm
+++ b/gnu/machine/ssh.scm
@@ -33,6 +33,7 @@
   #:use-module (guix records)
   #:use-module (guix remote)
   #:use-module (guix scripts system reconfigure)
+  #:use-module (guix self)
   #:use-module (guix ssh)
   #:use-module (guix store)
   #:use-module (guix utils)
@@ -246,10 +247,9 @@ not available in the initrd."
   (define (missing-modules fs)
     (define remote-exp
       (let ((device (file-system-device fs)))
-        (with-imported-modules (source-module-closure
-                                '((gnu build file-systems)
-                                  (gnu build linux-modules)
-                                  (gnu system uuid)))
+        (with-imported-modules+config '((gnu build file-systems)
+                                        (gnu build linux-modules)
+                                        (gnu system uuid))
           #~(begin
               (use-modules (gnu build file-systems)
                            (gnu build linux-modules)
diff --git a/gnu/services.scm b/gnu/services.scm
index f6dc56d940..4d7371cd78 100644
--- a/gnu/services.scm
+++ b/gnu/services.scm
@@ -28,6 +28,7 @@
   #:use-module (guix combinators)
   #:use-module (guix channels)
   #:use-module (guix describe)
+  #:use-module (guix self)
   #:use-module (guix sets)
   #:use-module (guix ui)
   #:use-module ((guix utils) #:select (source-properties->location))
@@ -542,9 +543,8 @@ ACTIVATION-SCRIPT-TYPE."
     (map (cut program-file "activate-service.scm" <>) gexps))
 
   (program-file "activate.scm"
-                (with-imported-modules (source-module-closure
-                                        '((gnu build activation)
-                                          (guix build utils)))
+                (with-imported-modules+config '((gnu build activation)
+                                                (guix build utils))
                   #~(begin
                       (use-modules (gnu build activation)
                                    (guix build utils))
diff --git a/gnu/services/base.scm b/gnu/services/base.scm
index 6ea7ef8e7e..94dfeb2315 100644
--- a/gnu/services/base.scm
+++ b/gnu/services/base.scm
@@ -30,6 +30,7 @@
 ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
 
 (define-module (gnu services base)
+  #:use-module (guix self)
   #:use-module (guix store)
   #:use-module (guix deprecation)
   #:use-module (gnu services)
@@ -832,8 +833,8 @@ the message of the day, among other things."
 (define (default-serial-port)
   "Return a gexp that determines a reasonable default serial port
 to use as the tty.  This is primarily useful for headless systems."
-  (with-imported-modules (source-module-closure
-                          '((gnu build linux-boot))) ;for 'find-long-options'
+  (with-imported-modules+config
+      '((gnu build linux-boot)) ;for 'find-long-options'
     #~(begin
         ;; console=device,options
         ;; device: can be tty0, ttyS0, lp0, ttyUSB0 (serial).
@@ -886,8 +887,7 @@ to use as the tty.  This is primarily useful for headless systems."
 
          (modules '((ice-9 match) (gnu build linux-boot)))
          (start
-          (with-imported-modules  (source-module-closure
-                                   '((gnu build linux-boot)))
+          (with-imported-modules+config '((gnu build linux-boot))
             #~(lambda args
                 (let ((defaulted-tty #$(or tty (default-serial-port))))
                   (apply
@@ -1935,8 +1935,7 @@ item of @var{packages}."
 
          (documentation "Populate the /dev directory, dynamically.")
          (start
-          (with-imported-modules (source-module-closure
-                                  '((gnu build linux-boot)))
+          (with-imported-modules+config '((gnu build linux-boot))
             #~(lambda ()
                 (define udevd
                   ;; 'udevd' from eudev.
diff --git a/gnu/system/linux-initrd.scm b/gnu/system/linux-initrd.scm
index 99ec82246b..8779ef58d7 100644
--- a/gnu/system/linux-initrd.scm
+++ b/gnu/system/linux-initrd.scm
@@ -28,6 +28,7 @@
   #:use-module ((guix derivations)
                 #:select (derivation->output-path))
   #:use-module (guix modules)
+  #:use-module (guix self)
   #:use-module (gnu packages compression)
   #:use-module (gnu packages disk)
   #:use-module (gnu packages linux)
@@ -214,12 +215,11 @@ upon error."
     (flat-linux-module-directory linux linux-modules))
 
   (expression->initrd
-   (with-imported-modules (source-module-closure
-                           '((gnu build linux-boot)
-                             (guix build utils)
-                             (guix build bournish)
-                             (gnu system file-systems)
-                             (gnu build file-systems)))
+   (with-imported-modules+config '((gnu build linux-boot)
+                                   (guix build utils)
+                                   (guix build bournish)
+                                   (gnu system file-systems)
+                                   (gnu build file-systems))
      #~(begin
          (use-modules (gnu build linux-boot)
                       (gnu system file-systems)
diff --git a/gnu/system/shadow.scm b/gnu/system/shadow.scm
index a69339bc07..e140f06913 100644
--- a/gnu/system/shadow.scm
+++ b/gnu/system/shadow.scm
@@ -22,6 +22,7 @@
 (define-module (gnu system shadow)
   #:use-module (guix records)
   #:use-module (guix gexp)
+  #:use-module (guix self)
   #:use-module (guix store)
   #:use-module (guix modules)
   #:use-module (guix sets)
@@ -321,9 +322,8 @@ accounts among ACCOUNTS+GROUPS."
          (one-shot? #t)
          (modules '((gnu build activation)
                     (gnu system accounts)))
-         (start (with-imported-modules (source-module-closure
-                                        '((gnu build activation)
-                                          (gnu system accounts)))
+         (start (with-imported-modules+config '((gnu build activation)
+                                                (gnu system accounts))
                   #~(lambda ()
                       (activate-user-home
                        (map sexp->user-account
diff --git a/guix/profiles.scm b/guix/profiles.scm
index f34f73e17e..f11e400dd3 100644
--- a/guix/profiles.scm
+++ b/guix/profiles.scm
@@ -40,6 +40,7 @@
   #:use-module (guix gexp)
   #:use-module (guix modules)
   #:use-module (guix monads)
+  #:use-module (guix self)
   #:use-module (guix store)
   #:use-module (ice-9 vlist)
   #:use-module (ice-9 match)
@@ -1205,9 +1206,8 @@ This is meant to be used as a profile hook."
   (define kmod                                    ; lazy reference
     (module-ref (resolve-interface '(gnu packages linux)) 'kmod))
   (define build
-    (with-imported-modules (source-module-closure
-                            '((guix build utils)
-                              (gnu build linux-modules)))
+    (with-imported-modules+config '((guix build utils)
+                                    (gnu build linux-modules))
       #~(begin
           (use-modules (ice-9 ftw)
                        (ice-9 match)
diff --git a/guix/self.scm b/guix/self.scm
index e1350a7403..82bb55f8e7 100644
--- a/guix/self.scm
+++ b/guix/self.scm
@@ -33,6 +33,8 @@
   #:use-module (srfi srfi-35)
   #:use-module (ice-9 match)
   #:export (make-config.scm
+            not-config?
+            with-imported-modules+config
             whole-package                     ;for internal use in 'guix pull'
             compiled-guix
             guix-derivation))
@@ -1063,6 +1065,24 @@ Info manual."
                ;; made relative to a nonexistent anonymous module.
                #:splice? #t))
 
+(define not-config?
+  ;; Select (guix …) and (gnu …) modules, except (guix config).
+  (match-lambda
+    (('guix 'config) #f)
+    (('guix rest ...) #t)
+    (('gnu rest ...) #t)
+    (rest #f)))
+
+(define-syntax-rule (with-imported-modules+config modules exp ...)
+  "Import the closure of MODULES and evaluate EXP within this context.  If the
+(guix config) module is part of the closure, it is not selected.  This module
+is always replaced by a mocked-one, created by MAKE-CONFIG.SCM pocedure."
+  (with-imported-modules `(,@(source-module-closure
+                              modules
+                              #:select? not-config?)
+                           ((guix config) => ,(make-config.scm)))
+    exp ...))
+
 \f
 ;;;
 ;;; Building.
@@ -1213,3 +1233,7 @@ is not supported."
       (if guix
           (lower-object guix)
           (return #f)))))
+
+;; Local Variables:
+;; eval: (put 'with-imported-modules+config 'scheme-indent-function 2)
+;; End:
-- 
2.24.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [bug#42123] [PATCH] linux-libre: Enable module compression.
  2020-07-06 14:23       ` Mathieu Othacehe
@ 2020-07-06 20:13         ` Ludovic Courtès
  2020-07-07  7:32           ` Mathieu Othacehe
  0 siblings, 1 reply; 8+ messages in thread
From: Ludovic Courtès @ 2020-07-06 20:13 UTC (permalink / raw)
  To: Mathieu Othacehe; +Cc: 42123

Hi,

Mathieu Othacehe <othacehe@gnu.org> skribis:

>> I don’t have other ideas, but both solutions sound good to me.  Using
>> (guix zlib) is slightly more “elegant” IMO, but no big deal.  I don’t
>> expect any significant difference from the use of in-process
>> decompression, unless we really have to go and decompress many modules
>> in a row.
>
> Creating the initrd implies to create the module name database, and it
> ends-up decompressing every single module. Using the in-process method
> it takes 2 seconds, using the second method 30 seconds.

Woow, interesting.

> So, I opted for the first solution as you suggested. Here's an attached
> patch that fixes the situation.

[...]

> From 8bbf343510091fad4a08758e0115a70410c1c8d7 Mon Sep 17 00:00:00 2001
> From: Mathieu Othacehe <othacehe@gnu.org>
> Date: Mon, 6 Jul 2020 16:04:21 +0200
> Subject: [PATCH] self: Add with-imported-modules+config and use it.
>
> Introduce "with-imported-modules+config" and use it to replace every call to
> "with-imported-modules" that would trigger an import of (guix config) module.
>
> * guix/self.scm (not-config?): New procedure,
> (with-imported-modules+config): new macro.
> * guix/profiles.scm (linux-module-database): Replace with-imported-modules by
> with-imported-modules+config.
> * gnu/system/shadow.scm (account-shepherd-service): Ditto.
> * gnu/system/linux-initrd.scm (raw-initrd): Ditto.
> * gnu/services/base.scm (default-serial-port): Ditto,
> (file-system-shepherd-service): ditto,
> (udev-shepherd-service): ditto.
> * gnu/services.scm (activation-script): Ditto.
> * gnu/machine/ssh.scm (machine-check-initrd-modules): Ditto.

[...]

> diff --git a/guix/self.scm b/guix/self.scm
> index e1350a7403..82bb55f8e7 100644
> --- a/guix/self.scm
> +++ b/guix/self.scm
> @@ -33,6 +33,8 @@
>    #:use-module (srfi srfi-35)
>    #:use-module (ice-9 match)
>    #:export (make-config.scm
> +            not-config?
> +            with-imported-modules+config
>              whole-package                     ;for internal use in 'guix pull'
>              compiled-guix
>              guix-derivation))
> @@ -1063,6 +1065,24 @@ Info manual."
>                 ;; made relative to a nonexistent anonymous module.
>                 #:splice? #t))
>  
> +(define not-config?
> +  ;; Select (guix …) and (gnu …) modules, except (guix config).
> +  (match-lambda
> +    (('guix 'config) #f)
> +    (('guix rest ...) #t)
> +    (('gnu rest ...) #t)
> +    (rest #f)))
> +
> +(define-syntax-rule (with-imported-modules+config modules exp ...)
> +  "Import the closure of MODULES and evaluate EXP within this context.  If the
> +(guix config) module is part of the closure, it is not selected.  This module
> +is always replaced by a mocked-one, created by MAKE-CONFIG.SCM pocedure."
> +  (with-imported-modules `(,@(source-module-closure
> +                              modules
> +                              #:select? not-config?)
> +                           ((guix config) => ,(make-config.scm)))
> +    exp ...))

Two remarks: I feel that this is not the best place for it, and I think
we should add (guix config) if and only if it’s actually part of the
closure.

For the name I’m tempted by a simpler but less descriptive option.

That would give us:

  (define-syntax-rule (with-imported-modules* modules exp ...)
    (let ((closure (source-module-closure modules #:select? not-config?)))
      (with-imported-modules (map (match-lambda
                                    (('guix 'config) …)
                                    (module module))
                                  closure)
        exp ...)))

WDYT?

As for the location… I think the reason things are like this is to avoid
having everything depend on (guix self), but maybe that’s OK after all?

If the zlib bindings were an external package, things would be easier
because we wouldn’t have to do this (guix config) dance.

More generally, we should look at all the uses of (guix config) and see
whether/how we can get rid of them.

But I digress… Thoughts?

Thank you,
Ludo’.




^ permalink raw reply	[flat|nested] 8+ messages in thread

* [bug#42123] [PATCH] linux-libre: Enable module compression.
  2020-07-06 20:13         ` Ludovic Courtès
@ 2020-07-07  7:32           ` Mathieu Othacehe
  0 siblings, 0 replies; 8+ messages in thread
From: Mathieu Othacehe @ 2020-07-07  7:32 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 42123


Hey Ludo!

> As for the location… I think the reason things are like this is to avoid
> having everything depend on (guix self), but maybe that’s OK after all?
>
> If the zlib bindings were an external package, things would be easier
> because we wouldn’t have to do this (guix config) dance.
>
> More generally, we should look at all the uses of (guix config) and see
> whether/how we can get rid of them.
>
> But I digress… Thoughts?

Thanks for your quick feedback. Given the time I spent debugging this
issue, I agree that this (guix config) module thing is hard to deal
with.

If it works for you, I could create three new Guile libraries:

* Guile-libz
* Guile-liblz
* Guile-xz

or maybe just,

* Guile-compression

It would help us getting rid of (guix config) as you proposed.

WDYT?

Thanks,

Mathieu




^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-29 14:24 [bug#42123] [PATCH] linux-libre: Enable module compression Mathieu Othacehe
2020-06-30  7:31 ` Mathieu Othacehe
2020-07-02 10:23 ` Ludovic Courtès
2020-07-06  8:48   ` Mathieu Othacehe
2020-07-06 12:20     ` Ludovic Courtès
2020-07-06 14:23       ` Mathieu Othacehe
2020-07-06 20:13         ` Ludovic Courtès
2020-07-07  7:32           ` Mathieu Othacehe

unofficial mirror of guix-patches@gnu.org 

Archives are clonable:
	git clone --mirror https://yhetil.org/guix-patches/1 guix-patches/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 guix-patches guix-patches/ https://yhetil.org/guix-patches \
		guix-patches@gnu.org
	public-inbox-index guix-patches

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://news.yhetil.org/yhetil.gnu.guix.patches


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git