1. Patchew
  2. linux
  3. View series

[PATCH v3] perf evlist: Fix evlist__new_default() for > 1 core PMU

James Clark posted 1 patch 1 year, 11 months ago
There is a newer version of this series
tools/perf/util/evlist.c | 7 +++++++
1 file changed, 7 insertions(+)
[PATCH v3] perf evlist: Fix evlist__new_default() for > 1 core PMU
Posted by James Clark 1 year, 11 months ago 
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) {
+		struct evsel *evsel;
+
+		evlist__for_each_entry(evlist, evsel)
+			evsel__set_sample_id(evsel, /*can_sample_identifier=*/false);
+	}
+
 	return evlist;
 }
 
-- 
2.34.1
Re: [PATCH v3] perf evlist: Fix evlist__new_default() for > 1 core PMU
Posted by Namhyung Kim 1 year, 11 months ago 
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


> +               struct evsel *evsel;
> +
> +               evlist__for_each_entry(evlist, evsel)
> +                       evsel__set_sample_id(evsel, /*can_sample_identifier=*/false);
> +       }
> +
>         return evlist;
>  }
>
> --
> 2.34.1
>
Re: [PATCH v3] perf evlist: Fix evlist__new_default() for > 1 core PMU
Posted by James Clark 1 year, 11 months ago 

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
>>
Re: [PATCH v3] perf evlist: Fix evlist__new_default() for > 1 core PMU
Posted by Arnaldo Carvalho de Melo 1 year, 11 months ago 
Em Wed, Jan 24, 2024 at 09:03:57AM +0000, James Clark escreveu:
> On 24/01/2024 00:46, Namhyung Kim wrote:
> > On Tue, Jan 23, 2024 at 2:39 AM James Clark <james.clark@arm.com> wrote:
> >> +++ 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.
 
> Oops yes. Or just return on the error above.

Was there a v4? I'm assuming this is for perf-tools, i.e. for v6.8-rc,
right?

- Arnaldo
Re: [PATCH v3] perf evlist: Fix evlist__new_default() for > 1 core PMU
Posted by Namhyung Kim 1 year, 11 months ago 
On Fri, Jan 26, 2024 at 8:25 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Wed, Jan 24, 2024 at 09:03:57AM +0000, James Clark escreveu:
> > On 24/01/2024 00:46, Namhyung Kim wrote:
> > > On Tue, Jan 23, 2024 at 2:39 AM James Clark <james.clark@arm.com> wrote:
> > >> +++ 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.
>
> > Oops yes. Or just return on the error above.
>
> Was there a v4?

https://lore.kernel.org/linux-perf-users/20240124094358.489372-1-james.clark@arm.com/

> I'm assuming this is for perf-tools, i.e. for v6.8-rc, right?

It fixes 251aa040244a which is added to v6.5.  So I applied it
to perf-tools-next, do you want to take it to perf-tools?

Thanks,
Namhyung

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.