2022-04-22 19:11:50

by Adrian Hunter

Subject: [PATCH RFC 05/21] perf auxtrace: Do not mix up mmap idx

The idx is with respect to evlist not evsel. That hasn't mattered because
they are the same at present. Prepare for that not being the case, which it
won't be when sideband tracking events are allowed on all CPUs even when
auxtrace is limited to selected CPUs.

Signed-off-by: Adrian Hunter <[email protected]>
---
tools/perf/util/auxtrace.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
index 10936a38031f..2d015b0be549 100644
--- a/tools/perf/util/auxtrace.c
+++ b/tools/perf/util/auxtrace.c
@@ -640,8 +640,14 @@ static int evlist__enable_event_idx(struct evlist *evlist, struct evsel *evsel,
{
bool per_cpu_mmaps = !perf_cpu_map__empty(evlist->core.user_requested_cpus);

- if (per_cpu_mmaps)
- return perf_evsel__enable_cpu(&evsel->core, idx);
+ if (per_cpu_mmaps) {
+ struct perf_cpu evlist_cpu = perf_cpu_map__cpu(evlist->core.all_cpus, idx);
+ int cpu = perf_cpu_map__idx(evsel->core.cpus, evlist_cpu);
+
+ if (cpu == -1)
+ return -EINVAL;
+ return perf_evsel__enable_cpu(&evsel->core, cpu);
+ }

return perf_evsel__enable_thread(&evsel->core, idx);
}
--
2.25.1

2022-04-27 22:52:08

by Namhyung Kim

Subject: Re: [PATCH RFC 05/21] perf auxtrace: Do not mix up mmap idx

On Fri, Apr 22, 2022 at 9:24 AM Adrian Hunter <[email protected]> wrote:
>
> The idx is with respect to evlist not evsel. That hasn't mattered because
> they are the same at present. Prepare for that not being the case, which it
> won't be when sideband tracking events are allowed on all CPUs even when
> auxtrace is limited to selected CPUs.
>
> Signed-off-by: Adrian Hunter <[email protected]>
> ---
> tools/perf/util/auxtrace.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> index 10936a38031f..2d015b0be549 100644
> --- a/tools/perf/util/auxtrace.c
> +++ b/tools/perf/util/auxtrace.c
> @@ -640,8 +640,14 @@ static int evlist__enable_event_idx(struct evlist *evlist, struct evsel *evsel,
> {
> bool per_cpu_mmaps = !perf_cpu_map__empty(evlist->core.user_requested_cpus);
>
> - if (per_cpu_mmaps)
> - return perf_evsel__enable_cpu(&evsel->core, idx);
> + if (per_cpu_mmaps) {
> + struct perf_cpu evlist_cpu = perf_cpu_map__cpu(evlist->core.all_cpus, idx);
> + int cpu = perf_cpu_map__idx(evsel->core.cpus, evlist_cpu);

While it can be thought of as an index from the function name,
it'd be nice if we could be explicit like cpu_map_idx.

Thanks,
Namhyung

> +
> + if (cpu == -1)
> + return -EINVAL;
> + return perf_evsel__enable_cpu(&evsel->core, cpu);
> + }
>
> return perf_evsel__enable_thread(&evsel->core, idx);
> }
> --
> 2.25.1
>

2022-04-28 11:24:57

by Adrian Hunter

Subject: Re: [PATCH RFC 05/21] perf auxtrace: Do not mix up mmap idx

On 28/04/22 00:54, Namhyung Kim wrote:
> On Fri, Apr 22, 2022 at 9:24 AM Adrian Hunter <[email protected]> wrote:
>>
>> The idx is with respect to evlist not evsel. That hasn't mattered because
>> they are the same at present. Prepare for that not being the case, which it
>> won't be when sideband tracking events are allowed on all CPUs even when
>> auxtrace is limited to selected CPUs.
>>
>> Signed-off-by: Adrian Hunter <[email protected]>
>> ---
>> tools/perf/util/auxtrace.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
>> index 10936a38031f..2d015b0be549 100644
>> --- a/tools/perf/util/auxtrace.c
>> +++ b/tools/perf/util/auxtrace.c
>> @@ -640,8 +640,14 @@ static int evlist__enable_event_idx(struct evlist *evlist, struct evsel *evsel,
>> {
>> bool per_cpu_mmaps = !perf_cpu_map__empty(evlist->core.user_requested_cpus);
>>
>> - if (per_cpu_mmaps)
>> - return perf_evsel__enable_cpu(&evsel->core, idx);
>> + if (per_cpu_mmaps) {
>> + struct perf_cpu evlist_cpu = perf_cpu_map__cpu(evlist->core.all_cpus, idx);
>> + int cpu = perf_cpu_map__idx(evsel->core.cpus, evlist_cpu);
>
> While it can be thought of as an index from the function name,
> it'd be nice if we could be explicit like cpu_map_idx.

Ok

>
> Thanks,
> Namhyung
>
>> +
>> + if (cpu == -1)
>> + return -EINVAL;
>> + return perf_evsel__enable_cpu(&evsel->core, cpu);
>> + }
>>
>> return perf_evsel__enable_thread(&evsel->core, idx);
>> }
>> --
>> 2.25.1
>>