Commit 0e0ae8742207 ("perf list: Display hybrid PMU events with cpu type")
changed the list for uncore PMUs, such that duplicate aliases are now
listed per PMU (which they should not be), like:
./perf list
...
unc_cbo_cache_lookup.any_es
[Unit: uncore_cbox L3 Lookup any request that access cache and found
line in E or S-state]
unc_cbo_cache_lookup.any_es
[Unit: uncore_cbox L3 Lookup any request that access cache and found
line in E or S-state]
unc_cbo_cache_lookup.any_i
[Unit: uncore_cbox L3 Lookup any request that access cache and found
line in I-state]
unc_cbo_cache_lookup.any_i
[Unit: uncore_cbox L3 Lookup any request that access cache and found
line in I-state]
...
Notice how the events are listed twice.
The named commit changed how we remove duplicate events, in that events
for different PMUs are not treated as duplicates. I suppose this is to
handle how "Each hybrid pmu event has been assigned with a pmu name".
Fix uncore PMU alias list by also checking if events with PMU name are not
cpu PMUs.
Fixes: 0e0ae8742207 ("perf list: Display hybrid PMU events with cpu type")
Signed-off-by: John Garry <[email protected]>
---
It would be helpful if someone with some of these hybrid CPU systems could
test this change, thanks!
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 6ae58406f4fc..392f6a36418b 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1659,6 +1659,24 @@ bool is_pmu_core(const char *name)
return !strcmp(name, "cpu") || is_arm_pmu_core(name);
}
+static bool pmu_alias_is_duplicate(struct sevent *alias_a,
+ struct sevent *alias_b)
+{
+ /* Different names -> never duplicates */
+ if (strcmp(alias_a->name, alias_b->name))
+ return false;
+ if (!alias_a->pmu)
+ return true;
+ if (!alias_b->pmu)
+ return true;
+ if (!strcmp(alias_a->pmu, alias_b->pmu))
+ return true;
+ /* uncore PMUs */
+ if (!alias_a->is_cpu && !alias_b->is_cpu)
+ return true;
+ return false;
+}
+
void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
bool long_desc, bool details_flag, bool deprecated,
const char *pmu_name)
@@ -1744,12 +1762,8 @@ void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
qsort(aliases, len, sizeof(struct sevent), cmp_sevent);
for (j = 0; j < len; j++) {
/* Skip duplicates */
- if (j > 0 && !strcmp(aliases[j].name, aliases[j - 1].name)) {
- if (!aliases[j].pmu || !aliases[j - 1].pmu ||
- !strcmp(aliases[j].pmu, aliases[j - 1].pmu)) {
- continue;
- }
- }
+ if (j > 0 && pmu_alias_is_duplicate(&aliases[j], &aliases[j - 1]))
+ continue;
if (name_only) {
printf("%s ", aliases[j].name);
--
2.26.2
Em Thu, Dec 16, 2021 at 11:53:37PM +0800, John Garry escreveu:
> Commit 0e0ae8742207 ("perf list: Display hybrid PMU events with cpu type")
> changed the list for uncore PMUs, such that duplicate aliases are now
> listed per PMU (which they should not be), like:
>
> ./perf list
> ...
> unc_cbo_cache_lookup.any_es
> [Unit: uncore_cbox L3 Lookup any request that access cache and found
> line in E or S-state]
> unc_cbo_cache_lookup.any_es
> [Unit: uncore_cbox L3 Lookup any request that access cache and found
> line in E or S-state]
> unc_cbo_cache_lookup.any_i
> [Unit: uncore_cbox L3 Lookup any request that access cache and found
> line in I-state]
> unc_cbo_cache_lookup.any_i
> [Unit: uncore_cbox L3 Lookup any request that access cache and found
> line in I-state]
> ...
>
> Notice how the events are listed twice.
Hi Jin,
Can I have your acked-by/tested-by?
- Arnaldo
> The named commit changed how we remove duplicate events, in that events
> for different PMUs are not treated as duplicates. I suppose this is to
> handle how "Each hybrid pmu event has been assigned with a pmu name".
>
> Fix uncore PMU alias list by also checking if events with PMU name are not
> cpu PMUs.
>
> Fixes: 0e0ae8742207 ("perf list: Display hybrid PMU events with cpu type")
> Signed-off-by: John Garry <[email protected]>
> ---
> It would be helpful if someone with some of these hybrid CPU systems could
> test this change, thanks!
>
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 6ae58406f4fc..392f6a36418b 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1659,6 +1659,24 @@ bool is_pmu_core(const char *name)
> return !strcmp(name, "cpu") || is_arm_pmu_core(name);
> }
>
> +static bool pmu_alias_is_duplicate(struct sevent *alias_a,
> + struct sevent *alias_b)
> +{
> + /* Different names -> never duplicates */
> + if (strcmp(alias_a->name, alias_b->name))
> + return false;
> + if (!alias_a->pmu)
> + return true;
> + if (!alias_b->pmu)
> + return true;
> + if (!strcmp(alias_a->pmu, alias_b->pmu))
> + return true;
> + /* uncore PMUs */
> + if (!alias_a->is_cpu && !alias_b->is_cpu)
> + return true;
> + return false;
> +}
> +
> void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
> bool long_desc, bool details_flag, bool deprecated,
> const char *pmu_name)
> @@ -1744,12 +1762,8 @@ void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
> qsort(aliases, len, sizeof(struct sevent), cmp_sevent);
> for (j = 0; j < len; j++) {
> /* Skip duplicates */
> - if (j > 0 && !strcmp(aliases[j].name, aliases[j - 1].name)) {
> - if (!aliases[j].pmu || !aliases[j - 1].pmu ||
> - !strcmp(aliases[j].pmu, aliases[j - 1].pmu)) {
> - continue;
> - }
> - }
> + if (j > 0 && pmu_alias_is_duplicate(&aliases[j], &aliases[j - 1]))
> + continue;
>
> if (name_only) {
> printf("%s ", aliases[j].name);
> --
> 2.26.2
--
- Arnaldo
On 18/12/2021 01:47, Arnaldo Carvalho de Melo wrote:
>> Commit 0e0ae8742207 ("perf list: Display hybrid PMU events with cpu type")
>> changed the list for uncore PMUs, such that duplicate aliases are now
>> listed per PMU (which they should not be), like:
>>
>> ./perf list
>> ...
>> unc_cbo_cache_lookup.any_es
>> [Unit: uncore_cbox L3 Lookup any request that access cache and found
>> line in E or S-state]
>> unc_cbo_cache_lookup.any_es
>> [Unit: uncore_cbox L3 Lookup any request that access cache and found
>> line in E or S-state]
>> unc_cbo_cache_lookup.any_i
>> [Unit: uncore_cbox L3 Lookup any request that access cache and found
>> line in I-state]
>> unc_cbo_cache_lookup.any_i
>> [Unit: uncore_cbox L3 Lookup any request that access cache and found
>> line in I-state]
>> ...
>>
>> Notice how the events are listed twice.
> Hi Jin,
>
> Can I have your acked-by/tested-by?
Hi Arnaldo,
I assume that address is bouncing for you also.
So if anyone else has one of these hybrid PMU x86 systems then it would
be appreciated to check this change.
Thanks!
On 12/20/2021 3:38 AM, John Garry wrote:
> On 18/12/2021 01:47, Arnaldo Carvalho de Melo wrote:
>>> Commit 0e0ae8742207 ("perf list: Display hybrid PMU events with cpu
>>> type")
>>> changed the list for uncore PMUs, such that duplicate aliases are now
>>> listed per PMU (which they should not be), like:
>>>
>>> ./perf list
>>> ...
>>> unc_cbo_cache_lookup.any_es
>>> [Unit: uncore_cbox L3 Lookup any request that access cache and found
>>> line in E or S-state]
>>> unc_cbo_cache_lookup.any_es
>>> [Unit: uncore_cbox L3 Lookup any request that access cache and found
>>> line in E or S-state]
>>> unc_cbo_cache_lookup.any_i
>>> [Unit: uncore_cbox L3 Lookup any request that access cache and found
>>> line in I-state]
>>> unc_cbo_cache_lookup.any_i
>>> [Unit: uncore_cbox L3 Lookup any request that access cache and found
>>> line in I-state]
>>> ...
>>>
>>> Notice how the events are listed twice.
>> Hi Jin,
>>
>> Can I have your acked-by/tested-by?
>
> Hi Arnaldo,
>
> I assume that address is bouncing for you also.
>
Yes, Jin Yao has left Intel.
> So if anyone else has one of these hybrid PMU x86 systems then it would
> be appreciated to check this change.
>
+ Zhengjun
Zhengjun,
Could you please help to verify the change?
Thanks,
Kan
On 12/21/2021 12:34 AM, Liang, Kan wrote:
>
>
> On 12/20/2021 3:38 AM, John Garry wrote:
>> On 18/12/2021 01:47, Arnaldo Carvalho de Melo wrote:
>>>> Commit 0e0ae8742207 ("perf list: Display hybrid PMU events with cpu
>>>> type")
>>>> changed the list for uncore PMUs, such that duplicate aliases are now
>>>> listed per PMU (which they should not be), like:
>>>>
>>>> ./perf list
>>>> ...
>>>> unc_cbo_cache_lookup.any_es
>>>> [Unit: uncore_cbox L3 Lookup any request that access cache and found
>>>> line in E or S-state]
>>>> unc_cbo_cache_lookup.any_es
>>>> [Unit: uncore_cbox L3 Lookup any request that access cache and found
>>>> line in E or S-state]
>>>> unc_cbo_cache_lookup.any_i
>>>> [Unit: uncore_cbox L3 Lookup any request that access cache and found
>>>> line in I-state]
>>>> unc_cbo_cache_lookup.any_i
>>>> [Unit: uncore_cbox L3 Lookup any request that access cache and found
>>>> line in I-state]
>>>> ...
>>>>
>>>> Notice how the events are listed twice.
>>> Hi Jin,
>>>
>>> Can I have your acked-by/tested-by?
>>
>> Hi Arnaldo,
>>
>> I assume that address is bouncing for you also.
>>
>
> Yes, Jin Yao has left Intel.
>
>> So if anyone else has one of these hybrid PMU x86 systems then it
>> would be appreciated to check this change.
>>
>
> + Zhengjun
>
> Zhengjun,
>
> Could you please help to verify the change?
>
> Thanks,
> Kan
Tested this patch on one hybrid PMU x86 system, it works OK.
Tested-by: Zhengjun Xing <[email protected]>
--
Zhengjun Xing
On Thu, Dec 16, 2021 at 11:53:37PM +0800, John Garry wrote:
> Commit 0e0ae8742207 ("perf list: Display hybrid PMU events with cpu type")
> changed the list for uncore PMUs, such that duplicate aliases are now
> listed per PMU (which they should not be), like:
>
> ./perf list
> ...
> unc_cbo_cache_lookup.any_es
> [Unit: uncore_cbox L3 Lookup any request that access cache and found
> line in E or S-state]
> unc_cbo_cache_lookup.any_es
> [Unit: uncore_cbox L3 Lookup any request that access cache and found
> line in E or S-state]
> unc_cbo_cache_lookup.any_i
> [Unit: uncore_cbox L3 Lookup any request that access cache and found
> line in I-state]
> unc_cbo_cache_lookup.any_i
> [Unit: uncore_cbox L3 Lookup any request that access cache and found
> line in I-state]
> ...
>
> Notice how the events are listed twice.
>
> The named commit changed how we remove duplicate events, in that events
> for different PMUs are not treated as duplicates. I suppose this is to
> handle how "Each hybrid pmu event has been assigned with a pmu name".
>
> Fix uncore PMU alias list by also checking if events with PMU name are not
> cpu PMUs.
>
> Fixes: 0e0ae8742207 ("perf list: Display hybrid PMU events with cpu type")
> Signed-off-by: John Garry <[email protected]>
> ---
> It would be helpful if someone with some of these hybrid CPU systems could
> test this change, thanks!
>
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 6ae58406f4fc..392f6a36418b 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1659,6 +1659,24 @@ bool is_pmu_core(const char *name)
> return !strcmp(name, "cpu") || is_arm_pmu_core(name);
> }
>
> +static bool pmu_alias_is_duplicate(struct sevent *alias_a,
> + struct sevent *alias_b)
> +{
> + /* Different names -> never duplicates */
> + if (strcmp(alias_a->name, alias_b->name))
> + return false;
> + if (!alias_a->pmu)
> + return true;
> + if (!alias_b->pmu)
> + return true;
nit could be:
if (!alias_a->pmu || !alias_b->pmu)
return true;
would be great to have more comments explaining the check
thanks,
jirka
> + if (!strcmp(alias_a->pmu, alias_b->pmu))
> + return true;
> + /* uncore PMUs */
> + if (!alias_a->is_cpu && !alias_b->is_cpu)
> + return true;
> + return false;
> +}
> +
> void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
> bool long_desc, bool details_flag, bool deprecated,
> const char *pmu_name)
> @@ -1744,12 +1762,8 @@ void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
> qsort(aliases, len, sizeof(struct sevent), cmp_sevent);
> for (j = 0; j < len; j++) {
> /* Skip duplicates */
> - if (j > 0 && !strcmp(aliases[j].name, aliases[j - 1].name)) {
> - if (!aliases[j].pmu || !aliases[j - 1].pmu ||
> - !strcmp(aliases[j].pmu, aliases[j - 1].pmu)) {
> - continue;
> - }
> - }
> + if (j > 0 && pmu_alias_is_duplicate(&aliases[j], &aliases[j - 1]))
> + continue;
>
> if (name_only) {
> printf("%s ", aliases[j].name);
> --
> 2.26.2
>
On 21/12/2021 07:58, Jiri Olsa wrote:
>> + /* Different names -> never duplicates */
>> + if (strcmp(alias_a->name, alias_b->name))
>> + return false;
>> + if (!alias_a->pmu)
>> + return true;
>> + if (!alias_b->pmu)
>> + return true;
> nit could be:
>
> if (!alias_a->pmu || !alias_b->pmu)
> return true;
>
> would be great to have more comments explaining the check
>
This is just a sanity check that both strings are non-NULL as we do a
strcmp() next. So would this be better:
if (!alias_a->pmu || !alias_b->pmu || !strcmp(alias_a->pmu, alias_b->pmu))
return true
?
It will spill a line.
Thanks,
John
> thanks,
> jirka
>
>> + if (!strcmp(alias_a->pmu, alias_b->pmu))
>> + return true;
>> + /* uncore PMUs */
On Tue, Dec 21, 2021 at 09:10:37AM +0000, John Garry wrote:
> On 21/12/2021 07:58, Jiri Olsa wrote:
> > > + /* Different names -> never duplicates */
> > > + if (strcmp(alias_a->name, alias_b->name))
> > > + return false;
> > > + if (!alias_a->pmu)
> > > + return true;
> > > + if (!alias_b->pmu)
> > > + return true;
> > nit could be:
> >
> > if (!alias_a->pmu || !alias_b->pmu)
> > return true;
> >
> > would be great to have more comments explaining the check
> >
>
> This is just a sanity check that both strings are non-NULL as we do a
> strcmp() next. So would this be better:
>
> if (!alias_a->pmu || !alias_b->pmu || !strcmp(alias_a->pmu, alias_b->pmu))
> return true
>
> ?
>
> It will spill a line.
sure, it cought my eye because the is_cpu check later is done on
the same line, so I started wondering what's the difference ;-)
jirka
>
> Thanks,
> John
>
> > thanks,
> > jirka
> >
> > > + if (!strcmp(alias_a->pmu, alias_b->pmu))
> > > + return true;
> > > + /* uncore PMUs */
>
>
On 21/12/2021 09:35, Jiri Olsa wrote:
> On Tue, Dec 21, 2021 at 09:10:37AM +0000, John Garry wrote:
>> On 21/12/2021 07:58, Jiri Olsa wrote:
>>>> + /* Different names -> never duplicates */
>>>> + if (strcmp(alias_a->name, alias_b->name))
>>>> + return false;
>>>> + if (!alias_a->pmu)
>>>> + return true;
>>>> + if (!alias_b->pmu)
>>>> + return true;
>>> nit could be:
>>>
>>> if (!alias_a->pmu || !alias_b->pmu)
>>> return true;
>>>
>>> would be great to have more comments explaining the check
>>>
>>
>> This is just a sanity check that both strings are non-NULL as we do a
>> strcmp() next. So would this be better:
>>
>> if (!alias_a->pmu || !alias_b->pmu || !strcmp(alias_a->pmu, alias_b->pmu))
>> return true
>>
>> ?
>>
>> It will spill a line.
>
> sure, it cought my eye because the is_cpu check later is done on
> the same line, so I started wondering what's the difference ;-)
>
Now thinking a bit more I am not confident that this patch is a full fix.
arm have heterogeneous CPU systems as well - which are not "hybrid" -
and I need to ensure that aliasing is still working properly there, as I
think that this following check would stop removing duplicates there:
+ /* uncore PMUs */
+ if (!alias_a->is_cpu && !alias_b->is_cpu)
+ return true;
+ return false;
Thanks,
John
Em Tue, Dec 21, 2021 at 10:14:42AM +0000, John Garry escreveu:
> On 21/12/2021 09:35, Jiri Olsa wrote:
> > On Tue, Dec 21, 2021 at 09:10:37AM +0000, John Garry wrote:
> > > On 21/12/2021 07:58, Jiri Olsa wrote:
> > > > > + /* Different names -> never duplicates */
> > > > > + if (strcmp(alias_a->name, alias_b->name))
> > > > > + return false;
> > > > > + if (!alias_a->pmu)
> > > > > + return true;
> > > > > + if (!alias_b->pmu)
> > > > > + return true;
> > > > nit could be:
> > > >
> > > > if (!alias_a->pmu || !alias_b->pmu)
> > > > return true;
> > > >
> > > > would be great to have more comments explaining the check
> > > >
> > >
> > > This is just a sanity check that both strings are non-NULL as we do a
> > > strcmp() next. So would this be better:
> > >
> > > if (!alias_a->pmu || !alias_b->pmu || !strcmp(alias_a->pmu, alias_b->pmu))
> > > return true
> > >
> > > ?
> > >
> > > It will spill a line.
> >
> > sure, it cought my eye because the is_cpu check later is done on
> > the same line, so I started wondering what's the difference ;-)
> >
>
> Now thinking a bit more I am not confident that this patch is a full fix.
>
> arm have heterogeneous CPU systems as well - which are not "hybrid" - and I
> need to ensure that aliasing is still working properly there, as I think
> that this following check would stop removing duplicates there:
>
> + /* uncore PMUs */
> + if (!alias_a->is_cpu && !alias_b->is_cpu)
> + return true;
> + return false;
I was about to process this, do you think its better to revert the
original patch while this gets fixed?
- Arnaldo
On 21/12/2021 20:48, Arnaldo Carvalho de Melo wrote:
>> Now thinking a bit more I am not confident that this patch is a full fix.
>>
>> arm have heterogeneous CPU systems as well - which are not "hybrid" - and I
>> need to ensure that aliasing is still working properly there, as I think
>> that this following check would stop removing duplicates there:
>>
>> + /* uncore PMUs */
>> + if (!alias_a->is_cpu && !alias_b->is_cpu)
>> + return true;
>> + return false;
> I was about to process this, do you think its better to revert the
> original patch while this gets fixed?
I think that the v2 should now be ok.
@jirka, can you kindly help to check that?
BTW, my patch is based on v5.16-rc5 . I assumed that I would need to be
based on acme/perf/urgent, but that seems to be based on 5.15
Thanks,
John