Re: [PATCH] perf evsel: Ignore the non-group case for branch counters

[Liang, Kan]

On 2023-11-13 10:49 a.m., Ian Rogers wrote:
> On Thu, Nov 9, 2023 at 8:41 AM <kan.liang@xxxxxxxxxxxxxxx> wrote:
>>
>> From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>>
>> The perf test 27: Sample parsing fails with the branch counters support
>> introduced.
>>
>> The branch counters feature requires all the events to belong to a
>> group. There is no problem with the normal perf usage which usually
>> initializes an evlist even for a single evsel.
>> But the perf test is special, which may not initialize an evlist. The
>> Sample parsing test case is one of the examples. The existing code
>> crashes with the !evsel->evlist.
>>
>> Non-group means the evsel doesn't have branch counters support.
> 
> Thanks Kan, do we need to add this condition to
> parse_events__sort_events_and_fix_groups?
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/parse-events.c#n2174
> 

I don't think so. The "non-group" here means the case that the evlist is
not initialized. It should only happen with some perf test case.

In the parse_events__sort_events_and_fix_groups(), IIUC, the ungrouped
event should mean a single event group. The feature works with the case.
We don't need to add the condition.

BTW: Arnaldo should have already folded it with the original patch. We
don't need the patch anymore.
https://lore.kernel.org/lkml/ZU0pGuUBJH+bF1yU@xxxxxxxxxx/

Thanks,
Kan

> Ian
> 
>> Reported-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>> Closes: https://lore.kernel.org/lkml/ZUv+G+w5EvJgQS45@xxxxxxxxxx/
>> Signed-off-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>> ---
>>  tools/perf/util/evsel.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>> index 58a9b8c82790..7a6a2d1f96db 100644
>> --- a/tools/perf/util/evsel.c
>> +++ b/tools/perf/util/evsel.c
>> @@ -2355,6 +2355,10 @@ static inline bool evsel__has_branch_counters(const struct evsel *evsel)
>>  {
>>         struct evsel *cur, *leader = evsel__leader(evsel);
>>
>> +       /* The branch counters feature only supports group */
>> +       if (!leader || !evsel->evlist)
>> +               return false;
>> +
>>         evlist__for_each_entry(evsel->evlist, cur) {
>>                 if ((leader == evsel__leader(cur)) &&
>>                     (cur->core.attr.branch_sample_type & PERF_SAMPLE_BRANCH_COUNTERS))
>> --
>> 2.35.1
>>