Re: [PATCH] cpupower: add a systemd service to run cpupower

["John B. Wyatt IV" <jwyatt@redhat.com>]

Thank you for sending the patch Francesco.

There are several issues:

On Tue, Apr 08, 2025 at 10:32:46PM +0200, Francesco Poli (wintermute) wrote:
> One of the most typical use cases of the 'cpupower' utility works as
> follows: run 'cpupower' at boot with the desired command-line options
> and then forget about it.
> 
> Add a systemd service (disabled by default) that automates this use
> case (for environments where the initialization system is 'systemd'),
> by running 'cpupower' at boot with the settings read from a default
> configuration file.
> 
> The systemd service, the associated support script and the
> corresponding default configuration file are derived from what is
> provided by the Arch Linux package (under "GPL-2.0-or-later" terms),
> modernized and enhanced in various ways (the script has also been
> checked with 'shellcheck').
> 
> Link: https://gitlab.archlinux.org/archlinux/packaging/packages/linux-tools/-/tree/dd2e2a311e05413d0d87a0346ffce8c7e98d6d2b
> 
> Signed-off-by: Francesco Poli (wintermute) <invernomuto@paranoici.org>
> ---
>  tools/power/cpupower/Makefile            | 14 ++++++++++++
>  tools/power/cpupower/README              | 18 +++++++++++++++
>  tools/power/cpupower/cpupower.default    | 28 ++++++++++++++++++++++++
>  tools/power/cpupower/cpupower.service.in | 16 ++++++++++++++
>  tools/power/cpupower/cpupower.sh         | 26 ++++++++++++++++++++++
>  5 files changed, 102 insertions(+)
>  create mode 100644 tools/power/cpupower/cpupower.default
>  create mode 100644 tools/power/cpupower/cpupower.service.in
>  create mode 100644 tools/power/cpupower/cpupower.sh
> 
> diff --git a/tools/power/cpupower/Makefile b/tools/power/cpupower/Makefile
> index 51a95239fe06..2bdfb2bfe88a 100644
> --- a/tools/power/cpupower/Makefile
> +++ b/tools/power/cpupower/Makefile
> @@ -2,6 +2,7 @@
>  # Makefile for cpupower
>  #
>  # Copyright (C) 2005,2006 Dominik Brodowski <linux@dominikbrodowski.net>
> +# Copyright (C) 2025      Francesco Poli <invernomuto@paranoici.org>
>  #
>  # Based largely on the Makefile for udev by:
>  #
> @@ -68,6 +69,7 @@ bindir ?=	/usr/bin
>  sbindir ?=	/usr/sbin
>  mandir ?=	/usr/man
>  libdir ?=	/usr/lib
> +libexecdir ?=	/usr/libexec
>  includedir ?=	/usr/include
>  localedir ?=	/usr/share/locale
>  docdir ?=       /usr/share/doc/packages/cpupower
> @@ -80,6 +82,7 @@ CP = cp -fpR
>  INSTALL = /usr/bin/install -c
>  INSTALL_PROGRAM = ${INSTALL}
>  INSTALL_DATA  = ${INSTALL} -m 644
> +SETPERM_DATA  = chmod 644
>  #bash completion scripts get sourced and so they should be rw only.
>  INSTALL_SCRIPT = ${INSTALL} -m 644
>  
> @@ -299,6 +302,14 @@ install-tools: $(OUTPUT)cpupower
>  	$(INSTALL_PROGRAM) $(OUTPUT)cpupower $(DESTDIR)${bindir}
>  	$(INSTALL) -d $(DESTDIR)${bash_completion_dir}
>  	$(INSTALL_SCRIPT) cpupower-completion.sh '$(DESTDIR)${bash_completion_dir}/cpupower'
> +	$(INSTALL) -d $(DESTDIR)${confdir}default
> +	$(INSTALL_DATA) cpupower.default '$(DESTDIR)${confdir}default/cpupower'
> +	$(INSTALL) -d $(DESTDIR)${libexecdir}
> +	$(INSTALL_PROGRAM) cpupower.sh '$(DESTDIR)${libexecdir}/cpupower'
> +	$(INSTALL) -d $(DESTDIR)${libdir}/systemd/system
> +	sed 's|___CDIR___|$(DESTDIR)${confdir}|; s|___LDIR___|$(DESTDIR)${libexecdir}|' cpupower.service.in > '$(DESTDIR)${libdir}/systemd/system/cpupower.service'
> +	$(SETPERM_DATA) '$(DESTDIR)${libdir}/systemd/system/cpupower.service'
> +	if test -d /run/systemd/system; then systemctl daemon-reload; fi
>  
>  install-man:
>  	$(INSTALL_DATA) -D man/cpupower.1 $(DESTDIR)${mandir}/man1/cpupower.1
> @@ -333,6 +344,9 @@ uninstall:
>  	- rm -f $(DESTDIR)${includedir}/cpufreq.h
>  	- rm -f $(DESTDIR)${includedir}/cpuidle.h
>  	- rm -f $(DESTDIR)${bindir}/utils/cpupower
> +	- rm -f $(DESTDIR)${confdir}default/cpupower
> +	- rm -f $(DESTDIR)${libexecdir}/cpupower
> +	- rm -f $(DESTDIR)${libdir}/systemd/system/cpupower.service
>  	- rm -f $(DESTDIR)${mandir}/man1/cpupower.1
>  	- rm -f $(DESTDIR)${mandir}/man1/cpupower-frequency-set.1
>  	- rm -f $(DESTDIR)${mandir}/man1/cpupower-frequency-info.1
> diff --git a/tools/power/cpupower/README b/tools/power/cpupower/README
> index 2678ed81d311..3c34ef67e0cf 100644
> --- a/tools/power/cpupower/README
> +++ b/tools/power/cpupower/README
> @@ -59,6 +59,10 @@ $ sudo make install
>  -----------------------------------------------------------------------
>  | man pages              | /usr/man                                   |
>  -----------------------------------------------------------------------
> +| systemd service        | /usr/lib                                   |
> +-----------------------------------------------------------------------
> +| systemd support script | /usr/libexec                               |
> +-----------------------------------------------------------------------
>  
>  To put it in other words it makes build results available system-wide,
>  enabling any user to simply start using it without any additional steps
> @@ -109,6 +113,10 @@ The files will be installed to the following dirs:
>  -----------------------------------------------------------------------
>  | man pages              | ${DESTDIR}/usr/man                         |
>  -----------------------------------------------------------------------
> +| systemd service        | ${DESTDIR}/usr/lib                         |
> +-----------------------------------------------------------------------
> +| systemd support script | ${DESTDIR}/usr/libexec                     |
> +-----------------------------------------------------------------------
>  
>  If you look at the table for the default 'make' output dirs you will
>  notice that the only difference with the non-default case is the
> @@ -173,6 +181,16 @@ The issue is that binary cannot find the 'libcpupower' library. So, we
>  shall point to the lib dir:
>  sudo LD_LIBRARY_PATH=lib64/ ./bin/cpupower
>  
> +systemd service
> +---------------
> +
> +A systemd service is also provided to run the cpupower utility at boot with
> +settings read from a configuration file. In order to enable this systemd
> +service, edit '${DESTDIR}/etc/default/cpupower' and then issue the following
> +command:

