Re: [dpdk-dev] [PATCH v6 2/2] power: refactor pstate and acpi code

[David Hunt <david.hunt@intel.com>]

On 8/7/2021 1:49 PM, David Marchand wrote:
> On Wed, Jun 23, 2021 at 2:04 PM David Hunt <david.hunt@intel.com> wrote:
>> From: Anatoly Burakov <anatoly.burakov@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@intel.com>
>> Signed-off-by: David Hunt <david.hunt@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.


>