Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 24 Jan 2024 09:03:57 +0000 | Subject | Re: [PATCH v3] perf evlist: Fix evlist__new_default() for > 1 core PMU | From | James Clark <> |
| |
On 24/01/2024 00:46, Namhyung Kim wrote: > Hi James, > > On Tue, Jan 23, 2024 at 2:39 AM James Clark <james.clark@arm.com> wrote: >> >> The 'Session topology' test currently fails with this message when >> evlist__new_default() opens more than one event: >> >> 32: Session topology : >> --- start --- >> templ file: /tmp/perf-test-vv5YzZ >> Using CPUID 0x00000000410fd070 >> Opening: unknown-hardware:HG >> ------------------------------------------------------------ >> perf_event_attr: >> type 0 (PERF_TYPE_HARDWARE) >> config 0xb00000000 >> disabled 1 >> ------------------------------------------------------------ >> sys_perf_event_open: pid 0 cpu -1 group_fd -1 flags 0x8 = 4 >> Opening: unknown-hardware:HG >> ------------------------------------------------------------ >> perf_event_attr: >> type 0 (PERF_TYPE_HARDWARE) >> config 0xa00000000 >> disabled 1 >> ------------------------------------------------------------ >> sys_perf_event_open: pid 0 cpu -1 group_fd -1 flags 0x8 = 5 >> non matching sample_type >> FAILED tests/topology.c:73 can't get session >> ---- end ---- >> Session topology: FAILED! >> >> This is because when re-opening the file and parsing the header, Perf >> expects that any file that has more than one event has the sample ID >> flag set. Perf record already sets the flag in a similar way when there >> is more than one event, so add the same logic to evlist__new_default(). >> >> evlist__new_default() is only currently used in tests, so I don't >> expect this change to have any other side effects. The other tests that >> use it don't save and re-open the file so don't hit this issue. >> >> The session topology test has been failing on Arm big.LITTLE platforms >> since commit 251aa040244a ("perf parse-events: Wildcard most >> "numeric" events") when evlist__new_default() started opening multiple >> events for 'cycles'. >> >> Fixes: 251aa040244a ("perf parse-events: Wildcard most "numeric" events") >> Closes: https://lore.kernel.org/lkml/CAP-5=fWVQ-7ijjK3-w1q+k2WYVNHbAcejb-xY0ptbjRw476VKA@mail.gmail.com/ >> Tested-by: Ian Rogers <irogers@google.com> >> Reviewed-by: Ian Rogers <irogers@google.com> >> Tested-by: Kan Liang <kan.liang@linux.intel.com> >> Signed-off-by: James Clark <james.clark@arm.com> >> --- >> tools/perf/util/evlist.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> Changes since v2: >> >> * Undo the fact that v2 was accidentally based on v1 instead of >> perf-tools >> >> Changes since v1: >> >> * Reduce scope of evsel variable >> * Add argument label >> * Change summary to be less specific about the failing test >> * Add the closes: tag >> >> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c >> index 95f25e9fb994..979a6053a84d 100644 >> --- a/tools/perf/util/evlist.c >> +++ b/tools/perf/util/evlist.c >> @@ -106,6 +106,13 @@ struct evlist *evlist__new_default(void) >> evlist = NULL; >> } >> >> + if (evlist->core.nr_entries > 1) { > > I think you need a NULL check for evlist here. > > Thanks, > Namhyung > >
Oops yes. Or just return on the error above.
>> + struct evsel *evsel; >> + >> + evlist__for_each_entry(evlist, evsel) >> + evsel__set_sample_id(evsel, /*can_sample_identifier=*/false); >> + } >> + >> return evlist; >> } >> >> -- >> 2.34.1 >>
| ![\](/images/icornerr.gif) |