When there are multiple PMUs that differ only by suffix, by default
just list the first one and skip all others. As the PMUs are sorted,
the scan routine checks that the PMU names match and the numbers are
consecutive. If "-v" is passed to "perf list" then list all PMUs.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/builtin-list.c | 8 +++++
tools/perf/util/pmus.c | 54 ++++++++++++++++++++++++++++++++--
tools/perf/util/print-events.h | 1 +
3 files changed, 61 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
index 7fec2cca759f..8fe4ddf02c14 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -423,6 +423,13 @@ static void json_print_metric(void *ps __maybe_unused, const char *group,
strbuf_release(&buf);
}
+static bool default_skip_duplicate_pmus(void *ps)
+{
+ struct print_state *print_state = ps;
+
+ return !print_state->long_desc;
+}
+
int cmd_list(int argc, const char **argv)
{
int i, ret = 0;
@@ -434,6 +441,7 @@ int cmd_list(int argc, const char **argv)
.print_end = default_print_end,
.print_event = default_print_event,
.print_metric = default_print_metric,
+ .skip_duplicate_pmus = default_skip_duplicate_pmus,
};
const char *cputype = NULL;
const char *unit_name = NULL;
diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
index 3581710667b0..5073843aca19 100644
--- a/tools/perf/util/pmus.c
+++ b/tools/perf/util/pmus.c
@@ -275,6 +275,50 @@ struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu)
return NULL;
}
+static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
+{
+ bool use_core_pmus = !pmu || pmu->is_core;
+ int last_pmu_name_len = 0;
+ unsigned long last_pmu_num = 0;
+ const char *last_pmu_name = (pmu && pmu->name) ? pmu->name : "";
+
+ if (!pmu) {
+ pmu_read_sysfs(/*core_only=*/false);
+ pmu = list_prepare_entry(pmu, &core_pmus, list);
+ } else
+ last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &last_pmu_num);
+
+ if (use_core_pmus) {
+ list_for_each_entry_continue(pmu, &core_pmus, list) {
+ unsigned long pmu_num = 0;
+ int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
+
+ if (last_pmu_name_len == pmu_name_len &&
+ (last_pmu_num + 1 == pmu_num) &&
+ !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
+ last_pmu_num++;
+ continue;
+ }
+ return pmu;
+ }
+ pmu = NULL;
+ pmu = list_prepare_entry(pmu, &other_pmus, list);
+ }
+ list_for_each_entry_continue(pmu, &other_pmus, list) {
+ unsigned long pmu_num = 0;
+ int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
+
+ if (last_pmu_name_len == pmu_name_len &&
+ (last_pmu_num + 1 == pmu_num) &&
+ !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
+ last_pmu_num++;
+ continue;
+ }
+ return pmu;
+ }
+ return NULL;
+}
+
const struct perf_pmu *perf_pmus__pmu_for_pmu_filter(const char *str)
{
struct perf_pmu *pmu = NULL;
@@ -429,10 +473,16 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
int printed = 0;
int len, j;
struct sevent *aliases;
+ struct perf_pmu *(*scan_fn)(struct perf_pmu *);
+
+ if (print_cb->skip_duplicate_pmus(print_state))
+ scan_fn = perf_pmus__scan_skip_duplicates;
+ else
+ scan_fn = perf_pmus__scan;
pmu = NULL;
len = 0;
- while ((pmu = perf_pmus__scan(pmu)) != NULL) {
+ while ((pmu = scan_fn(pmu)) != NULL) {
list_for_each_entry(event, &pmu->aliases, list)
len++;
if (pmu->selectable)
@@ -445,7 +495,7 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
}
pmu = NULL;
j = 0;
- while ((pmu = perf_pmus__scan(pmu)) != NULL) {
+ while ((pmu = scan_fn(pmu)) != NULL) {
bool is_cpu = pmu->is_core;
list_for_each_entry(event, &pmu->aliases, list) {
diff --git a/tools/perf/util/print-events.h b/tools/perf/util/print-events.h
index d7fab411e75c..bf4290bef0cd 100644
--- a/tools/perf/util/print-events.h
+++ b/tools/perf/util/print-events.h
@@ -26,6 +26,7 @@ struct print_callbacks {
const char *expr,
const char *threshold,
const char *unit);
+ bool (*skip_duplicate_pmus)(void *print_state);
};
/** Print all events, the default when no options are specified. */
--
2.41.0.640.ga95def55d0-googOn 10/08/2023 22:49, Ian Rogers wrote:
> When there are multiple PMUs that differ only by suffix, by default
> just list the first one and skip all others. As the PMUs are sorted,
> the scan routine checks that the PMU names match and the numbers are
> consecutive. If "-v" is passed to "perf list" then list all PMUs.
I really think that this should be merged with the next change. I don't
like the intermediate step of by default only printing the first PMU.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> tools/perf/builtin-list.c | 8 +++++
> tools/perf/util/pmus.c | 54 ++++++++++++++++++++++++++++++++--
> tools/perf/util/print-events.h | 1 +
> 3 files changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> index 7fec2cca759f..8fe4ddf02c14 100644
> --- a/tools/perf/builtin-list.c
> +++ b/tools/perf/builtin-list.c
> @@ -423,6 +423,13 @@ static void json_print_metric(void *ps __maybe_unused, const char *group,
> strbuf_release(&buf);
> }
>
> +static bool default_skip_duplicate_pmus(void *ps)
> +{
> + struct print_state *print_state = ps;
> +
> + return !print_state->long_desc;
> +}
> +
> int cmd_list(int argc, const char **argv)
> {
> int i, ret = 0;
> @@ -434,6 +441,7 @@ int cmd_list(int argc, const char **argv)
> .print_end = default_print_end,
> .print_event = default_print_event,
> .print_metric = default_print_metric,
> + .skip_duplicate_pmus = default_skip_duplicate_pmus,
> };
> const char *cputype = NULL;
> const char *unit_name = NULL;
> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index 3581710667b0..5073843aca19 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -275,6 +275,50 @@ struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu)
> return NULL;
> }
>
> +static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
> +{
> + bool use_core_pmus = !pmu || pmu->is_core;
> + int last_pmu_name_len = 0;
> + unsigned long last_pmu_num = 0;
> + const char *last_pmu_name = (pmu && pmu->name) ? pmu->name : "";
> +
> + if (!pmu) {
> + pmu_read_sysfs(/*core_only=*/false);
> + pmu = list_prepare_entry(pmu, &core_pmus, list);
> + } else
> + last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &last_pmu_num);
> +
> + if (use_core_pmus) {
> + list_for_each_entry_continue(pmu, &core_pmus, list) {
> + unsigned long pmu_num = 0;
> + int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
> +
> + if (last_pmu_name_len == pmu_name_len &&
> + (last_pmu_num + 1 == pmu_num) &&
> + !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
> + last_pmu_num++;
> + continue;
> + }
> + return pmu;
> + }
> + pmu = NULL;
you assign pmu NULL
> + pmu = list_prepare_entry(pmu, &other_pmus, list);
and then re-assign it. If list_prepare_entry() needs first arg = NULL,
then can just use NULL explicitly?
> + }
> + list_for_each_entry_continue(pmu, &other_pmus, list) {
> + unsigned long pmu_num = 0;
> + int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
> +
> + if (last_pmu_name_len == pmu_name_len &&
> + (last_pmu_num + 1 == pmu_num) &&
> + !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
> + last_pmu_num++;
> + continue;
Can some of this code be factored out from the previous patch? It's
doing something similar, right?
> + }
> + return pmu;
> + }
> + return NULL;
> +}
> +
> const struct perf_pmu *perf_pmus__pmu_for_pmu_filter(const char *str)
> {
> struct perf_pmu *pmu = NULL;
> @@ -429,10 +473,16 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
> int printed = 0;
> int len, j;
> struct sevent *aliases;
> + struct perf_pmu *(*scan_fn)(struct perf_pmu *);
> +
> + if (print_cb->skip_duplicate_pmus(print_state))
> + scan_fn = perf_pmus__scan_skip_duplicates;
> + else
> + scan_fn = perf_pmus__scan;
>
> pmu = NULL;
> len = 0;
> - while ((pmu = perf_pmus__scan(pmu)) != NULL) {
> + while ((pmu = scan_fn(pmu)) != NULL) {
> list_for_each_entry(event, &pmu->aliases, list)
> len++;
> if (pmu->selectable)
> @@ -445,7 +495,7 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
> }
> pmu = NULL;
> j = 0;
> - while ((pmu = perf_pmus__scan(pmu)) != NULL) {
> + while ((pmu = scan_fn(pmu)) != NULL) {
> bool is_cpu = pmu->is_core;
>
> list_for_each_entry(event, &pmu->aliases, list) {
> diff --git a/tools/perf/util/print-events.h b/tools/perf/util/print-events.h
> index d7fab411e75c..bf4290bef0cd 100644
> --- a/tools/perf/util/print-events.h
> +++ b/tools/perf/util/print-events.h
> @@ -26,6 +26,7 @@ struct print_callbacks {
> const char *expr,
> const char *threshold,
> const char *unit);
> + bool (*skip_duplicate_pmus)(void *print_state);
> };
>
> /** Print all events, the default when no options are specified. */
On Fri, Aug 11, 2023 at 8:51 AM John Garry <john.g.garry@oracle.com> wrote:
>
> On 10/08/2023 22:49, Ian Rogers wrote:
> > When there are multiple PMUs that differ only by suffix, by default
> > just list the first one and skip all others. As the PMUs are sorted,
> > the scan routine checks that the PMU names match and the numbers are
> > consecutive. If "-v" is passed to "perf list" then list all PMUs.
>
> I really think that this should be merged with the next change. I don't
> like the intermediate step of by default only printing the first PMU.
Ack. I'll leave it as 3 patches and then leave it to Arnaldo squash as
quite often he wants more patches.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> > tools/perf/builtin-list.c | 8 +++++
> > tools/perf/util/pmus.c | 54 ++++++++++++++++++++++++++++++++--
> > tools/perf/util/print-events.h | 1 +
> > 3 files changed, 61 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> > index 7fec2cca759f..8fe4ddf02c14 100644
> > --- a/tools/perf/builtin-list.c
> > +++ b/tools/perf/builtin-list.c
> > @@ -423,6 +423,13 @@ static void json_print_metric(void *ps __maybe_unused, const char *group,
> > strbuf_release(&buf);
> > }
> >
> > +static bool default_skip_duplicate_pmus(void *ps)
> > +{
> > + struct print_state *print_state = ps;
> > +
> > + return !print_state->long_desc;
> > +}
> > +
> > int cmd_list(int argc, const char **argv)
> > {
> > int i, ret = 0;
> > @@ -434,6 +441,7 @@ int cmd_list(int argc, const char **argv)
> > .print_end = default_print_end,
> > .print_event = default_print_event,
> > .print_metric = default_print_metric,
> > + .skip_duplicate_pmus = default_skip_duplicate_pmus,
> > };
> > const char *cputype = NULL;
> > const char *unit_name = NULL;
> > diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> > index 3581710667b0..5073843aca19 100644
> > --- a/tools/perf/util/pmus.c
> > +++ b/tools/perf/util/pmus.c
> > @@ -275,6 +275,50 @@ struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu)
> > return NULL;
> > }
> >
> > +static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
> > +{
> > + bool use_core_pmus = !pmu || pmu->is_core;
> > + int last_pmu_name_len = 0;
> > + unsigned long last_pmu_num = 0;
> > + const char *last_pmu_name = (pmu && pmu->name) ? pmu->name : "";
> > +
> > + if (!pmu) {
> > + pmu_read_sysfs(/*core_only=*/false);
> > + pmu = list_prepare_entry(pmu, &core_pmus, list);
> > + } else
> > + last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &last_pmu_num);
> > +
> > + if (use_core_pmus) {
> > + list_for_each_entry_continue(pmu, &core_pmus, list) {
> > + unsigned long pmu_num = 0;
> > + int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
> > +
> > + if (last_pmu_name_len == pmu_name_len &&
> > + (last_pmu_num + 1 == pmu_num) &&
> > + !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
> > + last_pmu_num++;
> > + continue;
> > + }
> > + return pmu;
> > + }
> > + pmu = NULL;
>
> you assign pmu NULL
>
> > + pmu = list_prepare_entry(pmu, &other_pmus, list);
>
> and then re-assign it. If list_prepare_entry() needs first arg = NULL,
> then can just use NULL explicitly?
Done.
> > + }
> > + list_for_each_entry_continue(pmu, &other_pmus, list) {
> > + unsigned long pmu_num = 0;
> > + int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
> > +
> > + if (last_pmu_name_len == pmu_name_len &&
> > + (last_pmu_num + 1 == pmu_num) &&
> > + !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
> > + last_pmu_num++;
> > + continue;
>
> Can some of this code be factored out from the previous patch? It's
> doing something similar, right?
The previous patch implemented list sorting and a list comparator
whilst this patch is skipping PMUs if they follow the pattern:
uncore_xyz_0
uncore_xyz_1 <- skip
uncore_xyz_2 <- skip
The pmu_name_len_no_suffix is factored out and shared between both
routines. The comparator doesn't maintain state whilst this code does.
So I don't see a way to refactor things further.
Thanks,
Ian
> > + }
> > + return pmu;
> > + }
> > + return NULL;
> > +}
> > +
> > const struct perf_pmu *perf_pmus__pmu_for_pmu_filter(const char *str)
> > {
> > struct perf_pmu *pmu = NULL;
> > @@ -429,10 +473,16 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
> > int printed = 0;
> > int len, j;
> > struct sevent *aliases;
> > + struct perf_pmu *(*scan_fn)(struct perf_pmu *);
> > +
> > + if (print_cb->skip_duplicate_pmus(print_state))
> > + scan_fn = perf_pmus__scan_skip_duplicates;
> > + else
> > + scan_fn = perf_pmus__scan;
> >
> > pmu = NULL;
> > len = 0;
> > - while ((pmu = perf_pmus__scan(pmu)) != NULL) {
> > + while ((pmu = scan_fn(pmu)) != NULL) {
> > list_for_each_entry(event, &pmu->aliases, list)
> > len++;
> > if (pmu->selectable)
> > @@ -445,7 +495,7 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
> > }
> > pmu = NULL;
> > j = 0;
> > - while ((pmu = perf_pmus__scan(pmu)) != NULL) {
> > + while ((pmu = scan_fn(pmu)) != NULL) {
> > bool is_cpu = pmu->is_core;
> >
> > list_for_each_entry(event, &pmu->aliases, list) {
> > diff --git a/tools/perf/util/print-events.h b/tools/perf/util/print-events.h
> > index d7fab411e75c..bf4290bef0cd 100644
> > --- a/tools/perf/util/print-events.h
> > +++ b/tools/perf/util/print-events.h
> > @@ -26,6 +26,7 @@ struct print_callbacks {
> > const char *expr,
> > const char *threshold,
> > const char *unit);
> > + bool (*skip_duplicate_pmus)(void *print_state);
> > };
> >
> > /** Print all events, the default when no options are specified. */
>
On Mon, Aug 14, 2023 at 8:57 AM Ian Rogers <irogers@google.com> wrote:
>
> On Fri, Aug 11, 2023 at 8:51 AM John Garry <john.g.garry@oracle.com> wrote:
> >
> > On 10/08/2023 22:49, Ian Rogers wrote:
> > > When there are multiple PMUs that differ only by suffix, by default
> > > just list the first one and skip all others. As the PMUs are sorted,
> > > the scan routine checks that the PMU names match and the numbers are
> > > consecutive. If "-v" is passed to "perf list" then list all PMUs.
> >
> > I really think that this should be merged with the next change. I don't
> > like the intermediate step of by default only printing the first PMU.
>
> Ack. I'll leave it as 3 patches and then leave it to Arnaldo squash as
> quite often he wants more patches.
>
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > > tools/perf/builtin-list.c | 8 +++++
> > > tools/perf/util/pmus.c | 54 ++++++++++++++++++++++++++++++++--
> > > tools/perf/util/print-events.h | 1 +
> > > 3 files changed, 61 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> > > index 7fec2cca759f..8fe4ddf02c14 100644
> > > --- a/tools/perf/builtin-list.c
> > > +++ b/tools/perf/builtin-list.c
> > > @@ -423,6 +423,13 @@ static void json_print_metric(void *ps __maybe_unused, const char *group,
> > > strbuf_release(&buf);
> > > }
> > >
> > > +static bool default_skip_duplicate_pmus(void *ps)
> > > +{
> > > + struct print_state *print_state = ps;
> > > +
> > > + return !print_state->long_desc;
> > > +}
> > > +
> > > int cmd_list(int argc, const char **argv)
> > > {
> > > int i, ret = 0;
> > > @@ -434,6 +441,7 @@ int cmd_list(int argc, const char **argv)
> > > .print_end = default_print_end,
> > > .print_event = default_print_event,
> > > .print_metric = default_print_metric,
> > > + .skip_duplicate_pmus = default_skip_duplicate_pmus,
> > > };
> > > const char *cputype = NULL;
> > > const char *unit_name = NULL;
> > > diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> > > index 3581710667b0..5073843aca19 100644
> > > --- a/tools/perf/util/pmus.c
> > > +++ b/tools/perf/util/pmus.c
> > > @@ -275,6 +275,50 @@ struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu)
> > > return NULL;
> > > }
> > >
> > > +static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
> > > +{
> > > + bool use_core_pmus = !pmu || pmu->is_core;
> > > + int last_pmu_name_len = 0;
> > > + unsigned long last_pmu_num = 0;
> > > + const char *last_pmu_name = (pmu && pmu->name) ? pmu->name : "";
> > > +
> > > + if (!pmu) {
> > > + pmu_read_sysfs(/*core_only=*/false);
> > > + pmu = list_prepare_entry(pmu, &core_pmus, list);
> > > + } else
> > > + last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &last_pmu_num);
> > > +
> > > + if (use_core_pmus) {
> > > + list_for_each_entry_continue(pmu, &core_pmus, list) {
> > > + unsigned long pmu_num = 0;
> > > + int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
> > > +
> > > + if (last_pmu_name_len == pmu_name_len &&
> > > + (last_pmu_num + 1 == pmu_num) &&
> > > + !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
> > > + last_pmu_num++;
> > > + continue;
> > > + }
> > > + return pmu;
> > > + }
> > > + pmu = NULL;
> >
> > you assign pmu NULL
> >
> > > + pmu = list_prepare_entry(pmu, &other_pmus, list);
> >
> > and then re-assign it. If list_prepare_entry() needs first arg = NULL,
> > then can just use NULL explicitly?
>
> Done.
So because of the macro magic in list_prepare_entry you can't
explicitly pass NULL here as doing so yields:
tools/include/linux/kernel.h:36:33: error: request for member ‘list’
in something not a structure or union
36 | const typeof(((type *)0)->member) * __mptr = (ptr); \
| ^~
Thanks,
Ian
> > > + }
> > > + list_for_each_entry_continue(pmu, &other_pmus, list) {
> > > + unsigned long pmu_num = 0;
> > > + int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
> > > +
> > > + if (last_pmu_name_len == pmu_name_len &&
> > > + (last_pmu_num + 1 == pmu_num) &&
> > > + !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
> > > + last_pmu_num++;
> > > + continue;
> >
> > Can some of this code be factored out from the previous patch? It's
> > doing something similar, right?
>
> The previous patch implemented list sorting and a list comparator
> whilst this patch is skipping PMUs if they follow the pattern:
> uncore_xyz_0
> uncore_xyz_1 <- skip
> uncore_xyz_2 <- skip
> The pmu_name_len_no_suffix is factored out and shared between both
> routines. The comparator doesn't maintain state whilst this code does.
> So I don't see a way to refactor things further.
>
> Thanks,
> Ian
>
> > > + }
> > > + return pmu;
> > > + }
> > > + return NULL;
> > > +}
> > > +
> > > const struct perf_pmu *perf_pmus__pmu_for_pmu_filter(const char *str)
> > > {
> > > struct perf_pmu *pmu = NULL;
> > > @@ -429,10 +473,16 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
> > > int printed = 0;
> > > int len, j;
> > > struct sevent *aliases;
> > > + struct perf_pmu *(*scan_fn)(struct perf_pmu *);
> > > +
> > > + if (print_cb->skip_duplicate_pmus(print_state))
> > > + scan_fn = perf_pmus__scan_skip_duplicates;
> > > + else
> > > + scan_fn = perf_pmus__scan;
> > >
> > > pmu = NULL;
> > > len = 0;
> > > - while ((pmu = perf_pmus__scan(pmu)) != NULL) {
> > > + while ((pmu = scan_fn(pmu)) != NULL) {
> > > list_for_each_entry(event, &pmu->aliases, list)
> > > len++;
> > > if (pmu->selectable)
> > > @@ -445,7 +495,7 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
> > > }
> > > pmu = NULL;
> > > j = 0;
> > > - while ((pmu = perf_pmus__scan(pmu)) != NULL) {
> > > + while ((pmu = scan_fn(pmu)) != NULL) {
> > > bool is_cpu = pmu->is_core;
> > >
> > > list_for_each_entry(event, &pmu->aliases, list) {
> > > diff --git a/tools/perf/util/print-events.h b/tools/perf/util/print-events.h
> > > index d7fab411e75c..bf4290bef0cd 100644
> > > --- a/tools/perf/util/print-events.h
> > > +++ b/tools/perf/util/print-events.h
> > > @@ -26,6 +26,7 @@ struct print_callbacks {
> > > const char *expr,
> > > const char *threshold,
> > > const char *unit);
> > > + bool (*skip_duplicate_pmus)(void *print_state);
> > > };
> > >
> > > /** Print all events, the default when no options are specified. */
> >
On 14/08/2023 17:09, Ian Rogers wrote:
> On Mon, Aug 14, 2023 at 8:57 AM Ian Rogers<irogers@google.com> wrote:
>> On Fri, Aug 11, 2023 at 8:51 AM John Garry<john.g.garry@oracle.com> wrote:
>>> On 10/08/2023 22:49, Ian Rogers wrote:
>>>> When there are multiple PMUs that differ only by suffix, by default
>>>> just list the first one and skip all others. As the PMUs are sorted,
>>>> the scan routine checks that the PMU names match and the numbers are
>>>> consecutive. If "-v" is passed to "perf list" then list all PMUs.
>>> I really think that this should be merged with the next change. I don't
>>> like the intermediate step of by default only printing the first PMU.
>> Ack. I'll leave it as 3 patches and then leave it to Arnaldo squash as
>> quite often he wants more patches.
Why are more patches desirable? I do like the approach of taking a bite
at a time, but we should also maintain ability to easily bisect and keep
logical steps as one.
>>
>>>> Signed-off-by: Ian Rogers<irogers@google.com>
>>>> ---
>>>> tools/perf/builtin-list.c | 8 +++++
>>>> tools/perf/util/pmus.c | 54 ++++++++++++++++++++++++++++++++--
>>>> tools/perf/util/print-events.h | 1 +
>>>> 3 files changed, 61 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
>>>> index 7fec2cca759f..8fe4ddf02c14 100644
>>>> --- a/tools/perf/builtin-list.c
>>>> +++ b/tools/perf/builtin-list.c
>>>> @@ -423,6 +423,13 @@ static void json_print_metric(void *ps __maybe_unused, const char *group,
>>>> strbuf_release(&buf);
>>>> }
>>>>
>>>> +static bool default_skip_duplicate_pmus(void *ps)
>>>> +{
>>>> + struct print_state *print_state = ps;
>>>> +
>>>> + return !print_state->long_desc;
>>>> +}
>>>> +
>>>> int cmd_list(int argc, const char **argv)
>>>> {
>>>> int i, ret = 0;
>>>> @@ -434,6 +441,7 @@ int cmd_list(int argc, const char **argv)
>>>> .print_end = default_print_end,
>>>> .print_event = default_print_event,
>>>> .print_metric = default_print_metric,
>>>> + .skip_duplicate_pmus = default_skip_duplicate_pmus,
>>>> };
>>>> const char *cputype = NULL;
>>>> const char *unit_name = NULL;
>>>> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
>>>> index 3581710667b0..5073843aca19 100644
>>>> --- a/tools/perf/util/pmus.c
>>>> +++ b/tools/perf/util/pmus.c
>>>> @@ -275,6 +275,50 @@ struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu)
>>>> return NULL;
>>>> }
>>>>
>>>> +static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
>>>> +{
>>>> + bool use_core_pmus = !pmu || pmu->is_core;
>>>> + int last_pmu_name_len = 0;
>>>> + unsigned long last_pmu_num = 0;
>>>> + const char *last_pmu_name = (pmu && pmu->name) ? pmu->name : "";
>>>> +
>>>> + if (!pmu) {
>>>> + pmu_read_sysfs(/*core_only=*/false);
>>>> + pmu = list_prepare_entry(pmu, &core_pmus, list);
>>>> + } else
>>>> + last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &last_pmu_num);
>>>> +
>>>> + if (use_core_pmus) {
>>>> + list_for_each_entry_continue(pmu, &core_pmus, list) {
>>>> + unsigned long pmu_num = 0;
>>>> + int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
>>>> +
>>>> + if (last_pmu_name_len == pmu_name_len &&
>>>> + (last_pmu_num + 1 == pmu_num) &&
>>>> + !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
>>>> + last_pmu_num++;
>>>> + continue;
>>>> + }
>>>> + return pmu;
>>>> + }
>>>> + pmu = NULL;
>>> you assign pmu NULL
>>>
>>>> + pmu = list_prepare_entry(pmu, &other_pmus, list);
>>> and then re-assign it. If list_prepare_entry() needs first arg = NULL,
>>> then can just use NULL explicitly?
>> Done.
> So because of the macro magic in list_prepare_entry you can't
> explicitly pass NULL here as doing so yields:
> tools/include/linux/kernel.h:36:33: error: request for member ‘list’
> in something not a structure or union
> 36 | const typeof(((type *)0)->member) * __mptr = (ptr); \
> | ^~
ok, fine, so maybe add a comment as the code looks odd..
Thanks,
John
© 2016 - 2026 Red Hat, Inc.