Edit what? What should they change?

I am new to systemd files so knowing how to test this is important.

> +
> +$ sudo systemctl enable --now cpupower.service
> +
>  
>  THANKS
>  ------
> diff --git a/tools/power/cpupower/cpupower.default b/tools/power/cpupower/cpupower.default
> new file mode 100644
> index 000000000000..b2fd3c37e277
> --- /dev/null
> +++ b/tools/power/cpupower/cpupower.default
> @@ -0,0 +1,28 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (C) 2012, Sébastien Luttringer
> +# Copyright (C) 2024, Francesco Poli <invernomuto@paranoici.org>
> +
> +# defaults file for linux-cpupower

Should be:

# Default file for linux-cpupower

Otherwise, the additional comments added to the Arch files are welcome.
:)

> +
> +# --- CPU clock frequency ---
> +
> +# Define CPU governor
> +# valid governors: ondemand, performance, powersave, conservative, userspace.

Since we are at proofreading; please capitalize valid.

> +#GOVERNOR='ondemand'

One thing I noticed is that you changed the variables to their
uppercase. Is there a reason for that? Have you tested it?

Last you wrote, you copied the Arch files to test in Debian. Once again,
not familiar with systemd files so this is a change.

> +
> +# Limit frequency range
> +# Valid suffixes: Hz, kHz (default), MHz, GHz, THz
> +#MIN_FREQ="2.25GHz"
> +#MAX_FREQ="3GHz"
> +
> +# Specific frequency to be set.
> +# Requires userspace governor to be available.
> +# If this option is set, all the previous frequency options are ignored
> +#FREQ=
> +
> +# --- CPU policy ---
> +
> +# Sets a register on supported Intel processore which allows software to convey
> +# its policy for the relative importance of performance versus energy savings to
> +# the  processor. See man (1) CPUPOWER-SET for additional details.

