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

[Yushan Wang <wangyushan12@huawei.com>]

On 5/7/2026 11:47 PM, Ian Rogers wrote:
> On Wed, May 6, 2026 at 11:37 PM Yushan Wang <wangyushan12@huawei.com> wrote:
>>
>> From: Shiju Jose <shiju.jose@huawei.com>
>>
>> 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@huawei.com>
>> Co-developed-by: Yushan Wang <wangyushan12@huawei.com>
>> Signed-off-by: Yushan Wang <wangyushan12@huawei.com>
>> ---
>>  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?

At the second thought, to support cross-platform iostat, multiple printing
function should be supported, otherwise interpreting dumped perf.data from
another architecture may just not work.

This may require some more changes to the embedded iostat function calls, I can
try to add that support in the next version, thanks for pointing out!

>
>>  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.

These __weak symbols are added to avoid confliction with existing x86 iostat
implementations, and are removed in patch 6.

>

[...]

>>
>> -__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.

Sorry, I will fix that with explict calls to constructors and destructors.

>
>> +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.

Yes. iostat_backend or iostat_handlers may be more specific. I'll change that in
the next version

>
> Thanks,
> Ian

Thank you for review!
Yushan

>

[...]