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

[David Marchand]

On Wed, Jun 23, 2021 at 2:04 PM David Hunt <david.h...@intel.com> wrote:
>
> From: Anatoly Burakov <anatoly.bura...@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.bura...@intel.com>
> Signed-off-by: David Hunt <david.h...@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)).


-- 
David Marchand