Re: [PATCH 3/5] gnu: Add tlp service.

[Clément Lassieur]

Mathieu Othacehe <address@hidden> writes:

> * gnu/services/pm.scm: New file.
> * gnu/local.mk (GNU_SYSTEM_MODULES): Add gnu/services/tlp.scm.
> * doc/guix.texi (Power management Services): New section.

Hi Mathieu,

Very nice!

On my x200, with tlp, powertop reports:

    The battery reports a discharge rate of 10.1 W
    The estimated remaining time is 4 hours, 52 minutes

Without tlp:

    The battery reports a discharge rate of 11.9 W
    The estimated remaining time is 4 hours, 11 minutes


Here are a few comments:

[...]

> +(define-module (gnu services pm)
> +  #:use-module (guix gexp)
> +  #:use-module (guix packages)
> +  #:use-module (guix records)
> +  #:use-module (gnu packages linux)
> +  #:use-module (gnu services)
> +  #:use-module (gnu services base)
> +  #:use-module (gnu services configuration)
> +  #:use-module (gnu services shepherd)
> +  #:use-module (gnu system shadow)
> +  #:export (tlp-service-type
> +            tlp-configuration
> +            generate-tlp-documentation))

I don't think generate-tlp-documentation needs to be exported.

> +(define (uglify-field-name field-name)
> +  (let ((str (symbol->string field-name)))
> +    (string-join (string-split
> +                  (string-upcase
> +                   (if (string-suffix? "?" str)
> +                       (substring str 0 (1- (string-length str)))
> +                       str))
> +                  #\-)
> +                 "_")))
> +
> +(define (serialize-field field-name val)
> +  (format #t "~a=~a\n" (uglify-field-name field-name) val))
> +
> +(define (serialize-boolean field-name val)
> +  (serialize-field field-name (if val "1" "0")))
> +(define-maybe boolean)
> +
> +(define (serialize-string field-name val)
> +  (serialize-field field-name val))
> +(define-maybe string)
> +
> +(define (space-separated-string-list? val)
> +  (and (list? val)
> +       (and-map (lambda (x)
> +                  (and (string? x) (not (string-index x #\space))))
> +                val)))
> +(define (serialize-space-separated-string-list field-name val)
> +  (serialize-field field-name
> +                   (format #f "~s"
> +                           (string-join val " "))))
> +(define-maybe space-separated-string-list)
> +
> +(define (non-negative-integer? val)
> +  (and (exact-integer? val) (not (negative? val))))
> +(define (serialize-non-negative-integer field-name val)
> +  (serialize-field field-name val))
> +(define-maybe non-negative-integer)
> +
> +(define (on-off-boolean? val)
> +  (boolean? val))
> +(define (serialize-on-off-boolean field-name val)
> +  (serialize-field field-name (if val "on" "off")))
> +(define-maybe on-off-boolean)
> +
> +(define (y-n-boolean? val)
> +  (boolean? val))
> +(define (serialize-y-n-boolean field-name val)
> +  (serialize-field field-name (if val "Y" "N")))
> +
> +(define-configuration tlp-configuration
> +  (tlp
> +   (package tlp)
> +   "The TLP package.")
> +
> +  (tlp-enable?
> +   (boolean #t)
> +   "Set to true if you wish to enable TLP.")

>From this point, the indentation is broken.

> +   (tlp-default-mode
> +    (string "AC")
> +    "Default mode when no power supply can be detected. Alternatives are
> +AC and BAT.")
> +
> +   (disk-idle-secs-on-ac
> +    (non-negative-integer 0)
> +    "Number of seconds Linux kernel has to wait after the disk goes idle,
> +before syncing on AC.")
> +
> +   (disk-idle-secs-on-bat
> +    (non-negative-integer 2)
> +    "Same as @code{disk-idle-ac} but on BAT mode.")
> +
> +   (max-lost-work-secs-on-ac
> +    (non-negative-integer 15)
> +    "Dirty pages flushing periodicity, expressed in seconds.")
> +
> +   (max-lost-work-secs-on-bat
> +    (non-negative-integer 60)
> +    "Same as @code{max-lost-work-secs-on-ac} but on BAT mode.")
> +
> +   (cpu-scaling-governor-on-ac
> +    (maybe-space-separated-string-list 'disabled)
> +    "CPU frequency scaling governor on AC mode. With intel_pstate
> +driver, alternatives are powersave and performance. With acpi-cpufreq driver,
> +alternatives are ondemand, powersave, performance and conservative.")

Please, could you put two spaces between phrases? :) As in "and
performance.  With...".  I've seen this in other parts of your patch as
well.

[...]

> +(define (tlp-shepherd-service config)
> +  (let* ((tlp-bin (file-append
> +                   (tlp-configuration-tlp config) "/bin/tlp"))
> +         (tlp-action (lambda args
> +                              #~(lambda _
> +                                  (zero? (system* #$tlp-bin 
> address@hidden))))))
> +    (list (shepherd-service
> +           (documentation "Run TLP script.")
> +           (provision '(tlp))
> +           (requirement '(syslogd user-processes))

I have not been able to see anything related to tlp in
/var/log/messages.  And if I set trace mode (TLP_DEBUG, in /etc/tlp), I
have an error message: "logger: unknown facility name: debug", which I
think could be patched in tlp source, maybe by removing "-p debug".  Or
maybe it is logger that needs to be patched, I don't know.

Otherwise syslogd would not be needed here.  WDYT?

BTW, could you consider adding TLP_DEBUG to the service?

> +           (start (tlp-action "init" "start"))
> +           (stop  (tlp-action "init" "stop"))))))
> +
> +(define (tlp-activation config)
> +  (let* ((config-dir "/etc")
> +         (config-str (with-output-to-string
> +                       (lambda ()
> +                         (serialize-configuration
> +                          config
> +                          tlp-configuration-fields))))
> +         (config-file (plain-file "tlp" config-str)))
> +  #~(begin
> +      (use-modules (guix build utils))
> +      (mkdir-p #$config-dir)
> +      (copy-file #$config-file (string-append #$config-dir "/tlp")))))

Here I think it is better to wrap the gexp in (with-imported-modules
'((guix build utils)) ...), as in the CUPS service for example, even
though I don't understand fully the consequences of not doing it.

[...]

Thanks for this :)
Clément