Re: [PATCH 3/3] perf list: Don't deduplicate core PMUs when listing events

[James Clark <james.clark@linaro.org>]

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