2024-01-22 17:09:13

by James Clark

[permalink] [raw]
Subject: [PATCH] perf test: Fix session topology test on heterogeneous systems

The 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 session 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 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")
Signed-off-by: James Clark <[email protected]>
---
tools/perf/util/evlist.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 95f25e9fb994..56db37fac6f6 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -95,6 +95,7 @@ struct evlist *evlist__new_default(void)
struct evlist *evlist = evlist__new();
bool can_profile_kernel;
int err;
+ struct evsel *evsel;

if (!evlist)
return NULL;
@@ -106,6 +107,10 @@ struct evlist *evlist__new_default(void)
evlist = NULL;
}

+ if (evlist->core.nr_entries > 1)
+ evlist__for_each_entry(evlist, evsel)
+ evsel__set_sample_id(evsel, false);
+
return evlist;
}

--
2.34.1



2024-01-22 17:12:43

by James Clark

[permalink] [raw]
Subject: Re: [PATCH] perf test: Fix session topology test on heterogeneous systems



On 22/01/2024 15:54, James Clark wrote:
> The 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 session ID

session ID -> sample ID

2024-01-22 17:41:44

by Liang, Kan

[permalink] [raw]
Subject: Re: [PATCH] perf test: Fix session topology test on heterogeneous systems



On 2024-01-22 10:54 a.m., James Clark wrote:
> The 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 session 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 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")
> Signed-off-by: James Clark <[email protected]>
Verified on an Intel hybrid machine.

Tested-by: Kan Liang <[email protected]>

Thanks,
Kan


> ---
> tools/perf/util/evlist.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 95f25e9fb994..56db37fac6f6 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -95,6 +95,7 @@ struct evlist *evlist__new_default(void)
> struct evlist *evlist = evlist__new();
> bool can_profile_kernel;
> int err;
> + struct evsel *evsel;
>
> if (!evlist)
> return NULL;
> @@ -106,6 +107,10 @@ struct evlist *evlist__new_default(void)
> evlist = NULL;
> }
>
> + if (evlist->core.nr_entries > 1)
> + evlist__for_each_entry(evlist, evsel)
> + evsel__set_sample_id(evsel, false);
> +
> return evlist;
> }
>

2024-01-22 17:46:11

by Ian Rogers

[permalink] [raw]
Subject: Re: [PATCH] perf test: Fix session topology test on heterogeneous systems

Hi James, I think the subject should be something like "perf evlist:
Fix new_default for >1 core PMU" as the change will apply more widely
than just the test. The test failure fix can be in the subject. You
could add a:

Closes: https://lore.kernel.org/lkml/CAP-5=fWVQ-7ijjK3-w1q+k2WYVNHbAcejb-xY0ptbjRw476VKA@mail.gmail.com/

On Mon, Jan 22, 2024 at 7:55 AM James Clark <[email protected]> wrote:
>
> The 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 session 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 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")
> Signed-off-by: James Clark <[email protected]>
> ---
> tools/perf/util/evlist.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 95f25e9fb994..56db37fac6f6 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -95,6 +95,7 @@ struct evlist *evlist__new_default(void)
> struct evlist *evlist = evlist__new();
> bool can_profile_kernel;
> int err;
> + struct evsel *evsel;
>
> if (!evlist)
> return NULL;
> @@ -106,6 +107,10 @@ struct evlist *evlist__new_default(void)
> evlist = NULL;
> }
>
> + if (evlist->core.nr_entries > 1)
> + evlist__for_each_entry(evlist, evsel)
> + evsel__set_sample_id(evsel, false);
> +

nit: the if should have curlies, with them we can reduce the scope of
evsel like below. It is also nice for constants to name the arguments
[1].

if (evlist->core.nr_entries > 1) {
struct evsel *evsel;

evlist__for_each_entry(evlist, evsel)
evsel__set_sample_id(evsel, /*can_sample_identifier=*/false);
}

Tested-by: Ian Rogers <[email protected]>
(also Reviewed-by)

