diff mbox series

perf top: Use evsel's cpus to replace user_requested_cpus

Message ID 20231208210855.407580-1-kan.liang@linux.intel.com
State New
Headers
Series perf top: Use evsel's cpus to replace user_requested_cpus |

Commit Message

Liang, Kan Dec. 8, 2023, 9:08 p.m. UTC 
  From: Kan Liang <kan.liang@linux.intel.com>

perf top errors out on a hybrid machine
 $perf top

 Error:
 The cycles:P event is not supported.

The user_requested_cpus may contain CPUs that are invalid for a hybrid
PMU. It causes perf_event_open to fail.

The commit ef91871c960e ("perf evlist: Propagate user CPU maps
intersecting core PMU maps") already intersect the requested CPU map
with the CPU map of the PMU. Use the evsel's cpus to replace
user_requested_cpus.

The evlist's threads is also propagated to evsel's threads in
__perf_evlist__propagate_maps(). Replace it as well.

Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
Closes: https://lore.kernel.org/linux-perf-users/ZXNnDrGKXbEELMXV@kernel.org/
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
 tools/perf/builtin-top.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Arnaldo Carvalho de Melo Dec. 11, 2023, 9:13 p.m. UTC | #1 
Em Fri, Dec 08, 2023 at 01:08:55PM -0800, kan.liang@linux.intel.com escreveu:
> From: Kan Liang <kan.liang@linux.intel.com>
> 
> perf top errors out on a hybrid machine
>  $perf top
> 
>  Error:
>  The cycles:P event is not supported.
> 
> The user_requested_cpus may contain CPUs that are invalid for a hybrid
> PMU. It causes perf_event_open to fail.

?

All perf top expects is that the "cycles", the most basic one, be
collected, on all CPUs in the system.

> The commit ef91871c960e ("perf evlist: Propagate user CPU maps
> intersecting core PMU maps") already intersect the requested CPU map
> with the CPU map of the PMU. Use the evsel's cpus to replace
> user_requested_cpus.
 
> The evlist's threads is also propagated to evsel's threads in
> __perf_evlist__propagate_maps(). Replace it as well.

Thanks, but please try to add more detail to the fix so as to help
others to consider looking at the patch.

- Arnaldo
 
> Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
> Closes: https://lore.kernel.org/linux-perf-users/ZXNnDrGKXbEELMXV@kernel.org/
> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> ---
>  tools/perf/builtin-top.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index ea8c7eca5eee..cce9350177e2 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1027,8 +1027,8 @@ static int perf_top__start_counters(struct perf_top *top)
>  
>  	evlist__for_each_entry(evlist, counter) {
>  try_again:
> -		if (evsel__open(counter, top->evlist->core.user_requested_cpus,
> -				     top->evlist->core.threads) < 0) {
> +		if (evsel__open(counter, counter->core.cpus,
> +				counter->core.threads) < 0) {
>  
>  			/*
>  			 * Specially handle overwrite fall back.
> -- 
> 2.35.1
>
Ian Rogers Dec. 12, 2023, 12:02 a.m. UTC | #2 
On Fri, Dec 8, 2023 at 1:09 PM <kan.liang@linux.intel.com> wrote:
>
> From: Kan Liang <kan.liang@linux.intel.com>
>
> perf top errors out on a hybrid machine
>  $perf top
>
>  Error:
>  The cycles:P event is not supported.
>
> The user_requested_cpus may contain CPUs that are invalid for a hybrid
> PMU. It causes perf_event_open to fail.
>
> The commit ef91871c960e ("perf evlist: Propagate user CPU maps
> intersecting core PMU maps") already intersect the requested CPU map
> with the CPU map of the PMU. Use the evsel's cpus to replace
> user_requested_cpus.
>
> The evlist's threads is also propagated to evsel's threads in
> __perf_evlist__propagate_maps(). Replace it as well.
>
> Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
> Closes: https://lore.kernel.org/linux-perf-users/ZXNnDrGKXbEELMXV@kernel.org/
> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>

Sorry I missed top doing the evlist__create_maps calls:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/builtin-top.c?h=perf-tools-next#n1761
so it is right that the only divergence from record:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/builtin-record.c?h=perf-tools-next#n1362
and stat:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/stat.c?h=perf-tools-next#n797
So this is the right fix I believe.

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/builtin-top.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index ea8c7eca5eee..cce9350177e2 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1027,8 +1027,8 @@ static int perf_top__start_counters(struct perf_top *top)
>
>         evlist__for_each_entry(evlist, counter) {
>  try_again:
> -               if (evsel__open(counter, top->evlist->core.user_requested_cpus,
> -                                    top->evlist->core.threads) < 0) {
> +               if (evsel__open(counter, counter->core.cpus,
> +                               counter->core.threads) < 0) {
>
>                         /*
>                          * Specially handle overwrite fall back.
> --
> 2.35.1
>
Liang, Kan Dec. 12, 2023, 3:56 p.m. UTC | #3 
On 2023-12-11 4:13 p.m., Arnaldo Carvalho de Melo wrote:
> Em Fri, Dec 08, 2023 at 01:08:55PM -0800, kan.liang@linux.intel.com escreveu:
>> From: Kan Liang <kan.liang@linux.intel.com>
>>
>> perf top errors out on a hybrid machine
>>  $perf top
>>
>>  Error:
>>  The cycles:P event is not supported.
>>
>> The user_requested_cpus may contain CPUs that are invalid for a hybrid
>> PMU. It causes perf_event_open to fail.
> 
> ?
> 
> All perf top expects is that the "cycles", the most basic one, be
> collected, on all CPUs in the system.
>

Yes, but for hybrid there is no single "cycles" event which can cover
all CPUs. Perf has to split it into two cycles events, cpu_core/cycles/
and cpu_atom/cycles/. Each event has its own CPU mask. If a event is
opened on the unsupported CPU. The open fails. That's the reason perf
top fails. So perf should only open the cycles event on the
corresponding CPU.

>> The commit ef91871c960e ("perf evlist: Propagate user CPU maps
>> intersecting core PMU maps") already intersect the requested CPU map
>> with the CPU map of the PMU. Use the evsel's cpus to replace
>> user_requested_cpus.
>  
>> The evlist's threads is also propagated to evsel's threads in
>> __perf_evlist__propagate_maps(). Replace it as well.
> 
> Thanks, but please try to add more detail to the fix so as to help
> others to consider looking at the patch.

OK. For the threads, the same as other tools, e.g., perf record, perf
appends a dummy for the system wide event. For a per-thread event, the
evlist's thread_map is assigned to the evsel. So using the evsel's
threads is appropriate and also be consistent with other tools.

I will update the description and send a V2.

Thanks,
Kan

> 
> - Arnaldo
>  
>> Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
>> Closes: https://lore.kernel.org/linux-perf-users/ZXNnDrGKXbEELMXV@kernel.org/
>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
>> ---
>>  tools/perf/builtin-top.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
>> index ea8c7eca5eee..cce9350177e2 100644
>> --- a/tools/perf/builtin-top.c
>> +++ b/tools/perf/builtin-top.c
>> @@ -1027,8 +1027,8 @@ static int perf_top__start_counters(struct perf_top *top)
>>  
>>  	evlist__for_each_entry(evlist, counter) {
>>  try_again:
>> -		if (evsel__open(counter, top->evlist->core.user_requested_cpus,
>> -				     top->evlist->core.threads) < 0) {
>> +		if (evsel__open(counter, counter->core.cpus,
>> +				counter->core.threads) < 0) {
>>  
>>  			/*
>>  			 * Specially handle overwrite fall back.
>> -- 
>> 2.35.1
>>
>
Arnaldo Carvalho de Melo Dec. 12, 2023, 4:58 p.m. UTC | #4 
Em Tue, Dec 12, 2023 at 10:56:15AM -0500, Liang, Kan escreveu:
> 
> 
> On 2023-12-11 4:13 p.m., Arnaldo Carvalho de Melo wrote:
> > Em Fri, Dec 08, 2023 at 01:08:55PM -0800, kan.liang@linux.intel.com escreveu:
> >> From: Kan Liang <kan.liang@linux.intel.com>
> >>
> >> perf top errors out on a hybrid machine
> >>  $perf top
> >>
> >>  Error:
> >>  The cycles:P event is not supported.
> >>
> >> The user_requested_cpus may contain CPUs that are invalid for a hybrid
> >> PMU. It causes perf_event_open to fail.

> > ?

> > All perf top expects is that the "cycles", the most basic one, be
> > collected, on all CPUs in the system.
 
> Yes, but for hybrid there is no single "cycles" event which can cover
> all CPUs. Perf has to split it into two cycles events, cpu_core/cycles/
> and cpu_atom/cycles/. Each event has its own CPU mask. If a event is
> opened on the unsupported CPU. The open fails. That's the reason perf
> top fails. So perf should only open the cycles event on the
> corresponding CPU.

Great explanation, please make sure it is present in the fix, i.e. in
the v2 you mentioned.
 
> >> The commit ef91871c960e ("perf evlist: Propagate user CPU maps
> >> intersecting core PMU maps") already intersect the requested CPU map
> >> with the CPU map of the PMU. Use the evsel's cpus to replace
> >> user_requested_cpus.
> >  
> >> The evlist's threads is also propagated to evsel's threads in
> >> __perf_evlist__propagate_maps(). Replace it as well.
> > 
> > Thanks, but please try to add more detail to the fix so as to help
> > others to consider looking at the patch.
> 
> OK. For the threads, the same as other tools, e.g., perf record, perf
> appends a dummy for the system wide event. For a per-thread event, the
> evlist's thread_map is assigned to the evsel. So using the evsel's
> threads is appropriate and also be consistent with other tools.
> 
> I will update the description and send a V2.
> 
> Thanks,
> Kan
> 
> > 
> > - Arnaldo
> >  
> >> Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
> >> Closes: https://lore.kernel.org/linux-perf-users/ZXNnDrGKXbEELMXV@kernel.org/
> >> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> >> ---
> >>  tools/perf/builtin-top.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> >> index ea8c7eca5eee..cce9350177e2 100644
> >> --- a/tools/perf/builtin-top.c
> >> +++ b/tools/perf/builtin-top.c
> >> @@ -1027,8 +1027,8 @@ static int perf_top__start_counters(struct perf_top *top)
> >>  
> >>  	evlist__for_each_entry(evlist, counter) {
> >>  try_again:
> >> -		if (evsel__open(counter, top->evlist->core.user_requested_cpus,
> >> -				     top->evlist->core.threads) < 0) {
> >> +		if (evsel__open(counter, counter->core.cpus,
> >> +				counter->core.threads) < 0) {
> >>  
> >>  			/*
> >>  			 * Specially handle overwrite fall back.
Namhyung Kim Dec. 12, 2023, 5:23 p.m. UTC | #5 
On Tue, Dec 12, 2023 at 7:56 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2023-12-11 4:13 p.m., Arnaldo Carvalho de Melo wrote:
> > Em Fri, Dec 08, 2023 at 01:08:55PM -0800, kan.liang@linux.intel.com escreveu:
> >> From: Kan Liang <kan.liang@linux.intel.com>
> >>
> >> perf top errors out on a hybrid machine
> >>  $perf top
> >>
> >>  Error:
> >>  The cycles:P event is not supported.
> >>
> >> The user_requested_cpus may contain CPUs that are invalid for a hybrid
> >> PMU. It causes perf_event_open to fail.
> >
> > ?
> >
> > All perf top expects is that the "cycles", the most basic one, be
> > collected, on all CPUs in the system.
> >
>
> Yes, but for hybrid there is no single "cycles" event which can cover
> all CPUs.

Does that mean the kernel would reject the legacy "cycles" event
on hybrid CPUs?

Thanks,
Namhyung
Ian Rogers Dec. 12, 2023, 6 p.m. UTC | #6 
On Tue, Dec 12, 2023 at 9:23 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Tue, Dec 12, 2023 at 7:56 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
> >
> >
> >
> > On 2023-12-11 4:13 p.m., Arnaldo Carvalho de Melo wrote:
> > > Em Fri, Dec 08, 2023 at 01:08:55PM -0800, kan.liang@linux.intel.com escreveu:
> > >> From: Kan Liang <kan.liang@linux.intel.com>
> > >>
> > >> perf top errors out on a hybrid machine
> > >>  $perf top
> > >>
> > >>  Error:
> > >>  The cycles:P event is not supported.
> > >>
> > >> The user_requested_cpus may contain CPUs that are invalid for a hybrid
> > >> PMU. It causes perf_event_open to fail.
> > >
> > > ?
> > >
> > > All perf top expects is that the "cycles", the most basic one, be
> > > collected, on all CPUs in the system.
> > >
> >
> > Yes, but for hybrid there is no single "cycles" event which can cover
> > all CPUs.
>
> Does that mean the kernel would reject the legacy "cycles" event
> on hybrid CPUs?

I believe not. When the extended type isn't set on legacy cycles we
often have the CPU and from that can determine the PMU. The issue is
with the -1 any CPU perf_event_open option. As I was told, the PMU the
event is opened on in this case is the first one registered in the
kernel, on Intel hybrid this could be cpu_core or cpu_atom.. but IIRC
it'll probably be cpu_core. On ARM ¯\_(ツ)_/¯. In any case you only
have an event that will be enabled on a fraction of the CPU cores the
thread you are monitoring could be scheduled on. This is why 2 or more
events are needed (depending on how many core types you have).

Thanks,
Ian

> Thanks,
> Namhyung
Mark Rutland Dec. 12, 2023, 6:31 p.m. UTC | #7 
On Tue, Dec 12, 2023 at 10:00:16AM -0800, Ian Rogers wrote:
> On Tue, Dec 12, 2023 at 9:23 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Tue, Dec 12, 2023 at 7:56 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
> > >
> > >
> > >
> > > On 2023-12-11 4:13 p.m., Arnaldo Carvalho de Melo wrote:
> > > > Em Fri, Dec 08, 2023 at 01:08:55PM -0800, kan.liang@linux.intel.com escreveu:
> > > >> From: Kan Liang <kan.liang@linux.intel.com>
> > > >>
> > > >> perf top errors out on a hybrid machine
> > > >>  $perf top
> > > >>
> > > >>  Error:
> > > >>  The cycles:P event is not supported.
> > > >>
> > > >> The user_requested_cpus may contain CPUs that are invalid for a hybrid
> > > >> PMU. It causes perf_event_open to fail.
> > > >
> > > > ?
> > > >
> > > > All perf top expects is that the "cycles", the most basic one, be
> > > > collected, on all CPUs in the system.
> > > >
> > >
> > > Yes, but for hybrid there is no single "cycles" event which can cover
> > > all CPUs.
> >
> > Does that mean the kernel would reject the legacy "cycles" event
> > on hybrid CPUs?
> 
> I believe not. When the extended type isn't set on legacy cycles we
> often have the CPU and from that can determine the PMU. The issue is
> with the -1 any CPU perf_event_open option. As I was told, the PMU the
> event is opened on in this case is the first one registered in the
> kernel, on Intel hybrid this could be cpu_core or cpu_atom.. but IIRC
> it'll probably be cpu_core. On ARM ¯\_(ツ)_/¯.

On ARM it'll be essentially the same as on x86: if you open an event with
type==PERF_EVENT_TYPE_HARDWARE (without the extended HW type pointing to a
specific PMU), and with cpu==-1, it'll go to an arbitrary CPU PMU, whichever
happens to be found by perf_init_event() when iterating over the 'pmus' list.

If you open an event with type==PERF_EVENT_TYPE_HARDWARE and cpu!=-1, the event
will opened on the appropriate CPU PMU, by virtue of being rejected by others
when perf_init_event() iterates over the 'pmus' list.

Mark.
Namhyung Kim Dec. 12, 2023, 6:49 p.m. UTC | #8 
On Tue, Dec 12, 2023 at 10:31 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Tue, Dec 12, 2023 at 10:00:16AM -0800, Ian Rogers wrote:
> > On Tue, Dec 12, 2023 at 9:23 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > On Tue, Dec 12, 2023 at 7:56 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
> > > >
> > > >
> > > >
> > > > On 2023-12-11 4:13 p.m., Arnaldo Carvalho de Melo wrote:
> > > > > Em Fri, Dec 08, 2023 at 01:08:55PM -0800, kan.liang@linux.intel.com escreveu:
> > > > >> From: Kan Liang <kan.liang@linux.intel.com>
> > > > >>
> > > > >> perf top errors out on a hybrid machine
> > > > >>  $perf top
> > > > >>
> > > > >>  Error:
> > > > >>  The cycles:P event is not supported.
> > > > >>
> > > > >> The user_requested_cpus may contain CPUs that are invalid for a hybrid
> > > > >> PMU. It causes perf_event_open to fail.
> > > > >
> > > > > ?
> > > > >
> > > > > All perf top expects is that the "cycles", the most basic one, be
> > > > > collected, on all CPUs in the system.
> > > > >
> > > >
> > > > Yes, but for hybrid there is no single "cycles" event which can cover
> > > > all CPUs.
> > >
> > > Does that mean the kernel would reject the legacy "cycles" event
> > > on hybrid CPUs?
> >
> > I believe not. When the extended type isn't set on legacy cycles we
> > often have the CPU and from that can determine the PMU. The issue is
> > with the -1 any CPU perf_event_open option. As I was told, the PMU the
> > event is opened on in this case is the first one registered in the
> > kernel, on Intel hybrid this could be cpu_core or cpu_atom.. but IIRC
> > it'll probably be cpu_core. On ARM ¯\_(ツ)_/¯.
>
> On ARM it'll be essentially the same as on x86: if you open an event with
> type==PERF_EVENT_TYPE_HARDWARE (without the extended HW type pointing to a
> specific PMU), and with cpu==-1, it'll go to an arbitrary CPU PMU, whichever
> happens to be found by perf_init_event() when iterating over the 'pmus' list.
>
> If you open an event with type==PERF_EVENT_TYPE_HARDWARE and cpu!=-1, the event
> will opened on the appropriate CPU PMU, by virtue of being rejected by others
> when perf_init_event() iterates over the 'pmus' list.

Ok, that means "cycles" with cpu == -1 would not work well.

I'm curious if it's possible to do some basic work at the event_init()
like to preserve (common) resource and to do some other work at
sched to config PMU on the current CPU.  So that users can simply
use "cycles" or "instructions" for their processes.

Thanks,
Namhyung
Liang, Kan Dec. 12, 2023, 7:22 p.m. UTC | #9 
On 2023-12-12 1:49 p.m., Namhyung Kim wrote:
> On Tue, Dec 12, 2023 at 10:31 AM Mark Rutland <mark.rutland@arm.com> wrote:
>>
>> On Tue, Dec 12, 2023 at 10:00:16AM -0800, Ian Rogers wrote:
>>> On Tue, Dec 12, 2023 at 9:23 AM Namhyung Kim <namhyung@kernel.org> wrote:
>>>>
>>>> On Tue, Dec 12, 2023 at 7:56 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 2023-12-11 4:13 p.m., Arnaldo Carvalho de Melo wrote:
>>>>>> Em Fri, Dec 08, 2023 at 01:08:55PM -0800, kan.liang@linux.intel.com escreveu:
>>>>>>> From: Kan Liang <kan.liang@linux.intel.com>
>>>>>>>
>>>>>>> perf top errors out on a hybrid machine
>>>>>>>  $perf top
>>>>>>>
>>>>>>>  Error:
>>>>>>>  The cycles:P event is not supported.
>>>>>>>
>>>>>>> The user_requested_cpus may contain CPUs that are invalid for a hybrid
>>>>>>> PMU. It causes perf_event_open to fail.
>>>>>>
>>>>>> ?
>>>>>>
>>>>>> All perf top expects is that the "cycles", the most basic one, be
>>>>>> collected, on all CPUs in the system.
>>>>>>
>>>>>
>>>>> Yes, but for hybrid there is no single "cycles" event which can cover
>>>>> all CPUs.
>>>>
>>>> Does that mean the kernel would reject the legacy "cycles" event
>>>> on hybrid CPUs?
>>>
>>> I believe not. When the extended type isn't set on legacy cycles we
>>> often have the CPU and from that can determine the PMU. The issue is
>>> with the -1 any CPU perf_event_open option. As I was told, the PMU the
>>> event is opened on in this case is the first one registered in the
>>> kernel, on Intel hybrid this could be cpu_core or cpu_atom.. but IIRC
>>> it'll probably be cpu_core. On ARM ¯\_(ツ)_/¯.
>>
>> On ARM it'll be essentially the same as on x86: if you open an event with
>> type==PERF_EVENT_TYPE_HARDWARE (without the extended HW type pointing to a
>> specific PMU), and with cpu==-1, it'll go to an arbitrary CPU PMU, whichever
>> happens to be found by perf_init_event() when iterating over the 'pmus' list.
>>
>> If you open an event with type==PERF_EVENT_TYPE_HARDWARE and cpu!=-1, the event
>> will opened on the appropriate CPU PMU, by virtue of being rejected by others
>> when perf_init_event() iterates over the 'pmus' list.
> 
> Ok, that means "cycles" with cpu == -1 would not work well.
>

Unless a PMU is specified.

> I'm curious if it's possible to do some basic work at the event_init()
> like to preserve (common) resource and to do some other work at
> sched to config PMU on the current CPU.  So that users can simply
> use "cycles" or "instructions" for their processes.
> 

The current code treats the hybrid as two standalone PMUs. To preserve
the common resource in the other PMU, I think the only way is to create
an event on the other PMU. It's what perf tool does now. I don't think
we want to move the logic to the kernel.

I think a possible way is to abstract a common PMU (cpu) which only
includes common PMU features. It should be doable, because without the
enabling code of hybrid, the default PMU is the common PMU. But I don't
know how does it coexist with the other hybrid PMUs if we have both
common PMU and hybrid PMUs available? It may just bring more complexity.

Thanks,
Kan
Ian Rogers Dec. 12, 2023, 7:26 p.m. UTC | #10 
On Tue, Dec 12, 2023 at 10:49 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Tue, Dec 12, 2023 at 10:31 AM Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > On Tue, Dec 12, 2023 at 10:00:16AM -0800, Ian Rogers wrote:
> > > On Tue, Dec 12, 2023 at 9:23 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > > >
> > > > On Tue, Dec 12, 2023 at 7:56 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 2023-12-11 4:13 p.m., Arnaldo Carvalho de Melo wrote:
> > > > > > Em Fri, Dec 08, 2023 at 01:08:55PM -0800, kan.liang@linux.intel.com escreveu:
> > > > > >> From: Kan Liang <kan.liang@linux.intel.com>
> > > > > >>
> > > > > >> perf top errors out on a hybrid machine
> > > > > >>  $perf top
> > > > > >>
> > > > > >>  Error:
> > > > > >>  The cycles:P event is not supported.
> > > > > >>
> > > > > >> The user_requested_cpus may contain CPUs that are invalid for a hybrid
> > > > > >> PMU. It causes perf_event_open to fail.
> > > > > >
> > > > > > ?
> > > > > >
> > > > > > All perf top expects is that the "cycles", the most basic one, be
> > > > > > collected, on all CPUs in the system.
> > > > > >
> > > > >
> > > > > Yes, but for hybrid there is no single "cycles" event which can cover
> > > > > all CPUs.
> > > >
> > > > Does that mean the kernel would reject the legacy "cycles" event
> > > > on hybrid CPUs?
> > >
> > > I believe not. When the extended type isn't set on legacy cycles we
> > > often have the CPU and from that can determine the PMU. The issue is
> > > with the -1 any CPU perf_event_open option. As I was told, the PMU the
> > > event is opened on in this case is the first one registered in the
> > > kernel, on Intel hybrid this could be cpu_core or cpu_atom.. but IIRC
> > > it'll probably be cpu_core. On ARM ¯\_(ツ)_/¯.
> >
> > On ARM it'll be essentially the same as on x86: if you open an event with
> > type==PERF_EVENT_TYPE_HARDWARE (without the extended HW type pointing to a
> > specific PMU), and with cpu==-1, it'll go to an arbitrary CPU PMU, whichever
> > happens to be found by perf_init_event() when iterating over the 'pmus' list.
> >
> > If you open an event with type==PERF_EVENT_TYPE_HARDWARE and cpu!=-1, the event
> > will opened on the appropriate CPU PMU, by virtue of being rejected by others
> > when perf_init_event() iterates over the 'pmus' list.
>
> Ok, that means "cycles" with cpu == -1 would not work well.
>
> I'm curious if it's possible to do some basic work at the event_init()
> like to preserve (common) resource and to do some other work at
> sched to config PMU on the current CPU.  So that users can simply
> use "cycles" or "instructions" for their processes.

It should be possible to make things better, especially for standard
events for cycles or instructions, for software that isn't aware of
multiple core PMUs and the extended type field. There are legacy
events like dTLB-speculative-read that may be supported by 1 PMU and
not the other, so what should happen with thread scheduling there?

On RISC-V there was discussion of the legacy to pmu/config mapping
that happens in the driver being something that is handled in the
tool. A lot of the legacy events end up being "<not supported>" which
is a pretty bad user experience, like this on AMD:
```
# perf stat -d -a sleep 1

 Performance counter stats for 'system wide':

        259,256.21 msec cpu-clock                        #  257.445
CPUs utilized
            11,931      context-switches                 #   46.020
/sec
               264      cpu-migrations                   #    1.018
/sec
               419      page-faults                      #    1.616
/sec
     5,892,812,250      cycles                           #    0.023
GHz                         (62.43%)
     1,159,909,806      stalled-cycles-frontend          #   19.68%
frontend cycles idle        (62.48%)
       831,668,644      stalled-cycles-backend           #   14.11%
backend cycles idle         (62.52%)
     2,825,351,898      instructions                     #    0.48
insn per cycle
                                                  #    0.41  stalled
cycles per insn     (62.54%)
       520,403,374      branches                         #    2.007
M/sec                       (62.57%)
        12,050,970      branch-misses                    #    2.32% of
all branches             (62.55%)
     1,117,382,209      L1-dcache-loads                  #    4.310
M/sec                       (62.48%)
        61,060,457      L1-dcache-load-misses            #    5.46% of
all L1-dcache accesses   (62.43%)
   <not supported>      LLC-loads
   <not supported>      LLC-load-misses

       1.007033432 seconds time elapsed
```
So I think the move should be away from legacy events but that doesn't
necessarily mean we can't make them better.

Thanks,
Ian

> Thanks,
> Namhyung
Mark Rutland Dec. 13, 2023, 12:05 p.m. UTC | #11 
On Tue, Dec 12, 2023 at 02:22:49PM -0500, Liang, Kan wrote:
> 
> 
> On 2023-12-12 1:49 p.m., Namhyung Kim wrote:
> > On Tue, Dec 12, 2023 at 10:31 AM Mark Rutland <mark.rutland@arm.com> wrote:
> >>
> >> On Tue, Dec 12, 2023 at 10:00:16AM -0800, Ian Rogers wrote:
> >>> On Tue, Dec 12, 2023 at 9:23 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >>>>
> >>>> On Tue, Dec 12, 2023 at 7:56 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 2023-12-11 4:13 p.m., Arnaldo Carvalho de Melo wrote:
> >>>>>> Em Fri, Dec 08, 2023 at 01:08:55PM -0800, kan.liang@linux.intel.com escreveu:
> >>>>>>> From: Kan Liang <kan.liang@linux.intel.com>
> >>>>>>>
> >>>>>>> perf top errors out on a hybrid machine
> >>>>>>>  $perf top
> >>>>>>>
> >>>>>>>  Error:
> >>>>>>>  The cycles:P event is not supported.
> >>>>>>>
> >>>>>>> The user_requested_cpus may contain CPUs that are invalid for a hybrid
> >>>>>>> PMU. It causes perf_event_open to fail.
> >>>>>>
> >>>>>> ?
> >>>>>>
> >>>>>> All perf top expects is that the "cycles", the most basic one, be
> >>>>>> collected, on all CPUs in the system.
> >>>>>>
> >>>>>
> >>>>> Yes, but for hybrid there is no single "cycles" event which can cover
> >>>>> all CPUs.
> >>>>
> >>>> Does that mean the kernel would reject the legacy "cycles" event
> >>>> on hybrid CPUs?
> >>>
> >>> I believe not. When the extended type isn't set on legacy cycles we
> >>> often have the CPU and from that can determine the PMU. The issue is
> >>> with the -1 any CPU perf_event_open option. As I was told, the PMU the
> >>> event is opened on in this case is the first one registered in the
> >>> kernel, on Intel hybrid this could be cpu_core or cpu_atom.. but IIRC
> >>> it'll probably be cpu_core. On ARM ¯\_(ツ)_/¯.
> >>
> >> On ARM it'll be essentially the same as on x86: if you open an event with
> >> type==PERF_EVENT_TYPE_HARDWARE (without the extended HW type pointing to a
> >> specific PMU), and with cpu==-1, it'll go to an arbitrary CPU PMU, whichever
> >> happens to be found by perf_init_event() when iterating over the 'pmus' list.
> >>
> >> If you open an event with type==PERF_EVENT_TYPE_HARDWARE and cpu!=-1, the event
> >> will opened on the appropriate CPU PMU, by virtue of being rejected by others
> >> when perf_init_event() iterates over the 'pmus' list.
> > 
> > Ok, that means "cycles" with cpu == -1 would not work well.
> 
> Unless a PMU is specified.
> 
> > I'm curious if it's possible to do some basic work at the event_init()
> > like to preserve (common) resource and to do some other work at
> > sched to config PMU on the current CPU.  So that users can simply
> > use "cycles" or "instructions" for their processes.
> 
> The current code treats the hybrid as two standalone PMUs. To preserve
> the common resource in the other PMU, I think the only way is to create
> an event on the other PMU. It's what perf tool does now. I don't think
> we want to move the logic to the kernel.

Agreed.

> I think a possible way is to abstract a common PMU (cpu) which only
> includes common PMU features. It should be doable, because without the
> enabling code of hybrid, the default PMU is the common PMU. But I don't
> know how does it coexist with the other hybrid PMUs if we have both
> common PMU and hybrid PMUs available? It may just bring more complexity.

I think that brings a surprising amount of complexity, and I'm not entirely
sure if that's practical (since you'd effectively end up with a logical PMU
being dependent on multiple other logical PMUs).

I also think that it's practically necessary to expose the counts to the user
separately, even for common events. For example, the 'instructions' event may
count differently (speculative vs architectural execution), and 'cycles' can be
wildly different across microarchitectures due to realizable IPC, and blindly
adding those up across PMUs is liable to produce a misleading figure (and/or
one with massive variation).

While it is ugly, I think that it's necessary for userspace to discover the set
of CPU PMUs and open seperate events on them in order to produce useful data.

Specifically for perf top, if one is monitoring all CPUs, it'd be fine to open
a PERF_TYPE_HARDWARE event for each CPU; so long as cpu!=-1 it would go to the
relevant PMU and be counted as expected.

Thanks,
Mark.
Arnaldo Carvalho de Melo Dec. 15, 2023, 3:36 p.m. UTC | #12 
Em Tue, Dec 12, 2023 at 06:31:05PM +0000, Mark Rutland escreveu:
> On ARM it'll be essentially the same as on x86: if you open an event with
> type==PERF_EVENT_TYPE_HARDWARE (without the extended HW type pointing to a
> specific PMU), and with cpu==-1, it'll go to an arbitrary CPU PMU, whichever
> happens to be found by perf_init_event() when iterating over the 'pmus' list.
 
> If you open an event with type==PERF_EVENT_TYPE_HARDWARE and cpu!=-1, the event
> will opened on the appropriate CPU PMU, by virtue of being rejected by others
> when perf_init_event() iterates over the 'pmus' list.

The way that it is working non on my intel hybrid system, with Kan's
patch, is equivalent to using this on the RK3399pc board I have:

root@roc-rk3399-pc:~# perf top -e armv8_cortex_a72/cycles/P,armv8_cortex_a53/cycles/P

Wouldn't be better to make 'perf top' on ARM work the way is being done
in x86 now, i.e. default to opening the two events, one per PMU and
allow the user to switch back and forth using the TUI/stdio?

Kan, I also noticed that the name of the event is:

1K cpu_atom/cycles:P/                                                                                                                                                                         ◆
11K cpu_core/cycles:P/

If I try to use that on the command line:

root@number:~# perf top -e cpu_atom/cycles:P/
event syntax error: 'cpu_atom/cycles:P/'
                              \___ Bad event or PMU

Unable to find PMU or event on a PMU of 'cpu_atom'

Initial error:
event syntax error: 'cpu_atom/cycles:P/'
                              \___ unknown term 'cycles:P' for pmu 'cpu_atom'

valid terms: event,pc,edge,offcore_rsp,ldlat,inv,umask,cmask,config,config1,config2,config3,name,period,freq,branch_type,time,call-graph,stack-size,no-inherit,inherit,max-stack,nr,no-overwrite,overwrite,driver-config,percore,aux-output,aux-sample-size,metric-id,raw,legacy-cache,hardware
Run 'perf list' for a list of valid events

 Usage: perf top [<options>]

    -e, --event <event>   event selector. use 'perf list' to list available events
root@number:~#

It should be:

  "cpu_atom/cycles/P"

- Arnaldo
Mark Rutland Dec. 15, 2023, 4:51 p.m. UTC | #13 
On Fri, Dec 15, 2023 at 12:36:10PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Dec 12, 2023 at 06:31:05PM +0000, Mark Rutland escreveu:
> > On ARM it'll be essentially the same as on x86: if you open an event with
> > type==PERF_EVENT_TYPE_HARDWARE (without the extended HW type pointing to a
> > specific PMU), and with cpu==-1, it'll go to an arbitrary CPU PMU, whichever
> > happens to be found by perf_init_event() when iterating over the 'pmus' list.
>  
> > If you open an event with type==PERF_EVENT_TYPE_HARDWARE and cpu!=-1, the event
> > will opened on the appropriate CPU PMU, by virtue of being rejected by others
> > when perf_init_event() iterates over the 'pmus' list.
> 
> The way that it is working non on my intel hybrid system, with Kan's
> patch, is equivalent to using this on the RK3399pc board I have:
> 
> root@roc-rk3399-pc:~# perf top -e armv8_cortex_a72/cycles/P,armv8_cortex_a53/cycles/P
> 
> Wouldn't be better to make 'perf top' on ARM work the way is being done
> in x86 now, i.e. default to opening the two events, one per PMU and
> allow the user to switch back and forth using the TUI/stdio?

TBH, for perf top I don't know *which* behaviour is preferable, but I agree
that it'd be good for x86 and arm to work in the same way.

For design-cleanliness and consistency with other commands I can see that
opening those separately is probably for the best, but for typical usage of
perf top it's really nice to have those presented together without having to
tab back-and-forth between the distinct PMUs, and so the existing behaviour of
using CPU-bound PERF_EVENT_TYPE_HARDWARE events is arguably nicer for the user.

I don't have a strong feeling either way; I'm personally happy so long as
explicit pmu_name/event/ events don't get silently converted into
PERF_EVENT_TYPE_HARDWARE events, and as long as we correctly set the extended
HW type when we decide to use that.

Thanks,
Mark.

> Kan, I also noticed that the name of the event is:
> 
> 1K cpu_atom/cycles:P/                                                                                                                                                                         ◆
> 11K cpu_core/cycles:P/
> 
> If I try to use that on the command line:
> 
> root@number:~# perf top -e cpu_atom/cycles:P/
> event syntax error: 'cpu_atom/cycles:P/'
>                               \___ Bad event or PMU
> 
> Unable to find PMU or event on a PMU of 'cpu_atom'
> 
> Initial error:
> event syntax error: 'cpu_atom/cycles:P/'
>                               \___ unknown term 'cycles:P' for pmu 'cpu_atom'
> 
> valid terms: event,pc,edge,offcore_rsp,ldlat,inv,umask,cmask,config,config1,config2,config3,name,period,freq,branch_type,time,call-graph,stack-size,no-inherit,inherit,max-stack,nr,no-overwrite,overwrite,driver-config,percore,aux-output,aux-sample-size,metric-id,raw,legacy-cache,hardware
> Run 'perf list' for a list of valid events
> 
>  Usage: perf top [<options>]
> 
>     -e, --event <event>   event selector. use 'perf list' to list available events
> root@number:~#
> 
> It should be:
> 
>   "cpu_atom/cycles/P"
> 
> - Arnaldo
Arnaldo Carvalho de Melo Dec. 15, 2023, 5:49 p.m. UTC | #14 
Em Fri, Dec 15, 2023 at 04:51:49PM +0000, Mark Rutland escreveu:
> On Fri, Dec 15, 2023 at 12:36:10PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Dec 12, 2023 at 06:31:05PM +0000, Mark Rutland escreveu:
> > > On ARM it'll be essentially the same as on x86: if you open an event with
> > > type==PERF_EVENT_TYPE_HARDWARE (without the extended HW type pointing to a
> > > specific PMU), and with cpu==-1, it'll go to an arbitrary CPU PMU, whichever
> > > happens to be found by perf_init_event() when iterating over the 'pmus' list.

> > > If you open an event with type==PERF_EVENT_TYPE_HARDWARE and cpu!=-1, the event
> > > will opened on the appropriate CPU PMU, by virtue of being rejected by others
> > > when perf_init_event() iterates over the 'pmus' list.

> > The way that it is working non on my intel hybrid system, with Kan's
> > patch, is equivalent to using this on the RK3399pc board I have:

> > root@roc-rk3399-pc:~# perf top -e armv8_cortex_a72/cycles/P,armv8_cortex_a53/cycles/P

> > Wouldn't be better to make 'perf top' on ARM work the way is being done
> > in x86 now, i.e. default to opening the two events, one per PMU and
> > allow the user to switch back and forth using the TUI/stdio?
 
> TBH, for perf top I don't know *which* behaviour is preferable, but I agree
> that it'd be good for x86 and arm to work in the same way.

Right, reducing the difference in the user experience accross arches.
 
> For design-cleanliness and consistency with other commands I can see that
> opening those separately is probably for the best, but for typical usage of
> perf top it's really nice to have those presented together without having to
> tab back-and-forth between the distinct PMUs, and so the existing behaviour of

Humm, so you would want two histogram viewers, one for each PMU, side by
side?

> using CPU-bound PERF_EVENT_TYPE_HARDWARE events is arguably nicer for the user.

So, on ARM64, start the following 'perf trace' session, then run the
stock 'perf top':

root@roc-rk3399-pc:~# perf trace -e perf_event_open
<SNIP calls doing capability queries and setting up sideband stuff>
   535.764 ( 0.015 ms): perf/15627 perf_event_open(attr_uptr: { type: 0 (PERF_TYPE_HARDWARE), size: 136, config: 0 (PERF_COUNT_HW_CPU_CYCLES), { sample_period, sample_freq }: 4000, sample_type: IP|TID|TIME|CPU|PERIOD, read_format: ID|LOST, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq: 1, task: 1, precise_ip: 3, sample_id_all: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 }, pid: -1, group_fd: -1, flags: FD_CLOEXEC) = 19
   535.783 ( 0.067 ms): perf/15627 perf_event_open(attr_uptr: { type: 0 (PERF_TYPE_HARDWARE), size: 136, config: 0 (PERF_COUNT_HW_CPU_CYCLES), { sample_period, sample_freq }: 4000, sample_type: IP|TID|TIME|CPU|PERIOD, read_format: ID|LOST, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq: 1, task: 1, precise_ip: 3, sample_id_all: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 }, pid: -1, cpu: 1, group_fd: -1, flags: FD_CLOEXEC) = 28
   535.854 ( 0.063 ms): perf/15627 perf_event_open(attr_uptr: { type: 0 (PERF_TYPE_HARDWARE), size: 136, config: 0 (PERF_COUNT_HW_CPU_CYCLES), { sample_period, sample_freq }: 4000, sample_type: IP|TID|TIME|CPU|PERIOD, read_format: ID|LOST, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq: 1, task: 1, precise_ip: 3, sample_id_all: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 }, pid: -1, cpu: 2, group_fd: -1, flags: FD_CLOEXEC) = 29
   535.920 ( 0.015 ms): perf/15627 perf_event_open(attr_uptr: { type: 0 (PERF_TYPE_HARDWARE), size: 136, config: 0 (PERF_COUNT_HW_CPU_CYCLES), { sample_period, sample_freq }: 4000, sample_type: IP|TID|TIME|CPU|PERIOD, read_format: ID|LOST, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq: 1, task: 1, precise_ip: 3, sample_id_all: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 }, pid: -1, cpu: 3, group_fd: -1, flags: FD_CLOEXEC) = 30
   535.939 ( 0.016 ms): perf/15627 perf_event_open(attr_uptr: { type: 0 (PERF_TYPE_HARDWARE), size: 136, config: 0 (PERF_COUNT_HW_CPU_CYCLES), { sample_period, sample_freq }: 4000, sample_type: IP|TID|TIME|CPU|PERIOD, read_format: ID|LOST, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq: 1, task: 1, precise_ip: 3, sample_id_all: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 }, pid: -1, cpu: 4, group_fd: -1, flags: FD_CLOEXEC) = 31
   535.959 ( 0.011 ms): perf/15627 perf_event_open(attr_uptr: { type: 0 (PERF_TYPE_HARDWARE), size: 136, config: 0 (PERF_COUNT_HW_CPU_CYCLES), { sample_period, sample_freq }: 4000, sample_type: IP|TID|TIME|CPU|PERIOD, read_format: ID|LOST, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq: 1, task: 1, precise_ip: 3, sample_id_all: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 }, pid: -1, cpu: 5, group_fd: -1, flags: FD_CLOEXEC) = 32

root@roc-rk3399-pc:~# grep "CPU part" /proc/cpuinfo  | uniq -c
      4 CPU part	: 0xd03
      2 CPU part	: 0xd08
root@roc-rk3399-pc:~#

It is already doing what you suggest, right? PERF_TYPE_HARDWARE, one
counter per CPU, maps to armv8_cortex_a72/cycles/P and
armv8_cortex_a53/cycles/P.

One thing I'm thinking is that we should split this per PMU at the
hist_entry, so that we could show how many samples/events came from each
of them...

- Arnaldo
 
> I don't have a strong feeling either way; I'm personally happy so long as
> explicit pmu_name/event/ events don't get silently converted into
> PERF_EVENT_TYPE_HARDWARE events, and as long as we correctly set the extended
> HW type when we decide to use that.
> 
> Thanks,
> Mark.
> 
> > Kan, I also noticed that the name of the event is:
> > 
> > 1K cpu_atom/cycles:P/                                                                                                                                                                         ◆
> > 11K cpu_core/cycles:P/
> > 
> > If I try to use that on the command line:
> > 
> > root@number:~# perf top -e cpu_atom/cycles:P/
> > event syntax error: 'cpu_atom/cycles:P/'
> >                               \___ Bad event or PMU
> > 
> > Unable to find PMU or event on a PMU of 'cpu_atom'
> > 
> > Initial error:
> > event syntax error: 'cpu_atom/cycles:P/'
> >                               \___ unknown term 'cycles:P' for pmu 'cpu_atom'
> > 
> > valid terms: event,pc,edge,offcore_rsp,ldlat,inv,umask,cmask,config,config1,config2,config3,name,period,freq,branch_type,time,call-graph,stack-size,no-inherit,inherit,max-stack,nr,no-overwrite,overwrite,driver-config,percore,aux-output,aux-sample-size,metric-id,raw,legacy-cache,hardware
> > Run 'perf list' for a list of valid events
> > 
> >  Usage: perf top [<options>]
> > 
> >     -e, --event <event>   event selector. use 'perf list' to list available events
> > root@number:~#
> > 
> > It should be:
> > 
> >   "cpu_atom/cycles/P"
Liang, Kan Dec. 15, 2023, 5:59 p.m. UTC | #15 
On 2023-12-15 10:36 a.m., Arnaldo Carvalho de Melo wrote:
> Kan, I also noticed that the name of the event is:
> 
> 1K cpu_atom/cycles:P/                                                   
>                                                                         
>                                               ◆
> 11K cpu_core/cycles:P/
> 
> If I try to use that on the command line:
> 
> root@number:~# perf top -e cpu_atom/cycles:P/
> event syntax error: 'cpu_atom/cycles:P/'
>                               \___ Bad event or PMU
> 
> Unable to find PMU or event on a PMU of 'cpu_atom'
> 
> Initial error:
> event syntax error: 'cpu_atom/cycles:P/'
>                               \___ unknown term 'cycles:P' for pmu 
> 'cpu_atom'
> 
> valid terms: 
> event,pc,edge,offcore_rsp,ldlat,inv,umask,cmask,config,config1,config2,config3,name,period,freq,branch_type,time,call-graph,stack-size,no-inherit,inherit,max-stack,nr,no-overwrite,overwrite,driver-config,percore,aux-output,aux-sample-size,metric-id,raw,legacy-cache,hardware
> Run
>  'perf list' for a list of valid events
> 
>  Usage: perf top [<options>]
> 
>     -e, --event <event>   event selector. use 'perf list' to list 
> available events
> root@number:~#
> 
> It should be:
> 
>   "cpu_atom/cycles/P"

The issue also impacts the perf record and report as well.

#perf record true
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.019 MB perf.data (16 samples) ]

#perf report --header-only | grep event
# event : name = cpu_atom/cycles:P/, , id = { 7360, 7361, 7362, 7363,
7364, 7365, 7366, 7367, 7368, 7369 }, type = 0 (PERF_TYPE_HARDWARE),
size = 136, config = 0xa00000000, { sample_period, sample_freq } = 3000,
sample_type = IP|TID|TIME|PERIOD|IDENTIFIER, read_format = ID|LOST,
disabled = 1, inherit = 1, freq = 1, enable_on_exec = 1, precise_ip = 3,
sample_id_all = 1
# event : name = cpu_core/cycles:P/, , id = { 7370, 7371, 7372, 7373,
7374, 7375, 7376, 7377, 7378, 7379, 7380, 7381 }, type = 0
(PERF_TYPE_HARDWARE), size = 136, config = 0x400000000, { sample_period,
sample_freq } = 3000, sample_type = IP|TID|TIME|PERIOD|IDENTIFIER,
read_format = ID|LOST, disabled = 1, inherit = 1, freq = 1,
enable_on_exec = 1, precise_ip = 3, sample_id_all = 1

I think we should move all the modifiers after the "/". The below patch
can fix it.

https://lore.kernel.org/lkml/20231215175455.1300261-1-kan.liang@linux.intel.com/

Thanks,
Kan
Arnaldo Carvalho de Melo Dec. 15, 2023, 6:26 p.m. UTC | #16 
Em Fri, Dec 15, 2023 at 12:59:22PM -0500, Liang, Kan escreveu:
> On 2023-12-15 10:36 a.m., Arnaldo Carvalho de Melo wrote:
> 
> #perf report --header-only | grep event
> # event : name = cpu_atom/cycles:P/, , id = { 7360, 7361, 7362, 7363,
> 7364, 7365, 7366, 7367, 7368, 7369 }, type = 0 (PERF_TYPE_HARDWARE),
> size = 136, config = 0xa00000000, { sample_period, sample_freq } = 3000,
> sample_type = IP|TID|TIME|PERIOD|IDENTIFIER, read_format = ID|LOST,
> disabled = 1, inherit = 1, freq = 1, enable_on_exec = 1, precise_ip = 3,
> sample_id_all = 1
> # event : name = cpu_core/cycles:P/, , id = { 7370, 7371, 7372, 7373,
> 7374, 7375, 7376, 7377, 7378, 7379, 7380, 7381 }, type = 0
> (PERF_TYPE_HARDWARE), size = 136, config = 0x400000000, { sample_period,
> sample_freq } = 3000, sample_type = IP|TID|TIME|PERIOD|IDENTIFIER,
> read_format = ID|LOST, disabled = 1, inherit = 1, freq = 1,
> enable_on_exec = 1, precise_ip = 3, sample_id_all = 1
> 
> I think we should move all the modifiers after the "/". The below patch
> can fix it.
> 
> https://lore.kernel.org/lkml/20231215175455.1300261-1-kan.liang@linux.intel.com/

Right, I implemented it in a slightly different way, but end result
should be the same:

From 5dd1b7ab1ba69ebb8e070923dcc214b7b489ffc2 Mon Sep 17 00:00:00 2001
From: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Fri, 15 Dec 2023 15:23:30 -0300
Subject: [PATCH 1/1] perf evlist: Move event attributes to after the / when
 uniquefying using the PMU name

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evlist.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 6f0892803c2249af..3a9505c99490b372 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -2522,7 +2522,7 @@ void evlist__warn_user_requested_cpus(struct evlist *evlist, const char *cpu_lis
 void evlist__uniquify_name(struct evlist *evlist)
 {
 	struct evsel *pos;
-	char *new_name;
+	char *new_name, *attributes;
 	int ret;
 
 	if (perf_pmus__num_core_pmus() == 1)
@@ -2535,8 +2535,16 @@ void evlist__uniquify_name(struct evlist *evlist)
 		if (strchr(pos->name, '/'))
 			continue;
 
-		ret = asprintf(&new_name, "%s/%s/",
-			       pos->pmu_name, pos->name);
+		attributes = strchr(pos->name, ':');
+		if (attributes)
+			*attributes = '\0';
+
+		ret = asprintf(&new_name, "%s/%s/%s",
+			       pos->pmu_name, pos->name, attributes ? attributes + 1 : "");
+
+		if (attributes)
+			*attributes = ':';
+
 		if (ret) {
 			free(pos->name);
 			pos->name = new_name;
Liang, Kan Dec. 15, 2023, 6:53 p.m. UTC | #17 
On 2023-12-15 1:26 p.m., Arnaldo Carvalho de Melo wrote:
> Em Fri, Dec 15, 2023 at 12:59:22PM -0500, Liang, Kan escreveu:
>> On 2023-12-15 10:36 a.m., Arnaldo Carvalho de Melo wrote:
>>
>> #perf report --header-only | grep event
>> # event : name = cpu_atom/cycles:P/, , id = { 7360, 7361, 7362, 7363,
>> 7364, 7365, 7366, 7367, 7368, 7369 }, type = 0 (PERF_TYPE_HARDWARE),
>> size = 136, config = 0xa00000000, { sample_period, sample_freq } = 3000,
>> sample_type = IP|TID|TIME|PERIOD|IDENTIFIER, read_format = ID|LOST,
>> disabled = 1, inherit = 1, freq = 1, enable_on_exec = 1, precise_ip = 3,
>> sample_id_all = 1
>> # event : name = cpu_core/cycles:P/, , id = { 7370, 7371, 7372, 7373,
>> 7374, 7375, 7376, 7377, 7378, 7379, 7380, 7381 }, type = 0
>> (PERF_TYPE_HARDWARE), size = 136, config = 0x400000000, { sample_period,
>> sample_freq } = 3000, sample_type = IP|TID|TIME|PERIOD|IDENTIFIER,
>> read_format = ID|LOST, disabled = 1, inherit = 1, freq = 1,
>> enable_on_exec = 1, precise_ip = 3, sample_id_all = 1
>>
>> I think we should move all the modifiers after the "/". The below patch
>> can fix it.
>>
>> https://lore.kernel.org/lkml/20231215175455.1300261-1-kan.liang@linux.intel.com/
> 
> Right, I implemented it in a slightly different way, but end result
> should be the same:
> 
> From 5dd1b7ab1ba69ebb8e070923dcc214b7b489ffc2 Mon Sep 17 00:00:00 2001
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date: Fri, 15 Dec 2023 15:23:30 -0300
> Subject: [PATCH 1/1] perf evlist: Move event attributes to after the / when
>  uniquefying using the PMU name
> 
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Looks good to me and verified.

Tested-by: Kan Liang <kan.liang@linux.intel.com>

Thanks,
Kan
> ---
>  tools/perf/util/evlist.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 6f0892803c2249af..3a9505c99490b372 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -2522,7 +2522,7 @@ void evlist__warn_user_requested_cpus(struct evlist *evlist, const char *cpu_lis
>  void evlist__uniquify_name(struct evlist *evlist)
>  {
>  	struct evsel *pos;
> -	char *new_name;
> +	char *new_name, *attributes;
>  	int ret;
>  
>  	if (perf_pmus__num_core_pmus() == 1)
> @@ -2535,8 +2535,16 @@ void evlist__uniquify_name(struct evlist *evlist)
>  		if (strchr(pos->name, '/'))
>  			continue;
>  
> -		ret = asprintf(&new_name, "%s/%s/",
> -			       pos->pmu_name, pos->name);
> +		attributes = strchr(pos->name, ':');
> +		if (attributes)
> +			*attributes = '\0';
> +
> +		ret = asprintf(&new_name, "%s/%s/%s",
> +			       pos->pmu_name, pos->name, attributes ? attributes + 1 : "");
> +
> +		if (attributes)
> +			*attributes = ':';
> +
>  		if (ret) {
>  			free(pos->name);
>  			pos->name = new_name;
Arnaldo Carvalho de Melo Dec. 18, 2023, 8:23 p.m. UTC | #18 
Em Fri, Dec 15, 2023 at 01:53:12PM -0500, Liang, Kan escreveu:
> On 2023-12-15 1:26 p.m., Arnaldo Carvalho de Melo wrote:
> > Right, I implemented it in a slightly different way, but end result
> > should be the same:

> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Date: Fri, 15 Dec 2023 15:23:30 -0300
> > Subject: [PATCH 1/1] perf evlist: Move event attributes to after the / when uniquefying using the PMU name
 
> Looks good to me and verified.

> Tested-by: Kan Liang <kan.liang@linux.intel.com>

I ended up with a bit more simplified version:

From 22ecc4601e28a12661f14ca877e39348dab6be8e Mon Sep 17 00:00:00 2001
From: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Fri, 15 Dec 2023 15:23:30 -0300
Subject: [PATCH 1/1] perf evlist: Move event attributes to after the / when
 uniquefying using the PMU name

When turning an event with attributes to the format including the PMU we
need to move the "event:attributes" format to "event/attributes/" so
that we can copy the event displayed and use it in the command line,
i.e. in 'perf top' we had:

 1K cpu_atom/cycles:P/
 11K cpu_core/cycles:P/

If I try to use that on the command line:

  # perf top -e cpu_atom/cycles:P/
  event syntax error: 'cpu_atom/cycles:P/'
                                \___ Bad event or PMU

  Unable to find PMU or event on a PMU of 'cpu_atom'

  Initial error:
  event syntax error: 'cpu_atom/cycles:P/'
                                \___ unknown term 'cycles:P' for pmu
  'cpu_atom'

  valid terms:

    event,pc,edge,offcore_rsp,ldlat,inv,umask,cmask,config,config1,config2,config3,name,period,freq,branch_type,time,call-graph,stack-size,no-inherit,inherit,max-stack,nr,no-overwrite,overwrite ,driver-config,percore,aux-output,aux-sample-size,metric-id,raw,legacy-cache,hardware
  Run
    'perf list' for a list of valid events

  Usage: perf top [<options>]

     -e, --event <event>   event selector. use 'perf list' to list available events
  #

Cc: Hector Martin <marcan@marcan.st>
Cc: Ian Rogers <irogers@google.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Link: https://lore.kernel.org/lkml/ZXxyanyZgWBTOnoK@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evlist.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 6f0892803c2249af..95f25e9fb994ab2a 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -2521,9 +2521,8 @@ void evlist__warn_user_requested_cpus(struct evlist *evlist, const char *cpu_lis
 
 void evlist__uniquify_name(struct evlist *evlist)
 {
+	char *new_name, empty_attributes[2] = ":", *attributes;
 	struct evsel *pos;
-	char *new_name;
-	int ret;
 
 	if (perf_pmus__num_core_pmus() == 1)
 		return;
@@ -2535,11 +2534,17 @@ void evlist__uniquify_name(struct evlist *evlist)
 		if (strchr(pos->name, '/'))
 			continue;
 
-		ret = asprintf(&new_name, "%s/%s/",
-			       pos->pmu_name, pos->name);
-		if (ret) {
+		attributes = strchr(pos->name, ':');
+		if (attributes)
+			*attributes = '\0';
+		else
+			attributes = empty_attributes;
+
+		if (asprintf(&new_name, "%s/%s/%s", pos->pmu_name, pos->name, attributes + 1)) {
 			free(pos->name);
 			pos->name = new_name;
+		} else {
+			*attributes = ':';
 		}
 	}
 }
Liang, Kan Dec. 18, 2023, 9:07 p.m. UTC | #19 
On 2023-12-18 3:23 p.m., Arnaldo Carvalho de Melo wrote:
> Em Fri, Dec 15, 2023 at 01:53:12PM -0500, Liang, Kan escreveu:
>> On 2023-12-15 1:26 p.m., Arnaldo Carvalho de Melo wrote:
>>> Right, I implemented it in a slightly different way, but end result
>>> should be the same:
> 
>>> From: Arnaldo Carvalho de Melo <acme@redhat.com>
>>> Date: Fri, 15 Dec 2023 15:23:30 -0300
>>> Subject: [PATCH 1/1] perf evlist: Move event attributes to after the / when uniquefying using the PMU name
>  
>> Looks good to me and verified.
> 
>> Tested-by: Kan Liang <kan.liang@linux.intel.com>
> 
> I ended up with a bit more simplified version:
> 
> From 22ecc4601e28a12661f14ca877e39348dab6be8e Mon Sep 17 00:00:00 2001
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date: Fri, 15 Dec 2023 15:23:30 -0300
> Subject: [PATCH 1/1] perf evlist: Move event attributes to after the / when
>  uniquefying using the PMU name
> 
> When turning an event with attributes to the format including the PMU we
> need to move the "event:attributes" format to "event/attributes/" so
> that we can copy the event displayed and use it in the command line,
> i.e. in 'perf top' we had:
> 
>  1K cpu_atom/cycles:P/
>  11K cpu_core/cycles:P/
> 
> If I try to use that on the command line:
> 
>   # perf top -e cpu_atom/cycles:P/
>   event syntax error: 'cpu_atom/cycles:P/'
>                                 \___ Bad event or PMU
> 
>   Unable to find PMU or event on a PMU of 'cpu_atom'
> 
>   Initial error:
>   event syntax error: 'cpu_atom/cycles:P/'
>                                 \___ unknown term 'cycles:P' for pmu
>   'cpu_atom'
> 
>   valid terms:
> 
>     event,pc,edge,offcore_rsp,ldlat,inv,umask,cmask,config,config1,config2,config3,name,period,freq,branch_type,time,call-graph,stack-size,no-inherit,inherit,max-stack,nr,no-overwrite,overwrite ,driver-config,percore,aux-output,aux-sample-size,metric-id,raw,legacy-cache,hardware
>   Run
>     'perf list' for a list of valid events
> 
>   Usage: perf top [<options>]
> 
>      -e, --event <event>   event selector. use 'perf list' to list available events
>   #
> 
> Cc: Hector Martin <marcan@marcan.st>
> Cc: Ian Rogers <irogers@google.com>
> Cc: Kan Liang <kan.liang@linux.intel.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Link: https://lore.kernel.org/lkml/ZXxyanyZgWBTOnoK@kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Tested-by: Kan Liang <kan.liang@linux.intel.com>

Thanks,
Kan
> ---
>  tools/perf/util/evlist.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 6f0892803c2249af..95f25e9fb994ab2a 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -2521,9 +2521,8 @@ void evlist__warn_user_requested_cpus(struct evlist *evlist, const char *cpu_lis
>  
>  void evlist__uniquify_name(struct evlist *evlist)
>  {
> +	char *new_name, empty_attributes[2] = ":", *attributes;
>  	struct evsel *pos;
> -	char *new_name;
> -	int ret;
>  
>  	if (perf_pmus__num_core_pmus() == 1)
>  		return;
> @@ -2535,11 +2534,17 @@ void evlist__uniquify_name(struct evlist *evlist)
>  		if (strchr(pos->name, '/'))
>  			continue;
>  
> -		ret = asprintf(&new_name, "%s/%s/",
> -			       pos->pmu_name, pos->name);
> -		if (ret) {
> +		attributes = strchr(pos->name, ':');
> +		if (attributes)
> +			*attributes = '\0';
> +		else
> +			attributes = empty_attributes;
> +
> +		if (asprintf(&new_name, "%s/%s/%s", pos->pmu_name, pos->name, attributes + 1)) {
>  			free(pos->name);
>  			pos->name = new_name;
> +		} else {
> +			*attributes = ':';
>  		}
>  	}
>  }
Mark Rutland Jan. 5, 2024, 12:31 p.m. UTC | #20 
On Fri, Dec 15, 2023 at 02:49:14PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Dec 15, 2023 at 04:51:49PM +0000, Mark Rutland escreveu:
> > On Fri, Dec 15, 2023 at 12:36:10PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Tue, Dec 12, 2023 at 06:31:05PM +0000, Mark Rutland escreveu:
> > > > On ARM it'll be essentially the same as on x86: if you open an event with
> > > > type==PERF_EVENT_TYPE_HARDWARE (without the extended HW type pointing to a
> > > > specific PMU), and with cpu==-1, it'll go to an arbitrary CPU PMU, whichever
> > > > happens to be found by perf_init_event() when iterating over the 'pmus' list.
> 
> > > > If you open an event with type==PERF_EVENT_TYPE_HARDWARE and cpu!=-1, the event
> > > > will opened on the appropriate CPU PMU, by virtue of being rejected by others
> > > > when perf_init_event() iterates over the 'pmus' list.
> 
> > > The way that it is working non on my intel hybrid system, with Kan's
> > > patch, is equivalent to using this on the RK3399pc board I have:
> 
> > > root@roc-rk3399-pc:~# perf top -e armv8_cortex_a72/cycles/P,armv8_cortex_a53/cycles/P
> 
> > > Wouldn't be better to make 'perf top' on ARM work the way is being done
> > > in x86 now, i.e. default to opening the two events, one per PMU and
> > > allow the user to switch back and forth using the TUI/stdio?
>  
> > TBH, for perf top I don't know *which* behaviour is preferable, but I agree
> > that it'd be good for x86 and arm to work in the same way.
> 
> Right, reducing the difference in the user experience accross arches.
>  
> > For design-cleanliness and consistency with other commands I can see that
> > opening those separately is probably for the best, but for typical usage of
> > perf top it's really nice to have those presented together without having to
> > tab back-and-forth between the distinct PMUs, and so the existing behaviour of
> 
> Humm, so you would want two histogram viewers, one for each PMU, side by
> side?

I had meant as an aggregated view (the same as what you'd get if you opened one
plain PERF_TYPE_HARDWARE event per cpu); I hadn't considered side-by-side views.

To be clear, I'm personally happy to tab between per-pmu views, and if that's
how x86 works today for heterogeneous PMUs, I'm fine with the same for
arm/arm64. I was trying to say that I didn't have a strong preference.

> > using CPU-bound PERF_EVENT_TYPE_HARDWARE events is arguably nicer for the user.
> 
> So, on ARM64, start the following 'perf trace' session, then run the
> stock 'perf top':
> 
> root@roc-rk3399-pc:~# perf trace -e perf_event_open
> <SNIP calls doing capability queries and setting up sideband stuff>
>    535.764 ( 0.015 ms): perf/15627 perf_event_open(attr_uptr: { type: 0 (PERF_TYPE_HARDWARE), size: 136, config: 0 (PERF_COUNT_HW_CPU_CYCLES), { sample_period, sample_freq }: 4000, sample_type: IP|TID|TIME|CPU|PERIOD, read_format: ID|LOST, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq: 1, task: 1, precise_ip: 3, sample_id_all: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 }, pid: -1, group_fd: -1, flags: FD_CLOEXEC) = 19
>    535.783 ( 0.067 ms): perf/15627 perf_event_open(attr_uptr: { type: 0 (PERF_TYPE_HARDWARE), size: 136, config: 0 (PERF_COUNT_HW_CPU_CYCLES), { sample_period, sample_freq }: 4000, sample_type: IP|TID|TIME|CPU|PERIOD, read_format: ID|LOST, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq: 1, task: 1, precise_ip: 3, sample_id_all: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 }, pid: -1, cpu: 1, group_fd: -1, flags: FD_CLOEXEC) = 28
>    535.854 ( 0.063 ms): perf/15627 perf_event_open(attr_uptr: { type: 0 (PERF_TYPE_HARDWARE), size: 136, config: 0 (PERF_COUNT_HW_CPU_CYCLES), { sample_period, sample_freq }: 4000, sample_type: IP|TID|TIME|CPU|PERIOD, read_format: ID|LOST, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq: 1, task: 1, precise_ip: 3, sample_id_all: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 }, pid: -1, cpu: 2, group_fd: -1, flags: FD_CLOEXEC) = 29
>    535.920 ( 0.015 ms): perf/15627 perf_event_open(attr_uptr: { type: 0 (PERF_TYPE_HARDWARE), size: 136, config: 0 (PERF_COUNT_HW_CPU_CYCLES), { sample_period, sample_freq }: 4000, sample_type: IP|TID|TIME|CPU|PERIOD, read_format: ID|LOST, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq: 1, task: 1, precise_ip: 3, sample_id_all: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 }, pid: -1, cpu: 3, group_fd: -1, flags: FD_CLOEXEC) = 30
>    535.939 ( 0.016 ms): perf/15627 perf_event_open(attr_uptr: { type: 0 (PERF_TYPE_HARDWARE), size: 136, config: 0 (PERF_COUNT_HW_CPU_CYCLES), { sample_period, sample_freq }: 4000, sample_type: IP|TID|TIME|CPU|PERIOD, read_format: ID|LOST, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq: 1, task: 1, precise_ip: 3, sample_id_all: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 }, pid: -1, cpu: 4, group_fd: -1, flags: FD_CLOEXEC) = 31
>    535.959 ( 0.011 ms): perf/15627 perf_event_open(attr_uptr: { type: 0 (PERF_TYPE_HARDWARE), size: 136, config: 0 (PERF_COUNT_HW_CPU_CYCLES), { sample_period, sample_freq }: 4000, sample_type: IP|TID|TIME|CPU|PERIOD, read_format: ID|LOST, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq: 1, task: 1, precise_ip: 3, sample_id_all: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 }, pid: -1, cpu: 5, group_fd: -1, flags: FD_CLOEXEC) = 32
> 
> root@roc-rk3399-pc:~# grep "CPU part" /proc/cpuinfo  | uniq -c
>       4 CPU part	: 0xd03
>       2 CPU part	: 0xd08
> root@roc-rk3399-pc:~#
> 
> It is already doing what you suggest, right? PERF_TYPE_HARDWARE, one
> counter per CPU, maps to armv8_cortex_a72/cycles/P and
> armv8_cortex_a53/cycles/P.

Sounds like it; as above I'm happy for that to change to per-PMU views.

> One thing I'm thinking is that we should split this per PMU at the
> hist_entry, so that we could show how many samples/events came from each
> of them...

That sounds sensible to me.

Mark.

> 
> - Arnaldo
>  
> > I don't have a strong feeling either way; I'm personally happy so long as
> > explicit pmu_name/event/ events don't get silently converted into
> > PERF_EVENT_TYPE_HARDWARE events, and as long as we correctly set the extended
> > HW type when we decide to use that.
> > 
> > Thanks,
> > Mark.
> > 
> > > Kan, I also noticed that the name of the event is:
> > > 
> > > 1K cpu_atom/cycles:P/                                                                                                                                                                         ◆
> > > 11K cpu_core/cycles:P/
> > > 
> > > If I try to use that on the command line:
> > > 
> > > root@number:~# perf top -e cpu_atom/cycles:P/
> > > event syntax error: 'cpu_atom/cycles:P/'
> > >                               \___ Bad event or PMU
> > > 
> > > Unable to find PMU or event on a PMU of 'cpu_atom'
> > > 
> > > Initial error:
> > > event syntax error: 'cpu_atom/cycles:P/'
> > >                               \___ unknown term 'cycles:P' for pmu 'cpu_atom'
> > > 
> > > valid terms: event,pc,edge,offcore_rsp,ldlat,inv,umask,cmask,config,config1,config2,config3,name,period,freq,branch_type,time,call-graph,stack-size,no-inherit,inherit,max-stack,nr,no-overwrite,overwrite,driver-config,percore,aux-output,aux-sample-size,metric-id,raw,legacy-cache,hardware
> > > Run 'perf list' for a list of valid events
> > > 
> > >  Usage: perf top [<options>]
> > > 
> > >     -e, --event <event>   event selector. use 'perf list' to list available events
> > > root@number:~#
> > > 
> > > It should be:
> > > 
> > >   "cpu_atom/cycles/P"
diff mbox series

Patch

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index ea8c7eca5eee..cce9350177e2 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1027,8 +1027,8 @@  static int perf_top__start_counters(struct perf_top *top)
 
 	evlist__for_each_entry(evlist, counter) {
 try_again:
-		if (evsel__open(counter, top->evlist->core.user_requested_cpus,
-				     top->evlist->core.threads) < 0) {
+		if (evsel__open(counter, counter->core.cpus,
+				counter->core.threads) < 0) {
 
 			/*
 			 * Specially handle overwrite fall back.