[dpdk-dev] [PATCH v6 2/2] power: refactor pstate and acpi code
David Hunt
david.hunt at intel.com
Thu Jul 8 15:33:19 CEST 2021
On 8/7/2021 1:49 PM, David Marchand wrote:
> On Wed, Jun 23, 2021 at 2:04 PM David Hunt <david.hunt at intel.com> wrote:
>> From: Anatoly Burakov <anatoly.burakov at intel.com>
>>
>> Currently, ACPI and PSTATE modes have lots of code duplication,
>> confusing logic, and a bunch of other issues that can, and have, led to
>> various bugs and resource leaks.
>>
>> This commit factors out the common parts of sysfs reading/writing for
>> ACPI and PSTATE drivers.
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov at intel.com>
>> Signed-off-by: David Hunt <david.hunt at intel.com>
>>
>> ---
>> changes in v5
>> * fixed bugs raised by Richael Zhuang in review - open file rw+, etc.
>> * removed FOPS* and FOPEN* macros, which contained control statements.
>> * fixed some checkpatch warnings.
>> changes in v6
>> * fixed check of fputs return, negative on error.
>> ---
>> lib/power/meson.build | 7 +
>> lib/power/power_acpi_cpufreq.c | 192 ++++------------
>> lib/power/power_common.c | 146 ++++++++++++
>> lib/power/power_common.h | 17 ++
>> lib/power/power_pstate_cpufreq.c | 374 ++++++++++---------------------
>> 5 files changed, 335 insertions(+), 401 deletions(-)
>>
>> diff --git a/lib/power/meson.build b/lib/power/meson.build
>> index c1097d32f1..74c5f3a294 100644
>> --- a/lib/power/meson.build
>> +++ b/lib/power/meson.build
>> @@ -5,6 +5,13 @@ if not is_linux
>> build = false
>> reason = 'only supported on Linux'
>> endif
>> +
>> +# we do some snprintf magic so silence format-nonliteral
>> +flag_nonliteral = '-Wno-format-nonliteral'
>> +if cc.has_argument(flag_nonliteral)
>> + cflags += flag_nonliteral
>> +endif
>> +
> This can be removed with __rte_format_printf tag + API change below.
>
>
>> sources = files(
>> 'guest_channel.c',
>> 'power_acpi_cpufreq.c',
> [snip]
>
>> diff --git a/lib/power/power_common.c b/lib/power/power_common.c
>> index 67e3318ec7..4deb343dae 100644
>> --- a/lib/power/power_common.c
>> +++ b/lib/power/power_common.c
>> @@ -3,13 +3,20 @@
>> */
>>
>> #include <limits.h>
>> +#include <stdlib.h>
>> #include <stdio.h>
>> #include <string.h>
>>
>> +#include <rte_log.h>
>> +#include <rte_string_fns.h>
>> +
>> #include "power_common.h"
>>
>> #define POWER_SYSFILE_SCALING_DRIVER \
>> "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_driver"
>> +#define POWER_SYSFILE_GOVERNOR \
>> + "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor"
>> +#define POWER_CONVERT_TO_DECIMAL 10
>>
>> int
>> cpufreq_check_scaling_driver(const char *driver_name)
>> @@ -58,3 +65,142 @@ cpufreq_check_scaling_driver(const char *driver_name)
>> */
>> return 1;
>> }
> cpufreq_check_scaling_driver can use open_core_sysfs_file, right?
>
>
>> +
>> +int
>> +open_core_sysfs_file(const char *template, unsigned int core, const char *mode,
>> + FILE **f)
>> +{
>> + char fullpath[PATH_MAX];
>> + FILE *tmpf;
>> +
>> + /* silenced -Wformat-nonliteral here */
>> + snprintf(fullpath, sizeof(fullpath), template, core);
>> + tmpf = fopen(fullpath, mode);
>> + *f = tmpf;
>> + if (tmpf == NULL)
>> + return -1;
>> +
>> + return 0;
>> +}
>
> @@ -67,14 +67,15 @@ cpufreq_check_scaling_driver(const char *driver_name)
> }
>
> int
> -open_core_sysfs_file(const char *template, unsigned int core, const char *mode,
> - FILE **f)
> +open_core_sysfs_file(FILE **f, const char *mode, const char *format, ...)
> {
> char fullpath[PATH_MAX];
> + va_list ap;
> FILE *tmpf;
>
> - /* silenced -Wformat-nonliteral here */
> - snprintf(fullpath, sizeof(fullpath), template, core);
> + va_start(ap, format);
> + vsnprintf(fullpath, sizeof(fullpath), format, ap);
> + va_end(ap);
> tmpf = fopen(fullpath, mode);
> *f = tmpf;
> if (tmpf == NULL)
>
>
>
> With declaration in .h as:
>
> @@ -7,6 +7,8 @@
>
> #include <inttypes.h>
>
> +#include <rte_common.h>
> +
> #define RTE_POWER_INVALID_FREQ_INDEX (~0)
>
>
> @@ -21,8 +23,8 @@
> int cpufreq_check_scaling_driver(const char *driver);
> int power_set_governor(unsigned int lcore_id, const char *new_governor,
> char *orig_governor, size_t orig_governor_len);
> -int open_core_sysfs_file(const char *template, unsigned int core,
> - const char *mode, FILE **f);
> +int open_core_sysfs_file(FILE **f, const char *mode, const char *format, ...)
> + __rte_format_printf(3, 4);
> int read_core_sysfs_u32(FILE *f, uint32_t *val);
> int read_core_sysfs_s(FILE *f, char *buf, unsigned int len);
> int write_core_sysfs_s(FILE *f, const char *str);
>
>
> This leaves the possibility to use any kind of formats.
> And to be honest, I did not manage to make gcc happy otherwise (even
> when passing __rte_format_printf(3, 0)).
Thanks, David, good suggestions. I'll make those changes and respin.
Rgds,
Dave.
>
More information about the dev
mailing list