Re: [PATCH 1/2] cpupower: Introduce powercap intel-rapl library helpers and powercap-info command

Shuah Khan
2022-11-18T22:14:26Z

Hi Thomas,

On 11/17/22 02:47, Thomas Renninger wrote:
> Read out powercap zone information via:
> cpupower powercap-info
> and show the zone hierarchy to the user:
>
> ./cpupower powercap-info
> Driver: intel-rapl
> Powercap domain hierarchy:
>
> Zone: package-0 (enabled)
> Power consumption can be monitored in micro Watts
>
> Zone: core (disabled)
> Power consumption can be monitored in micro Watts
>
> Zone: uncore (disabled)
> Power consumption can be monitored in micro Watts
>
> Zone: dram (disabled)
> Power consumption can be monitored in micro Watts
>
> There is a dummy -a option for powercap-info which can/should be used to show
> more detailed info later. Like that other args can be added easily later as well.
>
> A enable/disable option via powercap-set subcommand is also an enhancement
> for later.
>
> Also not all RAPL domains are shown. The func walking through RAPL subdomains
> is restricted and hardcoded to: "intel-rapl/intel-rapl:0"
> On my system above powercap domains map to:
> intel-rapl/intel-rapl:0
> -> pack (age-0)
> intel-rapl/intel-rapl:0/intel-rapl:0:0
> -> core
> intel-rapl/intel-rapl:0/intel-rapl:0:1
> -> uncore
>
> Missing ones on my system are:
> intel-rapl-mmio/intel-rapl-mmio:0
> -> pack (age-0)
>
> intel-rapl/intel-rapl:1
> -> psys
>
> This could get enhanced in:
> struct powercap_zone *powercap_init_zones()
> and adopted to walk through all intel-rapl zones, but
> also to other powercap drivers like dtpm
> (Dynamic Thermal Power Management framework),
> cmp with: drivers/powercap/dtpm_*
>

Thanks for the patch. A few comments below

