bug#54783: ZRAM default priority wrong

bug#54783: ZRAM default priority wrong

[Stefan Baums]

I have a swap file and zram configured without specifying
priority. The swap file was assigned -2, and zram -3, with the
effect that zram was never used. I had to manually change the zram
priority to something higher that that of the swap file.

The manual wrongly claims that the default priority for zram is
-1. The same paragraph of the manual also says:

   ‘swapon’ accepts values between -1 and 32767, with higher
   values indicating higher priority.

which makes the -2 priority of the swap file look suspect (though
it seems to work).

bug#54783: ZRAM default priority wrong

[Josselin Poiret via Bug reports for GNU Guix]

Hello Stefan and sorry for the late reply,

Stefan Baums <baums@stefanbaums.com> writes:

> The manual wrongly claims that the default priority for zram is
> -1. The same paragraph of the manual also says:
>
>    ‘swapon’ accepts values between -1 and 32767, with higher
>    values indicating higher priority.
>
> which makes the -2 priority of the swap file look suspect (though
> it seems to work).

This issue comes from the inconsistency with which swap priorities are
specified on Linux: `swapon` the binary from util-linux (man 8 swapon)
has roughly the same description as above, but internally uses the
syscall wrapper `swapon` from libc (man 2 swapon), which has a better
description of priorities.  The official interpretation of priorities
for the syscall is summarized in the "Swap space" part of the Guix
manual:

A swap space can have no priority, or a priority specified as an integer
between 0 and 32767.  The kernel will first use swap spaces of higher
priority when paging, and use same priority spaces on a round-robin
basis.  The kernel will use swap spaces without a set priority after
prioritized spaces, and in the order that they appeared in (not
round-robin).

Note that these are called high (for specified priority) or low
(unspecified) priority in `man 2 swapon` but that's a bit of a
misnomer.  So, `swapon` the binary actually maps -1 to no (low)
priority, and there actually is no -1 priority in the Linux kernel ABI!
Since your actual swap file is swapon'd first, and also has no priority
set, according to the rules above it will be used before the ZRAM which
also has no priority.

On the Guix side of things, we should really be using the same interface
as swap-space, as I think the distinction between #f and 0 to 32767 is
clearer.  I'll send some patches that adress this soon, along with the
zram-service-type documentation.

Best,
-- 
Josselin Poiret

bug#54783: ZRAM default priority wrong

[Stefan Baums]

Dear Josselin,

thank you very much! That should clarify things. I have been
happily using ZRAM on Guix since I figured it out.

All best,
Stefan

bug#54783: [PATCH 1/2] system: Align zram priority with swap-space spec to clarify.

[Josselin Poiret via Bug reports for GNU Guix]

* gnu/services/linux.scm
(zram-device-configuration) [priority]: Adapt to use #f or an integer
from 0 to 32767.  Add sanitizer to warn for the change and delay the
field.
(zram-device-configuration->udev-string): Adapt as above.
* doc/guix.texi (Zram Device Service): Change priority description to
refer to the Swap Space one, and suggest not leaving the default #f on
to properly use zram.
---
 doc/guix.texi          | 10 +++++-----
 gnu/services/linux.scm | 26 +++++++++++++++++++++++---
 2 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index b7005f0ef1..31f391357d 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -96,7 +96,7 @@ Copyright @copyright{} 2021 Domagoj Stolfa@*
 Copyright @copyright{} 2021 Hui Lu@*
 Copyright @copyright{} 2021 pukkamustard@*
 Copyright @copyright{} 2021 Alice Brenon@*
-Copyright @copyright{} 2021 Josselin Poiret@*
+Copyright @copyright{} 2021, 2022 Josselin Poiret@*
 Copyright @copyright{} 2021 Andrew Tropin@*
 Copyright @copyright{} 2021 Sarah Morgensen@*
 Copyright @copyright{} 2021 Josselin Poiret@*
@@ -34650,11 +34650,11 @@ that compression will be 2:1, it is possible that uncompressable data
 can be written to swap and this is a method to limit how much memory can
 be used.  It accepts a string and can be a number of bytes or use a
 suffix, eg.: @code{"2G"}.
