Commit 7afbf90ea2e2 ("perf pmu: Don't de-duplicate core PMUs") fixed a
display mismatch related to deduplication within a single PMU, but it
didn't fix the case where deduplicated PMUs aren't listed at all.
Fix it by using the same function which takes is_core into account,
except in the use_core_pmus block where it's always going to be true.
Before this change, -v would be required to get the same behavior for
core PMUs. Now it's no longer required:
Before:
$ perf list | grep br_indirect_spec -A 1
br_indirect_spec
[Branch speculatively executed,indirect branch. Unit: armv8_cortex_a53]
After:
$ perf list | grep br_indirect_spec -A 2
[Branch speculatively executed,indirect branch. Unit: armv8_cortex_a53,
armv8_cortex_a57]
Signed-off-by: James Clark <james.clark@linaro.org>
---
tools/perf/util/pmu.c | 5 +++--
tools/perf/util/pmu.h | 2 ++
tools/perf/util/pmus.c | 8 +++++---
3 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 57450c73fb63..caff0d309012 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -834,9 +834,10 @@ static int is_sysfs_pmu_core(const char *name)
*
* @skip_duplicate_pmus: False in verbose mode so all uncore PMUs are visible
*/
-static size_t pmu_deduped_name_len(const struct perf_pmu *pmu, const char *name,
- bool skip_duplicate_pmus)
+size_t pmu_deduped_name_len(const struct perf_pmu *pmu, const char *name,
+ bool skip_duplicate_pmus)
{
+ name = name ?: "";
return skip_duplicate_pmus && !pmu->is_core
? pmu_name_len_no_suffix(name)
: strlen(name);
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index b93014cc3670..ce6a394a695d 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -297,5 +297,7 @@ struct perf_pmu *perf_pmus__find_core_pmu(void);
const char *perf_pmu__name_from_config(struct perf_pmu *pmu, u64 config);
bool perf_pmu__is_fake(const struct perf_pmu *pmu);
+size_t pmu_deduped_name_len(const struct perf_pmu *pmu, const char *name,
+ bool skip_duplicate_pmus);
#endif /* __PMU_H */
diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
index cb1b14ade25b..1acc27af4d02 100644
--- a/tools/perf/util/pmus.c
+++ b/tools/perf/util/pmus.c
@@ -358,12 +358,14 @@ static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
if (!pmu) {
pmu_read_sysfs(PERF_TOOL_PMU_TYPE_ALL_MASK);
pmu = list_prepare_entry(pmu, &core_pmus, list);
- } else
- last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "");
+ } else {
+ last_pmu_name_len = pmu_deduped_name_len(pmu, pmu->name,
+ /*skip_duplicate_pmus=*/true);
+ }
if (use_core_pmus) {
list_for_each_entry_continue(pmu, &core_pmus, list) {
- int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "");
+ int pmu_name_len = strlen(pmu->name ?: "");
if (last_pmu_name_len == pmu_name_len &&
!strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len))
--
2.34.1On Tue, Mar 4, 2025 at 5:50 AM James Clark <james.clark@linaro.org> wrote:
>
> Commit 7afbf90ea2e2 ("perf pmu: Don't de-duplicate core PMUs") fixed a
> display mismatch related to deduplication within a single PMU, but it
> didn't fix the case where deduplicated PMUs aren't listed at all.
>
> Fix it by using the same function which takes is_core into account,
> except in the use_core_pmus block where it's always going to be true.
> Before this change, -v would be required to get the same behavior for
> core PMUs. Now it's no longer required:
>
> Before:
> $ perf list | grep br_indirect_spec -A 1
> br_indirect_spec
> [Branch speculatively executed,indirect branch. Unit: armv8_cortex_a53]
>
> After:
> $ perf list | grep br_indirect_spec -A 2
> [Branch speculatively executed,indirect branch. Unit: armv8_cortex_a53,
> armv8_cortex_a57]
>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
> tools/perf/util/pmu.c | 5 +++--
> tools/perf/util/pmu.h | 2 ++
> tools/perf/util/pmus.c | 8 +++++---
> 3 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 57450c73fb63..caff0d309012 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -834,9 +834,10 @@ static int is_sysfs_pmu_core(const char *name)
> *
> * @skip_duplicate_pmus: False in verbose mode so all uncore PMUs are visible
> */
> -static size_t pmu_deduped_name_len(const struct perf_pmu *pmu, const char *name,
> - bool skip_duplicate_pmus)
> +size_t pmu_deduped_name_len(const struct perf_pmu *pmu, const char *name,
> + bool skip_duplicate_pmus)
nit: I think the name should be perf_pmu__deduped_name_len for
consistency with the other non-static functions.
> {
> + name = name ?: "";
nit: Should this just use pmu->name ?
> return skip_duplicate_pmus && !pmu->is_core
> ? pmu_name_len_no_suffix(name)
> : strlen(name);
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index b93014cc3670..ce6a394a695d 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -297,5 +297,7 @@ struct perf_pmu *perf_pmus__find_core_pmu(void);
>
> const char *perf_pmu__name_from_config(struct perf_pmu *pmu, u64 config);
> bool perf_pmu__is_fake(const struct perf_pmu *pmu);
> +size_t pmu_deduped_name_len(const struct perf_pmu *pmu, const char *name,
> + bool skip_duplicate_pmus);
>
> #endif /* __PMU_H */
> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index cb1b14ade25b..1acc27af4d02 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -358,12 +358,14 @@ static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
> if (!pmu) {
> pmu_read_sysfs(PERF_TOOL_PMU_TYPE_ALL_MASK);
> pmu = list_prepare_entry(pmu, &core_pmus, list);
> - } else
> - last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "");
> + } else {
> + last_pmu_name_len = pmu_deduped_name_len(pmu, pmu->name,
> + /*skip_duplicate_pmus=*/true);
> + }
>
> if (use_core_pmus) {
> list_for_each_entry_continue(pmu, &core_pmus, list) {
> - int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "");
> + int pmu_name_len = strlen(pmu->name ?: "");
>
> if (last_pmu_name_len == pmu_name_len &&
> !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len))
Can this code be removed given there shouldn't be core PMUs with
identical names? ie:
```
if (use_core_pmus) {
list_for_each_entry_continue(pmu, &core_pmus, list)
return pmu;
pmu = NULL;
pmu = list_prepare_entry(pmu, &other_pmus, list);
}
```
Thanks,
Ian
>
> --
> 2.34.1
>
On 05/03/2025 9:51 pm, Ian Rogers wrote:
> On Tue, Mar 4, 2025 at 5:50 AM James Clark <james.clark@linaro.org> wrote:
>>
>> Commit 7afbf90ea2e2 ("perf pmu: Don't de-duplicate core PMUs") fixed a
>> display mismatch related to deduplication within a single PMU, but it
>> didn't fix the case where deduplicated PMUs aren't listed at all.
>>
>> Fix it by using the same function which takes is_core into account,
>> except in the use_core_pmus block where it's always going to be true.
>> Before this change, -v would be required to get the same behavior for
>> core PMUs. Now it's no longer required:
>>
>> Before:
>> $ perf list | grep br_indirect_spec -A 1
>> br_indirect_spec
>> [Branch speculatively executed,indirect branch. Unit: armv8_cortex_a53]
>>
>> After:
>> $ perf list | grep br_indirect_spec -A 2
>> [Branch speculatively executed,indirect branch. Unit: armv8_cortex_a53,
>> armv8_cortex_a57]
>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>> tools/perf/util/pmu.c | 5 +++--
>> tools/perf/util/pmu.h | 2 ++
>> tools/perf/util/pmus.c | 8 +++++---
>> 3 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index 57450c73fb63..caff0d309012 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -834,9 +834,10 @@ static int is_sysfs_pmu_core(const char *name)
>> *
>> * @skip_duplicate_pmus: False in verbose mode so all uncore PMUs are visible
>> */
>> -static size_t pmu_deduped_name_len(const struct perf_pmu *pmu, const char *name,
>> - bool skip_duplicate_pmus)
>> +size_t pmu_deduped_name_len(const struct perf_pmu *pmu, const char *name,
>> + bool skip_duplicate_pmus)
>
> nit: I think the name should be perf_pmu__deduped_name_len for
> consistency with the other non-static functions.
>
Will change.
>> {
>> + name = name ?: "";
>
> nit: Should this just use pmu->name ?
>
I can keep this part at the callsite in
perf_pmus__scan_skip_duplicates() to avoid any confusion about this
function accessing pmu->name or not. The only reason this function takes
a separate name parameter is to allow it to work with struct
pmu_event_info elsewhere as well.
>> return skip_duplicate_pmus && !pmu->is_core
>> ? pmu_name_len_no_suffix(name)
>> : strlen(name);
>> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
>> index b93014cc3670..ce6a394a695d 100644
>> --- a/tools/perf/util/pmu.h
>> +++ b/tools/perf/util/pmu.h
>> @@ -297,5 +297,7 @@ struct perf_pmu *perf_pmus__find_core_pmu(void);
>>
>> const char *perf_pmu__name_from_config(struct perf_pmu *pmu, u64 config);
>> bool perf_pmu__is_fake(const struct perf_pmu *pmu);
>> +size_t pmu_deduped_name_len(const struct perf_pmu *pmu, const char *name,
>> + bool skip_duplicate_pmus);
>>
>> #endif /* __PMU_H */
>> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
>> index cb1b14ade25b..1acc27af4d02 100644
>> --- a/tools/perf/util/pmus.c
>> +++ b/tools/perf/util/pmus.c
>> @@ -358,12 +358,14 @@ static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
>> if (!pmu) {
>> pmu_read_sysfs(PERF_TOOL_PMU_TYPE_ALL_MASK);
>> pmu = list_prepare_entry(pmu, &core_pmus, list);
>> - } else
>> - last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "");
>> + } else {
>> + last_pmu_name_len = pmu_deduped_name_len(pmu, pmu->name,
>> + /*skip_duplicate_pmus=*/true);
>> + }
>>
>> if (use_core_pmus) {
>> list_for_each_entry_continue(pmu, &core_pmus, list) {
>> - int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "");
>> + int pmu_name_len = strlen(pmu->name ?: "");
>>
>> if (last_pmu_name_len == pmu_name_len &&
>> !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len))
>
> Can this code be removed given there shouldn't be core PMUs with
> identical names? ie:
> ```
> if (use_core_pmus) {
> list_for_each_entry_continue(pmu, &core_pmus, list)
> return pmu;
>
> pmu = NULL;
> pmu = list_prepare_entry(pmu, &other_pmus, list);
> }
> ```
>
> Thanks,
> Ian
>
Even better, yes it probably can.
Thanks
James
>>
>> --
>> 2.34.1
>>
© 2016 - 2026 Red Hat, Inc.