diff mbox series

perf evsel: Ignore the non-group case for branch counters

Message ID 20231109164007.2037721-1-kan.liang@linux.intel.com
State New
Headers
Series perf evsel: Ignore the non-group case for branch counters |

Commit Message

Liang, Kan Nov. 9, 2023, 4:40 p.m. UTC 
  From: Kan Liang <kan.liang@linux.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.

Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Closes: https://lore.kernel.org/lkml/ZUv+G+w5EvJgQS45@kernel.org/
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
 tools/perf/util/evsel.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Ian Rogers Nov. 13, 2023, 3:49 p.m. UTC | #1 
On Thu, Nov 9, 2023 at 8:41 AM <kan.liang@linux.intel.com> wrote:
>
> From: Kan Liang <kan.liang@linux.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

Ian

> Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> Closes: https://lore.kernel.org/lkml/ZUv+G+w5EvJgQS45@kernel.org/
> Signed-off-by: Kan Liang <kan.liang@linux.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
>
Liang, Kan Nov. 13, 2023, 4:10 p.m. UTC | #2 
On 2023-11-13 10:49 a.m., Ian Rogers wrote:
> On Thu, Nov 9, 2023 at 8:41 AM <kan.liang@linux.intel.com> wrote:
>>
>> From: Kan Liang <kan.liang@linux.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@redhat.com>
>> Closes: https://lore.kernel.org/lkml/ZUv+G+w5EvJgQS45@kernel.org/
>> Signed-off-by: Kan Liang <kan.liang@linux.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
>>
diff mbox series

Patch

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))