Re: [PATCH v2] perf evlist: Fix 'perf record -C xx' failed issue

[Ian Rogers <irogers@google.com>]

On Tue, Mar 19, 2024 at 2:14 AM Chunxin Zang <spring.cxz@gmail.com> wrote:
>
> The cpu has 8 performance-cores and 8 efficient-cores on my pc.
> 0-15 are performance-cores
> 16-23 are 8 efficient-cores
>
> When I run "perf record -C xxx", the command fails if xxx all belong to
> performance cores or efficient cores
>
> The results are as follows
>
>   # perf record -C 12
>   WARNING: A requested CPU in '12' is not supported by PMU 'cpu_atom' (CPUs 16-23) for event 'cycles:P'
>   Error:
>   The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (cpu_atom/cycles:P/).
>   /bin/dmesg | grep -i perf may provide additional information.
>
>   # perf record -C 14-17
>   WARNING: A requested CPU in '14-17' is not supported by PMU 'cpu_atom' (CPUs 16-23) for event 'cycles:P'
>   WARNING: A requested CPU in '14-17' is not supported by PMU 'cpu_core' (CPUs 0-15) for event 'cycles:P'
>   ^C[ perf record: Woken up 1 times to write data ]
>
> The reason is that the cpu_map of 'cpu_atom'-evsel is empty, causing
> the sys_perf_event_open() result to fail.
>
> Changes in v2:
> - fix memory leak about 'intersect'
>
> Signed-off-by: Chunxin Zang <spring.cxz@gmail.com>
> Reviewed-by: Chen Yang <yangchen11@lixiang.com>
> ---
>  tools/perf/util/evlist.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 55a300a0977b..babbde29341f 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -2499,7 +2499,7 @@ void evlist__check_mem_load_aux(struct evlist *evlist)
>  void evlist__warn_user_requested_cpus(struct evlist *evlist, const char *cpu_list)
>  {
>         struct perf_cpu_map *user_requested_cpus;
> -       struct evsel *pos;
> +       struct evsel *pos, *tmp;
>
>         if (!cpu_list)
>                 return;
> @@ -2508,18 +2508,28 @@ void evlist__warn_user_requested_cpus(struct evlist *evlist, const char *cpu_lis
>         if (!user_requested_cpus)
>                 return;
>
> -       evlist__for_each_entry(evlist, pos) {
> +       evlist__for_each_entry_safe(evlist, tmp, pos) {
>                 struct perf_cpu_map *intersect, *to_test;
>                 const struct perf_pmu *pmu = evsel__find_pmu(pos);
>
>                 to_test = pmu && pmu->is_core ? pmu->cpus : cpu_map__online();

Perhaps this would be clearer if we just made sure requested uncore
CPUs were online. Uncore cpu maps are weird, they say the default CPU
but other CPUs are valid. It'd be worth testing the impact of this
change on uncore events.

>                 intersect = perf_cpu_map__intersect(to_test, user_requested_cpus);
> -               if (!perf_cpu_map__equal(intersect, user_requested_cpus)) {
> -                       char buf[128];
> +               if (!intersect) {
> +                       evlist__remove(evlist, pos);
> +                       evsel__delete(pos);
> +                       perf_cpu_map__put(intersect);

evlist__warn_user_requested_cpus seems like a wrong function name if
evsels are being removed. Perhaps something like
evlist__remove_empty_cpu_map_evsels. It seems this change will remove
warnings in cases like:

$ perf record cpu_atom/cycles/ -C 0 ...

I wonder that we need a flag in the evsel to say when an event comes
from wildcard expansion so we only don't warn/remove in that case.

Wrt memory leaks, try compiling with EXTRA_CFLAGS="-fsanitize=address"
which also incorporates leak sanitizer.

Thanks,
Ian

> +                       continue;
> +               }
> +
> +               if (!perf_cpu_map__is_subset(user_requested_cpus, intersect)) {
> +                       char buf_test[128];
> +                       char buf_intersect[128];
>
> -                       cpu_map__snprint(to_test, buf, sizeof(buf));
> -                       pr_warning("WARNING: A requested CPU in '%s' is not supported by PMU '%s' (CPUs %s) for event '%s'\n",
> -                               cpu_list, pmu ? pmu->name : "cpu", buf, evsel__name(pos));
> +                       cpu_map__snprint(to_test, buf_test, sizeof(buf_test));
> +                       cpu_map__snprint(intersect, buf_intersect, sizeof(buf_intersect));
> +                       pr_warning("WARNING: A requested CPU '%s' in '%s' is not supported by "
> +                                  "PMU '%s' (CPUs %s) for event '%s'\n", buf_intersect, cpu_list,
> +                                  pmu ? pmu->name : "cpu", buf_test, evsel__name(pos));
>                 }
>                 perf_cpu_map__put(intersect);
>         }
> --
> 2.34.1
>