Re: [PATCH V2] perf top: Use evsel's cpus to replace user_requested_cpus

[Liang, Kan]

On 2023-12-12 8:06 p.m., Namhyung Kim wrote:
> On Tue, Dec 12, 2023 at 2:12 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>>
>> On Tue, Dec 12, 2023 at 1:25 PM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:
>>>
>>>
>>>
>>> On 2023-12-12 3:37 p.m., Ian Rogers wrote:
>>>> On Tue, Dec 12, 2023 at 11:39 AM <kan.liang@xxxxxxxxxxxxxxx> wrote:
>>>>>
>>>>> From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>>>>>
>>>>> perf top errors out on a hybrid machine
>>>>>  $perf top
>>>>>
>>>>>  Error:
>>>>>  The cycles:P event is not supported.
>>>>>
>>>>> The perf top expects that the "cycles" is collected on all CPUs in the
>>>>> system. But for hybrid there is no single "cycles" event which can cover
>>>>> all CPUs. Perf has to split it into two cycles events, e.g.,
>>>>> 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 of the above error out.
>>>>>
>>>>> Perf should only open the cycles event on the corresponding CPU. The
>>>>> commit ef91871c960e ("perf evlist: Propagate user CPU maps intersecting
>>>>> core PMU maps") 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 are also propagated to the evsel's threads in
>>>>> __perf_evlist__propagate_maps(). For a system-wide event, perf appends
>>>>> a dummy event and assign it to the evsel's threads. For a per-thread
>>>>> event, the evlist's thread_map is assigned to the evsel's threads. The
>>>>> same as the other tools, e.g., perf record, using the evsel's threads
>>>>> when opening an event.
>>>>>
>>>>> Reported-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>>>>> Closes: https://lore.kernel.org/linux-perf-users/ZXNnDrGKXbEELMXV@xxxxxxxxxx/
>>>>> Reviewed-by: Ian Rogers <irogers@xxxxxxxxxx>
>>>>> Signed-off-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>>>>> ---
>>>>>
>>>>> Changes since V1:
>>>>> - Update the description
>>>>> - Add Reviewed-by from Ian
>>>>
>>>> Thanks Kan, quick question. Does "perf top" on hybrid ask the user to
>>>> select between the cycles event on cpu_atom and cpu_core?
>>>
>>> Yes, but the event doesn't include the PMU information.
>>> We probably need a follow up patch to append the PMU name.
>>>
>>> Available samples
>>> 385 cycles:P
>>>
>>> 903 cycles:P
>>
>> Thanks and agreed, it isn't possible to tell which is which PMU/CPU
>> type at the moment. I tried the patch with perf top --stdio, there
>> wasn't a choice of event  

The perf top --stdio uses a dedicated display function, see
perf_top__header_snprintf() in util/top.c

It only shows one event at a time. "E" is used to switch the event.

>> and I can't tell what counter is being
>> displayed. 

For the hybrid case, I think we may display both PMU name and event
name. For example,

Available samples
656 cycles:P cpu_atom

701 cycles:P cpu_core

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index f4812b226818..afc7a1d54fe4 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -3433,8 +3433,10 @@ static void perf_evsel_menu__write(struct
ui_browser *browser,
        }

        nr_events = convert_unit(nr_events, &unit);
-       printed = scnprintf(bf, sizeof(bf), "%lu%c%s%s", nr_events,
-                          unit, unit == ' ' ? "" : " ", ev_name);
+       printed = scnprintf(bf, sizeof(bf), "%lu%c%s%s %s", nr_events,
+                          unit, unit == ' ' ? "" : " ", ev_name,
+                          evsel->pmu ? evsel->pmu_name : "");
+
        ui_browser__printf(browser, "%s", bf);

        nr_events = evsel->evlist->stats.nr_events[PERF_RECORD_LOST];


>> When I quit I also see:
>> ```
>> exiting.
>> corrupted double-linked list
>> Aborted (core dumped)
>> ```
>> but I wasn't able to repro this on a debuggable binary/system.

I haven't see the issue yet.

>>
>> If my memory serves there was a patch where perf top was showing >1
>> event. It would be nice here to do some kind of hybrid merging rather
>> than having to view each PMU's top separately.

The current perf top doesn't merge when there are >1 event.
sudo ./perf top -e "cycles,instructions"

Available samples
2K cycles

2K instructions

For now, I think we may just append a PMU name to distinguish the hybrid
case.

We may implement the merging feature which impacts both hybrid and
non-hybrid cases later separately.

> 
> Using event groups, but I noticed you removed the --group option.
> Maybe perf top can just use `{ ... }` notation for explicit grouping,
> but it might be implicit like in the hybrid case.
> 

Yes, if the events are from different PMUs, the perf tool will
implicitly de-group the hybrid events. "{ ... }" may not help here.

Thanks,
Kan