When testing with this with Mark's change [2] I see on alderlake two failures:
```
irogers@alderlake:~$ perf test 74 -vv
Couldn't bump rlimit(MEMLOCK), failures may take place when creating
BPF maps, etc
74: daemon operations :
--- start ---
test child forked, pid 553821
test daemon list
test daemon reconfig
test daemon stop
test daemon signal
signal 12 sent to session 'test [554082]'
signal 12 sent to session 'test [554082]'
FAILED: perf data no generated
test daemon ping
test daemon lock
test child finished with -1
---- end ----
daemon operations: FAILED!
irogers@alderlake:~$ perf test 76 -vv
Couldn't bump rlimit(MEMLOCK), failures may take place when creating
BPF maps, etc
76: perf list tests :
--- start ---
test child forked, pid 554167
Json output test
Expecting ',' delimiter: line 4971 column 2 (char 243497)
test child finished with -1
---- end ----
perf list tests: FAILED!
```
So I think this patch may be exposing other latent issues. I'll try to
take a look.

Another thought, rather than having an evlist validate we should just
assert the evlist is always in a good shape whenever it is mutated.
That would have avoided this bug as the code would have blown up
early.

Thanks,
Ian

[1] https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html
[2] https://lore.kernel.org/lkml/[email protected]/

> return evlist;
> }


>
> --
> 2.34.1
>

2024-01-23 10:27:48

by James Clark

[permalink] [raw]
Subject: [PATCH v2] perf evlist: Fix evlist__new_default() for > 1 core PMU

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 <[email protected]>
Reviewed-by: Ian Rogers <[email protected]>
Tested-by: Kan Liang <[email protected]>
Signed-off-by: James Clark <[email protected]>
---
tools/perf/util/evlist.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

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 56db37fac6f6..979a6053a84d 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -95,7 +95,6 @@ struct evlist *evlist__new_default(void)
struct evlist *evlist = evlist__new();
bool can_profile_kernel;
int err;
- struct evsel *evsel;

if (!evlist)
return NULL;
@@ -107,9 +106,12 @@ struct evlist *evlist__new_default(void)
evlist = NULL;
}

- if (evlist->core.nr_entries > 1)
+ if (evlist->core.nr_entries > 1) {
+ struct evsel *evsel;
+
evlist__for_each_entry(evlist, evsel)
- evsel__set_sample_id(evsel, false);
+ evsel__set_sample_id(evsel, /*can_sample_identifier=*/false);
+ }

return evlist;
}
--
2.34.1


2024-01-23 10:39:32

by James Clark

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

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 <[email protected]>
Reviewed-by: Ian Rogers <[email protected]>
Tested-by: Kan Liang <[email protected]>
Signed-off-by: James Clark <[email protected]>
---
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


2024-01-24 00:47:04

by Namhyung Kim

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

Hi James,

On Tue, Jan 23, 2024 at 2:39 AM James Clark <[email protected]> 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 <[email protected]>
> Reviewed-by: Ian Rogers <[email protected]>
> Tested-by: Kan Liang <[email protected]>
> Signed-off-by: James Clark <[email protected]>
> ---
> 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
>

2024-01-24 09:04:15

by James Clark

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



On 24/01/2024 00:46, Namhyung Kim wrote:
> Hi James,
>
> On Tue, Jan 23, 2024 at 2:39 AM James Clark <[email protected]> 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 <[email protected]>
>> Reviewed-by: Ian Rogers <[email protected]>
>> Tested-by: Kan Liang <[email protected]>
>> Signed-off-by: James Clark <[email protected]>
>> ---
>> 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
>>

2024-01-26 16:25:51

by Arnaldo Carvalho de Melo

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

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 <[email protected]> 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

2024-01-26 20:50:07

by Namhyung Kim

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

On Fri, Jan 26, 2024 at 8:25 AM Arnaldo Carvalho de Melo
<[email protected]> 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 <[email protected]> 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/[email protected]/

> 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