> Signed-off-by: Thomas Renninger <trenn@suse.de>
> CC: Shuah Khan <skhan@linuxfoundation.org>
> ---
> tools/power/cpupower/Makefile | 14 ++-
> tools/power/cpupower/utils/builtin.h | 2 +
> tools/power/cpupower/utils/cpupower.c | 1 +
> tools/power/cpupower/utils/powercap-info.c | 113 +++++++++++++++++++++
> 4 files changed, 126 insertions(+), 4 deletions(-)
> create mode 100644 tools/power/cpupower/utils/powercap-info.c
>
> diff --git a/tools/power/cpupower/Makefile b/tools/power/cpupower/Makefile
> index e9b6de314654..9fd3b309b3a6 100644
> --- a/tools/power/cpupower/Makefile
> +++ b/tools/power/cpupower/Makefile
> @@ -133,7 +133,7 @@ UTIL_OBJS = utils/helpers/amd.o utils/helpers/msr.o \
> utils/idle_monitor/mperf_monitor.o utils/idle_monitor/cpupower-monitor.o \
> utils/cpupower.o utils/cpufreq-info.o utils/cpufreq-set.o \
> utils/cpupower-set.o utils/cpupower-info.o utils/cpuidle-info.o \
> - utils/cpuidle-set.o
> + utils/cpuidle-set.o utils/powercap-info.o
>
> UTIL_SRC := $(UTIL_OBJS:.o=.c)
>
> @@ -143,9 +143,12 @@ UTIL_HEADERS = utils/helpers/helpers.h utils/idle_monitor/cpupower-monitor.h \
> utils/helpers/bitmask.h \
> utils/idle_monitor/idle_monitors.h utils/idle_monitor/idle_monitors.def
>
> -LIB_HEADERS = lib/cpufreq.h lib/cpupower.h lib/cpuidle.h lib/acpi_cppc.h
> -LIB_SRC = lib/cpufreq.c lib/cpupower.c lib/cpuidle.c lib/acpi_cppc.c
> -LIB_OBJS = lib/cpufreq.o lib/cpupower.o lib/cpuidle.o lib/acpi_cppc.o
> +LIB_HEADERS = lib/cpufreq.h lib/cpupower.h lib/cpuidle.h lib/acpi_cppc.h \
> + lib/powercap.h
> +LIB_SRC = lib/cpufreq.c lib/cpupower.c lib/cpuidle.c lib/acpi_cppc.c \
> + lib/powercap.c
> +LIB_OBJS = lib/cpufreq.o lib/cpupower.o lib/cpuidle.o lib/acpi_cppc.o \
> + lib/powercap.o
> LIB_OBJS := $(addprefix $(OUTPUT),$(LIB_OBJS))
>
> override CFLAGS += -pipe
> @@ -276,6 +279,7 @@ install-lib: libcpupower
> $(INSTALL) -d $(DESTDIR)${includedir}
> $(INSTALL_DATA) lib/cpufreq.h $(DESTDIR)${includedir}/cpufreq.h
> $(INSTALL_DATA) lib/cpuidle.h $(DESTDIR)${includedir}/cpuidle.h
> + $(INSTALL_DATA) lib/powercap.h $(DESTDIR)${includedir}/powercap.h
>
> install-tools: $(OUTPUT)cpupower
> $(INSTALL) -d $(DESTDIR)${bindir}
> @@ -292,6 +296,7 @@ install-man:
> $(INSTALL_DATA) -D man/cpupower-set.1 $(DESTDIR)${mandir}/man1/cpupower-set.1
> $(INSTALL_DATA) -D man/cpupower-info.1 $(DESTDIR)${mandir}/man1/cpupower-info.1
> $(INSTALL_DATA) -D man/cpupower-monitor.1 $(DESTDIR)${mandir}/man1/cpupower-monitor.1
> + $(INSTALL_DATA) -D man/cpupower-powercap-info.1 $(DESTDIR)${mandir}/man1/cpupower-powercap-info.1
>
> install-gmo: create-gmo
> $(INSTALL) -d $(DESTDIR)${localedir}
> @@ -321,6 +326,7 @@ uninstall:
> - rm -f $(DESTDIR)${mandir}/man1/cpupower-set.1
> - rm -f $(DESTDIR)${mandir}/man1/cpupower-info.1
> - rm -f $(DESTDIR)${mandir}/man1/cpupower-monitor.1
> + - rm -f $(DESTDIR)${mandir}/man1/cpupower-powercap-info.1
> - for HLANG in $(LANGUAGES); do \
> rm -f $(DESTDIR)${localedir}/$$HLANG/LC_MESSAGES/cpupower.mo; \
> done;
> diff --git a/tools/power/cpupower/utils/builtin.h b/tools/power/cpupower/utils/builtin.h
> index f7065ae60a14..e1caefd467cd 100644
> --- a/tools/power/cpupower/utils/builtin.h
> +++ b/tools/power/cpupower/utils/builtin.h
> @@ -8,6 +8,8 @@ extern int cmd_freq_set(int argc, const char **argv);
> extern int cmd_freq_info(int argc, const char **argv);
> extern int cmd_idle_set(int argc, const char **argv);
> extern int cmd_idle_info(int argc, const char **argv);
> +extern int cmd_cap_info(int argc, const char **argv);
> +extern int cmd_cap_set(int argc, const char **argv);
> extern int cmd_monitor(int argc, const char **argv);
>
> #endif
> diff --git a/tools/power/cpupower/utils/cpupower.c b/tools/power/cpupower/utils/cpupower.c
> index 8ac3304a9957..9ec973165af1 100644
> --- a/tools/power/cpupower/utils/cpupower.c
> +++ b/tools/power/cpupower/utils/cpupower.c
> @@ -54,6 +54,7 @@ static struct cmd_struct commands[] = {
> { "frequency-set", cmd_freq_set, 1 },
> { "idle-info", cmd_idle_info, 0 },
> { "idle-set", cmd_idle_set, 1 },
> + { "powercap-info", cmd_cap_info, 0 },
> { "set", cmd_set, 1 },
> { "info", cmd_info, 0 },
> { "monitor", cmd_monitor, 0 },
> diff --git a/tools/power/cpupower/utils/powercap-info.c b/tools/power/cpupower/utils/powercap-info.c
> new file mode 100644
> index 000000000000..989fafe581cf
> --- /dev/null
> +++ b/tools/power/cpupower/utils/powercap-info.c
> @@ -0,0 +1,113 @@
> +#include <unistd.h>
> +#include <stdio.h>
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <string.h>
> +
> +#include <getopt.h>
> +
> +#include "powercap.h"
> +#include "helpers/helpers.h"
> +
> +int powercap_show_all = 0;

Globals don't need initualization to 0.

> +
> +static struct option info_opts[] =
> +{

Open brace needs to be on the previous line. A few more errors about
function definitions etc..

Please add SPDX to new files.

Running checkpatch will show you the errors.

Compile failed with ./lib/powercap.o: No such file or directory
I installed libcpupower.so.0.0.1

thanks,
-- Shuah