-@item @code{priority} (default @code{-1})
+@item @code{priority} (default @code{#f})
 This is the priority of the swap device created from the zram device.
-@code{swapon} accepts values between -1 and 32767, with higher values
-indicating higher priority.  Higher priority swap will generally be used
-first.
+@xref{Swap Space} for a description of swap priorities.  You might want
+to set a specific priority for the zram device, otherwise it could end
+up not being used much for the reasons described there.
 @end table
 
 @end deftp
diff --git a/gnu/services/linux.scm b/gnu/services/linux.scm
index 2eb02ac5a3..9f598b2826 100644
--- a/gnu/services/linux.scm
+++ b/gnu/services/linux.scm
@@ -4,6 +4,7 @@
 ;;; Copyright © 2020 Efraim Flashner <efraim@flashner.co.il>
 ;;; Copyright © 2021 raid5atemyhomework <raid5atemyhomework@protonmail.com>
 ;;; Copyright © 2021 B. Wilson <elaexuotee@wilsonb.com>
+;;; Copyright © 2022 Josselin Poiret <dev@jpoiret.xyz>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -21,9 +22,11 @@
 ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
 
 (define-module (gnu services linux)
+  #:use-module (guix diagnostics)
   #:use-module (guix gexp)
   #:use-module (guix records)
   #:use-module (guix modules)
+  #:use-module (guix i18n)
   #:use-module (gnu services)
   #:use-module (gnu services base)
   #:use-module (gnu services shepherd)
@@ -252,7 +255,19 @@ (define-record-type* <zram-device-configuration>
   (memory-limit             zram-device-configuration-memory-limit
                             (default 0))        ; string or integer
   (priority                 zram-device-configuration-priority
-                            (default -1)))      ; integer
+                            (default #f)        ; integer | #f
+                            (delayed)
+                            (sanitize warn-zram-priority-change)))
+
+(define-with-syntax-properties
+  (warn-zram-priority-change (priority properties))
+  (if (eqv? priority -1)
+      (begin
+        (warning (source-properties->location properties)
+                 (G_ "Using -1 for zram priority is deprecated to align with \
+the corresponding swap-space field, please use #f from now on.~%"))
+        #f)
+      priority))
 
 (define (zram-device-configuration->udev-string config)
   "Translate a <zram-device-configuration> into a string which can be
@@ -278,8 +293,13 @@ (define (zram-device-configuration->udev-string config)
          "")
        "RUN+=\"/run/current-system/profile/sbin/mkswap /dev/zram0\" "
        "RUN+=\"/run/current-system/profile/sbin/swapon "
-       (if (not (equal? -1 priority))
-         (string-append "--priority " (number->string priority) " ")
+       ;; XXX: The field is delayed while the deprecation warning remains in
+       ;; place, so we can't use match to fetch it (it would give a promise)
+       (if (zram-device-configuration-priority config)
+           (string-append "--priority "
+                          (number->string
+                           (zram-device-configuration-priority config))
+                          " ")
          "")
        "/dev/zram0\"\n"))))
 
-- 
2.34.0

bug#54783: ZRAM default priority wrong

[Maxim Cournoyer]

Hi,

Josselin Poiret <dev@jpoiret.xyz> writes:

> * gnu/services/linux.scm
> (zram-device-configuration) [priority]: Adapt to use #f or an integer
> from 0 to 32767.  Add sanitizer to warn for the change and delay the
> field.
> (zram-device-configuration->udev-string): Adapt as above.
> * doc/guix.texi (Zram Device Service): Change priority description to
> refer to the Swap Space one, and suggest not leaving the default #f on
> to properly use zram.
> ---
>  doc/guix.texi          | 10 +++++-----
>  gnu/services/linux.scm | 26 +++++++++++++++++++++++---
>  2 files changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index b7005f0ef1..31f391357d 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -96,7 +96,7 @@ Copyright @copyright{} 2021 Domagoj Stolfa@*
>  Copyright @copyright{} 2021 Hui Lu@*
>  Copyright @copyright{} 2021 pukkamustard@*
>  Copyright @copyright{} 2021 Alice Brenon@*
> -Copyright @copyright{} 2021 Josselin Poiret@*
> +Copyright @copyright{} 2021, 2022 Josselin Poiret@*
>  Copyright @copyright{} 2021 Andrew Tropin@*
>  Copyright @copyright{} 2021 Sarah Morgensen@*
>  Copyright @copyright{} 2021 Josselin Poiret@*
> @@ -34650,11 +34650,11 @@ that compression will be 2:1, it is possible that uncompressable data
>  can be written to swap and this is a method to limit how much memory can
>  be used.  It accepts a string and can be a number of bytes or use a
>  suffix, eg.: @code{"2G"}.
> -@item @code{priority} (default @code{-1})
> +@item @code{priority} (default @code{#f})
>  This is the priority of the swap device created from the zram device.
> -@code{swapon} accepts values between -1 and 32767, with higher values
> -indicating higher priority.  Higher priority swap will generally be used
> -first.
> +@xref{Swap Space} for a description of swap priorities.  You might want
> +to set a specific priority for the zram device, otherwise it could end
> +up not being used much for the reasons described there.
>  @end table
>  
>  @end deftp
> diff --git a/gnu/services/linux.scm b/gnu/services/linux.scm
> index 2eb02ac5a3..9f598b2826 100644
> --- a/gnu/services/linux.scm
> +++ b/gnu/services/linux.scm
> @@ -4,6 +4,7 @@
>  ;;; Copyright © 2020 Efraim Flashner <efraim@flashner.co.il>
>  ;;; Copyright © 2021 raid5atemyhomework <raid5atemyhomework@protonmail.com>
>  ;;; Copyright © 2021 B. Wilson <elaexuotee@wilsonb.com>
> +;;; Copyright © 2022 Josselin Poiret <dev@jpoiret.xyz>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -21,9 +22,11 @@
>  ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
>  
>  (define-module (gnu services linux)
> +  #:use-module (guix diagnostics)
>    #:use-module (guix gexp)
>    #:use-module (guix records)
>    #:use-module (guix modules)
> +  #:use-module (guix i18n)
>    #:use-module (gnu services)
>    #:use-module (gnu services base)
>    #:use-module (gnu services shepherd)
> @@ -252,7 +255,19 @@ (define-record-type* <zram-device-configuration>
>    (memory-limit             zram-device-configuration-memory-limit
>                              (default 0))        ; string or integer
>    (priority                 zram-device-configuration-priority
> -                            (default -1)))      ; integer
> +                            (default #f)        ; integer | #f
> +                            (delayed)

I'm curious, what does delaying the field buys us here?  Is it to avoid
printing the warning multiple times when the record is evaluated?

> +                            (sanitize warn-zram-priority-change)))
> +
> +(define-with-syntax-properties
> +  (warn-zram-priority-change (priority properties))
> +  (if (eqv? priority -1)
> +      (begin
> +        (warning (source-properties->location properties)
> +                 (G_ "Using -1 for zram priority is deprecated to align with \
> +the corresponding swap-space field, please use #f from now on.~%"))
> +        #f)
> +      priority))

By convention, a warning message should not be a complete sentence (no
capitalized first letter nor last period) and be short.  To provide a
human friendly hint/message, you could use 'display-hint' (combined with
a more succinct warning).

The rest LGTM.

Maxim

bug#54783: [v2 0/2] Clarify zram priority

[Josselin Poiret via Bug reports for GNU Guix]

Hello Maxim,

Thanks for looking at this.  Here's a v2.

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
> I'm curious, what does delaying the field buys us here?  Is it to avoid
> printing the warning multiple times when the record is evaluated?

Right, and this would get worse if someone ended up inheriting from
another configuration.  It would be bad UX IMO. 

> By convention, a warning message should not be a complete sentence (no
> capitalized first letter nor last period) and be short.  To provide a
> human friendly hint/message, you could use 'display-hint' (combined with
> a more succinct warning).

Right, fixed!

> The rest LGTM.
>
> Maxim

Josselin Poiret (2):
  system: Align zram priority with swap-space spec to clarify.
  doc: Remove double copyright.

 doc/guix.texi          | 11 +++++------
 gnu/services/linux.scm | 29 ++++++++++++++++++++++++++---
 2 files changed, 31 insertions(+), 9 deletions(-)

-- 
2.36.0

bug#54783: [PATCH v2 1/2] system: Align zram priority with swap-space spec to clarify.

[Josselin Poiret via Bug reports for GNU Guix]

* gnu/services/linux.scm
(zram-device-configuration) [priority]: Adapt to use #f or an integer
from 0 to 32767.  Add sanitizer to warn for the change and delay the
field.
(zram-device-configuration->udev-string): Adapt as above.
* doc/guix.texi (Zram Device Service): Change priority description to
refer to the Swap Space one, and suggest not leaving the default #f on
to properly use zram.
---
 doc/guix.texi          | 10 +++++-----
 gnu/services/linux.scm | 29 ++++++++++++++++++++++++++---
 2 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 184206bec8..5f0120a3bd 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -96,7 +96,7 @@ Copyright @copyright{} 2021 Domagoj Stolfa@*
 Copyright @copyright{} 2021 Hui Lu@*
 Copyright @copyright{} 2021 pukkamustard@*
 Copyright @copyright{} 2021 Alice Brenon@*
-Copyright @copyright{} 2021 Josselin Poiret@*
+Copyright @copyright{} 2021, 2022 Josselin Poiret@*
 Copyright @copyright{} 2021 Andrew Tropin@*
 Copyright @copyright{} 2021 Sarah Morgensen@*
 Copyright @copyright{} 2021 Josselin Poiret@*
@@ -35213,11 +35213,11 @@ that compression will be 2:1, it is possible that uncompressable data
 can be written to swap and this is a method to limit how much memory can
 be used.  It accepts a string and can be a number of bytes or use a
 suffix, eg.: @code{"2G"}.
-@item @code{priority} (default @code{-1})
+@item @code{priority} (default @code{#f})
 This is the priority of the swap device created from the zram device.
-@code{swapon} accepts values between -1 and 32767, with higher values
-indicating higher priority.  Higher priority swap will generally be used
-first.
+@xref{Swap Space} for a description of swap priorities.  You might want
+to set a specific priority for the zram device, otherwise it could end
+up not being used much for the reasons described there.
 @end table
 
 @end deftp
diff --git a/gnu/services/linux.scm b/gnu/services/linux.scm
index 2eb02ac5a3..c6e460597a 100644
--- a/gnu/services/linux.scm
+++ b/gnu/services/linux.scm
@@ -4,6 +4,7 @@
 ;;; Copyright © 2020 Efraim Flashner <efraim@flashner.co.il>
 ;;; Copyright © 2021 raid5atemyhomework <raid5atemyhomework@protonmail.com>
 ;;; Copyright © 2021 B. Wilson <elaexuotee@wilsonb.com>
+;;; Copyright © 2022 Josselin Poiret <dev@jpoiret.xyz>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -21,9 +22,12 @@
 ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
 
 (define-module (gnu services linux)
+  #:use-module (guix diagnostics)
   #:use-module (guix gexp)
   #:use-module (guix records)
   #:use-module (guix modules)
+  #:use-module (guix i18n)
+  #:use-module (guix ui)
   #:use-module (gnu services)
   #:use-module (gnu services base)
   #:use-module (gnu services shepherd)
@@ -252,7 +256,21 @@ (define-record-type* <zram-device-configuration>
   (memory-limit             zram-device-configuration-memory-limit
                             (default 0))        ; string or integer
   (priority                 zram-device-configuration-priority
-                            (default -1)))      ; integer
+                            (default #f)        ; integer | #f
+                            (delayed) ; to avoid printing the deprecation
+                                      ; warning multiple times
+                            (sanitize warn-zram-priority-change)))
+
+(define-with-syntax-properties
+  (warn-zram-priority-change (priority properties))
+  (if (eqv? priority -1)
+      (begin
+        (warning (source-properties->location properties)
+                 (G_ "using -1 for zram priority is deprecated~%"))
+        (display-hint (G_ "Use #f or leave as default instead (@pxref{Linux \
+Services})."))
+        #f)
+      priority))
 
 (define (zram-device-configuration->udev-string config)
   "Translate a <zram-device-configuration> into a string which can be
@@ -278,8 +296,13 @@ (define (zram-device-configuration->udev-string config)
          "")
        "RUN+=\"/run/current-system/profile/sbin/mkswap /dev/zram0\" "
        "RUN+=\"/run/current-system/profile/sbin/swapon "
-       (if (not (equal? -1 priority))
-         (string-append "--priority " (number->string priority) " ")
+       ;; XXX: The field is delayed while the deprecation warning remains in
+       ;; place, so we can't use match to fetch it (it would give a promise)
+       (if (zram-device-configuration-priority config)
+           (string-append "--priority "
+                          (number->string
+                           (zram-device-configuration-priority config))
+                          " ")
          "")
        "/dev/zram0\"\n"))))
 
-- 
2.36.0

bug#54783: [PATCH v2 2/2] doc: Remove double copyright.

[Josselin Poiret via Bug reports for GNU Guix]

* doc/guix.texi: Remove doubled Josselin Poiret copyright line.
---
 doc/guix.texi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 5f0120a3bd..b960c546a8 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -99,7 +99,6 @@ Copyright @copyright{} 2021 Alice Brenon@*
 Copyright @copyright{} 2021, 2022 Josselin Poiret@*
 Copyright @copyright{} 2021 Andrew Tropin@*
 Copyright @copyright{} 2021 Sarah Morgensen@*
-Copyright @copyright{} 2021 Josselin Poiret@*
 Copyright @copyright{} 2022 Remco van 't Veer@*
 Copyright @copyright{} 2022 Aleksandr Vityazev@*
 Copyright @copyright{} 2022 Philip M@sup{c}Grath@*
-- 
2.36.0

bug#54783: [PATCH 2/2] doc: Remove double copyright.

[Josselin Poiret via Bug reports for GNU Guix]

* doc/guix.texi: Remove doubled Josselin Poiret copyright line.
---
 doc/guix.texi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 31f391357d..d9b26e440c 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -99,7 +99,6 @@ Copyright @copyright{} 2021 Alice Brenon@*
 Copyright @copyright{} 2021, 2022 Josselin Poiret@*
 Copyright @copyright{} 2021 Andrew Tropin@*
 Copyright @copyright{} 2021 Sarah Morgensen@*
-Copyright @copyright{} 2021 Josselin Poiret@*
 Copyright @copyright{} 2022 Remco van 't Veer@*
 Copyright @copyright{} 2022 Aleksandr Vityazev@*
 
-- 
2.34.0

bug#54783: ZRAM default priority wrong

[Maxim Cournoyer]

Hi Josselin,

[...]
>
> On the Guix side of things, we should really be using the same interface
> as swap-space, as I think the distinction between #f and 0 to 32767 is
> clearer.  I'll send some patches that adress this soon, along with the
> zram-service-type documentation.

I've made this smallish change:

--8<---------------cut here---------------start------------->8---
modified   gnu/services/linux.scm
@@ -296,14 +296,12 @@ (define (zram-device-configuration->udev-string config)
          "")
        "RUN+=\"/run/current-system/profile/sbin/mkswap /dev/zram0\" "
        "RUN+=\"/run/current-system/profile/sbin/swapon "
-       ;; XXX: The field is delayed while the deprecation warning remains in
-       ;; place, so we can't use match to fetch it (it would give a promise)
-       (if (zram-device-configuration-priority config)
-           (string-append "--priority "
-                          (number->string
-                           (zram-device-configuration-priority config))
-                          " ")
-         "")
+       ;; TODO: Revert to simply use 'priority' after removing the deprecation
+       ;; warning and the delayed property of the field.
+       (let ((priority* (force priority)))
+         (if priority*
+             (format #f "--priority ~a " priority*)
+             ""))
        "/dev/zram0\"\n"))))
 
 (define %zram-device-config
 
--8<---------------cut here---------------end--------------->8---

Because the TODO comments seems more actionable for my future self, and
pushed as a99015c878.

Thanks Stefan for reporting the issue and for Josselin for fixing it!

Closing.

Maxim