double space

> +#PERF_BIAS=
> diff --git a/tools/power/cpupower/cpupower.service.in b/tools/power/cpupower/cpupower.service.in
> new file mode 100644
> index 000000000000..f91eaed03872
> --- /dev/null
> +++ b/tools/power/cpupower/cpupower.service.in
> @@ -0,0 +1,16 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (C) 2012-2020, Sébastien Luttringer
> +# Copyright (C) 2024, Francesco Poli <invernomuto@paranoici.org>
> +
> +[Unit]
> +Description=Apply cpupower configuration
> +ConditionVirtualization=!container
> +
> +[Service]
> +Type=oneshot
> +EnvironmentFile=-___CDIR___default/cpupower
> +ExecStart=___LDIR___/cpupower
> +RemainAfterExit=yes
> +
> +[Install]
> +WantedBy=multi-user.target
> diff --git a/tools/power/cpupower/cpupower.sh b/tools/power/cpupower/cpupower.sh
> new file mode 100644
> index 000000000000..a37dd4cfdb2b
> --- /dev/null
> +++ b/tools/power/cpupower/cpupower.sh
> @@ -0,0 +1,26 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (C) 2012, Sébastien Luttringer
> +# Copyright (C) 2024, Francesco Poli <invernomuto@paranoici.org>
> +
> +ESTATUS=0
> +
> +# apply CPU clock frequency options
> +if test -n "$FREQ"
> +then
> +    cpupower frequency-set -f "$FREQ" > /dev/null || ESTATUS=1
> +elif test -n "${GOVERNOR}${MIN_FREQ}${MAX_FREQ}"
> +then
> +    cpupower frequency-set \
> +      ${GOVERNOR:+ -g "$GOVERNOR"} \
> +      ${MIN_FREQ:+ -d "$MIN_FREQ"} ${MAX_FREQ:+ -u "$MAX_FREQ"} \
> +      > /dev/null || ESTATUS=1
> +fi
> +
> +# apply CPU policy options
> +if test -n "$PERF_BIAS"
> +then
> +    cpupower set -b "$PERF_BIAS" > /dev/null || ESTATUS=1
> +fi
> +
> +exit $ESTATUS

Where did you get this file? I do not believe I see it in the Arch
package repo. What is it used for with the systemd scripts? They do not
reference it.

https://gitlab.archlinux.org/archlinux/packaging/packages/linux-tools/-/tree/main?ref_type=heads

> 
> base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3
> -- 
> 2.47.2
> 

Just got back from PTO so may have more comments later.

-- 
Sincerely,
John Wyatt
Software Engineer, Core Kernel
Red Hat