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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 13 Nov 2023 11:10:30 -0500
From:   "Liang, Kan" <kan.liang@...ux.intel.com>
To:     Ian Rogers <irogers@...gle.com>
Cc:     acme@...nel.org, linux-kernel@...r.kernel.org,
        linux-perf-users@...r.kernel.org, peterz@...radead.org,
        mingo@...hat.com, jolsa@...nel.org, namhyung@...nel.org,
        adrian.hunter@...el.com, tinghao.zhang@...el.com,
        Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH] perf evsel: Ignore the non-group case for branch counters



On 2023-11-13 10:49 a.m., Ian Rogers wrote:
> On Thu, Nov 9, 2023 at 8:41 AM <kan.liang@...ux.intel.com> wrote:
>>
>> From: Kan Liang <kan.liang@...ux.intel.com>
>>
>> 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@kernel.org/

Thanks,
Kan

> Ian
> 
>> Reported-by: Arnaldo Carvalho de Melo <acme@...hat.com>
>> Closes: https://lore.kernel.org/lkml/ZUv+G+w5EvJgQS45@kernel.org/
>> Signed-off-by: Kan Liang <kan.liang@...ux.intel.com>
>> ---
>>  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
>>