From: "Clément Lassieur" <clement@lassieur.org>
To: Mathieu Othacehe <m.othacehe@gmail.com>
Cc: guix-devel@gnu.org
Subject: Re: [PATCH 3/5] gnu: Add tlp service.
Date: Thu, 16 Mar 2017 17:25:33 +0100 [thread overview]
Message-ID: <87d1dhjiiq.fsf@lassieur.org> (raw)
In-Reply-To: <20170315204642.27626-4-m.othacehe@gmail.com>
Mathieu Othacehe <m.othacehe@gmail.com> 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 #$@args))))))
> + (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
next prev parent reply other threads:[~2017-03-16 16:25 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-15 20:46 [PATCH 0/5] Add tlp service Mathieu Othacehe
2017-03-15 20:46 ` [PATCH 1/5] gnu: tlp: Read configuration from /etc/tlp Mathieu Othacehe
2017-03-16 16:38 ` Clément Lassieur
2017-03-15 20:46 ` [PATCH 2/5] services: Factorize define-maybe macro Mathieu Othacehe
2017-03-16 17:25 ` Clément Lassieur
2017-03-15 20:46 ` [PATCH 3/5] gnu: Add tlp service Mathieu Othacehe
2017-03-16 16:25 ` Clément Lassieur [this message]
2017-03-16 17:08 ` Mathieu Othacehe
2017-03-16 21:05 ` Mathieu Othacehe
2017-03-16 16:33 ` Clément Lassieur
2017-03-16 17:09 ` Mathieu Othacehe
2017-03-16 17:18 ` Clément Lassieur
2017-03-15 20:46 ` [PATCH 4/5] doc: Re-generate openvpn service documentation Mathieu Othacehe
2017-03-16 17:33 ` Clément Lassieur
2017-03-15 20:46 ` [PATCH 5/5] services: Fix a typo which was corrected in generated doc Mathieu Othacehe
2017-03-16 16:44 ` Clément Lassieur
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://guix.gnu.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87d1dhjiiq.fsf@lassieur.org \
--to=clement@lassieur.org \
--cc=guix-devel@gnu.org \
--cc=m.othacehe@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/guix.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).