Re: [RFT PATCH v2 5/7] perf-iostat: Extend iostat interface to support different iostat PMUs

[Ian Rogers]

On Wed, May 6, 2026 at 11:37 PM Yushan Wang <wangyushan12@xxxxxxxxxx> wrote:
>
> From: Shiju Jose <shiju.jose@xxxxxxxxxx>
>
> Currently, platform-specific iostat code for PMUs is implemented as a
> common iostat callback interface and linked during build. This approach
> limits support for iostat across different implementations of PMU of the
> same architecture.
>
> To address this, extend common iostat interface to provide support for
> different PMUs by allowing each PMU to register itself and receive
> callbacks to its PMU-specific functions through the unified iostat
> framework.
>
> Signed-off-by: Shiju Jose  <shiju.jose@xxxxxxxxxx>
> Co-developed-by: Yushan Wang <wangyushan12@xxxxxxxxxx>
> Signed-off-by: Yushan Wang <wangyushan12@xxxxxxxxxx>
> ---
>  tools/perf/util/iostat.c | 88 ++++++++++++++++++++++++++++++----------
>  tools/perf/util/iostat.h | 38 ++++++++++++++++-
>  2 files changed, 102 insertions(+), 24 deletions(-)
>
> diff --git a/tools/perf/util/iostat.c b/tools/perf/util/iostat.c
> index a68ab100780d..90607d1cf3fa 100644
> --- a/tools/perf/util/iostat.c
> +++ b/tools/perf/util/iostat.c
> @@ -1,47 +1,91 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include "util/iostat.h"
> -#include "util/debug.h"
> +
> +/*
> + * Below iostat_* function calls are scattered through out perf stat process,
> + * allowing multiple iostat PMUs and iterated them in following functions may
> + * violate calling conventions or cause incorrect display.
> + *
> + * Default to register the first PMU device that matches any of the specified
> + * iostat pmu name wildcards.
> + */
> +static struct iostat_pmu *iostat_pmu;

Could there be more than set of printing functions?

>  enum iostat_mode_t iostat_mode = IOSTAT_NONE;
>
> -__weak int iostat_prepare(struct evlist *evlist __maybe_unused,
> -                         struct perf_stat_config *config __maybe_unused)
> +__weak int iostat_prepare(struct evlist *evlist, struct perf_stat_config *config)

Eww.. weak.

> +{
> +       if (!iostat_pmu)
> +               return -1;
> +
> +       return iostat_pmu->prepare(evlist, config);
> +}
> +
> +__weak int iostat_parse(const struct option *opt, const char *str, int unset)
>  {
> -       return -1;
> +       if (!iostat_pmu)
> +               return -1;
> +
> +       return iostat_pmu->parse(opt, str, unset);
>  }
>
> -__weak int iostat_parse(const struct option *opt __maybe_unused,
> -                        const char *str __maybe_unused,
> -                        int unset __maybe_unused)
> +__weak void iostat_list(struct evlist *evlist, struct perf_stat_config *config)
>  {
> -       pr_err("iostat mode is not supported on current platform\n");
> -       return -1;
> +       iostat_pmu->list(evlist, config);
>  }
>
> -__weak void iostat_list(struct evlist *evlist __maybe_unused,
> -                      struct perf_stat_config *config __maybe_unused)
> +__weak void iostat_release(struct evlist *evlist)
>  {
> +       iostat_pmu->release(evlist);
>  }
>
> -__weak void iostat_release(struct evlist *evlist __maybe_unused)
> +__weak void iostat_print_header_prefix(struct perf_stat_config *config)
>  {
> +       iostat_pmu->print_header_prefix(config);
>  }
>
> -__weak void iostat_print_header_prefix(struct perf_stat_config *config __maybe_unused)
> +__weak void iostat_print_metric(struct perf_stat_config *config,
> +                               struct evsel *evsel,
> +                               struct perf_stat_output_ctx *out)
>  {
> +       iostat_pmu->print_metric(config, evsel, out);
>  }
>
> -__weak void iostat_print_metric(struct perf_stat_config *config __maybe_unused,
> -                               struct evsel *evsel __maybe_unused,
> -                               struct perf_stat_output_ctx *out __maybe_unused)
> +__weak void iostat_print_counters(struct evlist *evlist,
> +                                 struct perf_stat_config *config,
> +                                 struct timespec *ts, char *prefix,
> +                                 iostat_print_counter_t print_cnt_cb,
> +                                 void *arg)
>  {
> +       iostat_pmu->print_counters(evlist, config, ts, prefix,
> +                                  print_cnt_cb, arg);
> +}
> +
> +void register_iostat_pmu(struct iostat_pmu *pmu)
> +{
> +       if (!pmu || !pmu->match)
> +               return;
> +
> +       if (iostat_pmu || !pmu->match(pmu))
> +               return;
> +
> +       iostat_pmu = pmu;
> +}
> +
> +static void unregister_iostat_pmu(void)
> +{
> +       if (!iostat_pmu)
> +               return;
> +
> +       /*
> +        * Release function of iostat_pmu is called on the exit of cmd_stat, we
> +        * don't need to call release function here.
> +        */
> +       iostat_pmu = NULL;
>  }
>
> -__weak void iostat_print_counters(struct evlist *evlist __maybe_unused,
> -                                 struct perf_stat_config *config __maybe_unused,
> -                                 struct timespec *ts __maybe_unused,
> -                                 char *prefix __maybe_unused,
> -                                 iostat_print_counter_t print_cnt_cb __maybe_unused,
> -                                 void *arg __maybe_unused)
> +__attribute__((destructor))

I don't believe using this attribute is standard in either perf or the kernel.

> +static void iostat_exit(void)
>  {
> +       unregister_iostat_pmu();
>  }
> diff --git a/tools/perf/util/iostat.h b/tools/perf/util/iostat.h
> index 820930a096d9..5cc8963c6122 100644
> --- a/tools/perf/util/iostat.h
> +++ b/tools/perf/util/iostat.h
> @@ -10,6 +10,7 @@
>  #ifndef _IOSTAT_H
>  #define _IOSTAT_H
>
> +#include <stdbool.h>
>  #include <subcmd/parse-options.h>
>  #include "util/stat.h"
>  #include "util/parse-events.h"
> @@ -31,8 +32,7 @@ extern enum iostat_mode_t iostat_mode;
>  typedef void (*iostat_print_counter_t)(struct perf_stat_config *, struct evsel *, void *);
>
>  int iostat_prepare(struct evlist *evlist, struct perf_stat_config *config);
> -int iostat_parse(const struct option *opt, const char *str,
> -                int unset __maybe_unused);
> +int iostat_parse(const struct option *opt, const char *str, int unset);
>  void iostat_list(struct evlist *evlist, struct perf_stat_config *config);
>  void iostat_release(struct evlist *evlist);
>  void iostat_print_header_prefix(struct perf_stat_config *config);
> @@ -42,4 +42,38 @@ void iostat_print_counters(struct evlist *evlist,
>                            struct perf_stat_config *config, struct timespec *ts,
>                            char *prefix, iostat_print_counter_t print_cnt_cb, void *arg);
>
> +/**
> + * struct iostat_pmu - Callbacks for an iostat-capable PMU backend.
> + * @pmu_name_wildcard: Glob pattern to identify the PMU (e.g. "uncore_iio*").
> + * @match: Detect whether matching PMUs exist on this system.
> + * @prepare: Set up events and config for iostat collection.
> + * @parse: Parse the --iostat option argument.
> + * @list: Display available iostat PMU instances.
> + * @print_header_prefix: Print the column header prefix.
> + * @print_metric: Format and print one metric value.
> + * @print_counters:  Iterate over counters and print per-port results.
> + * @release: Clean up PMU-specific resources.
> + */
> +struct iostat_pmu {

I wonder if iostat_pmu is the best name here, perhaps
iostat_callbacks? It would be nice not to overload the term PMU in the
code with regular PMUs and the iostat PMUs.

Thanks,
Ian

> +       const char *pmu_name_wildcard;
> +       bool (*match)(struct iostat_pmu *iostat_pmu);
> +       int (*prepare)(struct evlist *evlist, struct perf_stat_config *config);
> +       int (*parse)(const struct option *opt, const char *str, int unset);
> +       void (*list)(struct evlist *evlist, struct perf_stat_config *config);
> +       void (*print_header_prefix)(struct perf_stat_config *config);
> +       void (*print_metric)(struct perf_stat_config *config, struct evsel *evsel,
> +                            struct perf_stat_output_ctx *out);
> +       void (*print_counters)(struct evlist *evlist,
> +                              struct perf_stat_config *config, struct timespec *ts,
> +                              char *prefix, iostat_print_counter_t print_cnt_cb, void *arg);
> +       void (*release)(struct evlist *evlist __maybe_unused);
> +};
> +
> +/*
> + * Register an iostat PMU handler. Called from __attribute__((constructor))
> + * functions in each backend's translation unit.
> + *
> + * Only the first matched backend is activated.
> + */
> +void register_iostat_pmu(struct iostat_pmu *iostat_pmu);
>  #endif /* _IOSTAT_H */
> --
> 2.33.0
>