Re: [PATCH v2 2/2] libperf: evlist: Keep evsel idx contiguous on removal

[Ian Rogers <irogers@google.com>]

On Thu, Nov 14, 2024 at 8:07 AM James Clark <james.clark@linaro.org> wrote:
>
> The only two places where evsels are removed end up fixing up the index.
> Move it into libperf so that it's always done.
>
> Signed-off-by: James Clark <james.clark@linaro.org>

Reviewed-by: Ian Rogers <irogers@google.com>

but I was also reminded that the evsel idx is just a meaningless int
and we should stop trying to make it special, so I sent out some clean
up:
https://lore.kernel.org/lkml/20241114230713.330701-1-irogers@google.com/

You may get asked by the maintainers to separate the test from the
main logic to make patches with fewer lines of code ¯\_(ツ)_/¯

Thanks,
Ian

> ---
>  tools/lib/perf/evlist.c            | 17 +++++++------
>  tools/lib/perf/tests/test-evlist.c | 41 ++++++++++++++++++++++++++++++
>  tools/perf/builtin-record.c        | 18 +++----------
>  3 files changed, 53 insertions(+), 23 deletions(-)
>
> diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
> index 83c43dc13313..ffa1a28fb14f 100644
> --- a/tools/lib/perf/evlist.c
> +++ b/tools/lib/perf/evlist.c
> @@ -52,15 +52,8 @@ static void __perf_evlist__propagate_maps(struct perf_evlist *evlist,
>                  * Empty cpu lists would eventually get opened as "any" so remove
>                  * genuinely empty ones before they're opened in the wrong place.
>                  */
> -               if (perf_cpu_map__is_empty(evsel->cpus)) {
> -                       struct perf_evsel *next = perf_evlist__next(evlist, evsel);
> -
> +               if (perf_cpu_map__is_empty(evsel->cpus))
>                         perf_evlist__remove(evlist, evsel);
> -                       /* Keep idx contiguous */
> -                       if (next)
> -                               list_for_each_entry_from(next, &evlist->entries, node)
> -                                       next->idx--;
> -               }
>         } else if (!evsel->own_cpus || evlist->has_user_cpus ||
>                 (!evsel->requires_cpu && perf_cpu_map__has_any_cpu(evlist->user_requested_cpus))) {
>                 /*
> @@ -116,8 +109,16 @@ void perf_evlist__add(struct perf_evlist *evlist,
>  void perf_evlist__remove(struct perf_evlist *evlist,
>                          struct perf_evsel *evsel)
>  {
> +       struct perf_evsel *next = perf_evlist__next(evlist, evsel);
> +
>         list_del_init(&evsel->node);
>         evlist->nr_entries -= 1;
> +
> +       /* Keep idx contiguous */
> +       if (!next)
> +               return;
> +       list_for_each_entry_from(next, &evlist->entries, node)
> +               next->idx--;
>  }
>
>  struct perf_evlist *perf_evlist__new(void)
> diff --git a/tools/lib/perf/tests/test-evlist.c b/tools/lib/perf/tests/test-evlist.c
> index 10f70cb41ff1..06153820f408 100644
> --- a/tools/lib/perf/tests/test-evlist.c
> +++ b/tools/lib/perf/tests/test-evlist.c
> @@ -571,6 +571,46 @@ static int test_stat_multiplexing(void)
>         return 0;
>  }
>
> +static int test_list_remove(void)
> +{
> +       struct perf_evlist *evlist;
> +       struct perf_evsel *evsel, *evsel0, *evsel1, *evsel2;
> +       struct perf_event_attr attr = {};
> +       int idx = 0;
> +
> +       evlist = perf_evlist__new();
> +       __T("failed to create evlist", evlist);
> +
> +       evsel0 = perf_evsel__new(&attr);
> +       __T("failed to create evsel", evsel0);
> +       evsel1 = perf_evsel__new(&attr);
> +       __T("failed to create evsel", evsel1);
> +       evsel2 = perf_evsel__new(&attr);
> +       __T("failed to create evsel", evsel2);
> +
> +       perf_evlist__add(evlist, evsel0);
> +       perf_evlist__add(evlist, evsel1);
> +       perf_evlist__add(evlist, evsel2);
> +
> +       idx = 0;
> +       perf_evlist__for_each_evsel(evlist, evsel)
> +               __T("idx isn't contiguous", evsel->idx == idx++);
> +
> +       /* Test removing middle */
> +       perf_evlist__remove(evlist, evsel1);
> +       idx = 0;
> +       perf_evlist__for_each_evsel(evlist, evsel)
> +               __T("idx isn't contiguous", evsel->idx == idx++);
> +
> +       /* Test removing end */
> +       perf_evlist__remove(evlist, evsel2);
> +       idx = 0;
> +       perf_evlist__for_each_evsel(evlist, evsel)
> +               __T("idx isn't contiguous", evsel->idx == idx++);
> +
> +       return 0;
> +}
> +
>  int test_evlist(int argc, char **argv)
>  {
>         __T_START;
> @@ -583,6 +623,7 @@ int test_evlist(int argc, char **argv)
>         test_mmap_thread();
>         test_mmap_cpus();
>         test_stat_multiplexing();
> +       test_list_remove();
>
>         __T_END;
>         return tests_failed == 0 ? 0 : -1;
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 7e99743f7e42..2ebbb0fd0064 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -1359,14 +1359,13 @@ static int record__mmap(struct record *rec)
>  static int record__open(struct record *rec)
>  {
>         char msg[BUFSIZ];
> -       struct evsel *pos;
> +       struct evsel *pos, *tmp;
>         struct evlist *evlist = rec->evlist;
>         struct perf_session *session = rec->session;
>         struct record_opts *opts = &rec->opts;
>         int rc = 0;
> -       bool skipped = false;
>
> -       evlist__for_each_entry(evlist, pos) {
> +       evlist__for_each_entry_safe(evlist, tmp, pos) {
>  try_again:
>                 if (evsel__open(pos, pos->core.cpus, pos->core.threads) < 0) {
>                         if (evsel__fallback(pos, &opts->target, errno, msg, sizeof(msg))) {
> @@ -1383,23 +1382,12 @@ static int record__open(struct record *rec)
>                         evsel__open_strerror(pos, &opts->target, errno, msg, sizeof(msg));
>                         ui__error("Failure to open event '%s' on PMU '%s' which will be removed.\n%s\n",
>                                   evsel__name(pos), evsel__pmu_name(pos), msg);
> -                       pos->skippable = true;
> -                       skipped = true;
> +                       evlist__remove(evlist, pos);
>                 } else {
>                         pos->supported = true;
>                 }
>         }
>
> -       if (skipped) {
> -               struct evsel *tmp;
> -               int idx = 0;
> -
> -               evlist__for_each_entry_safe(evlist, tmp, pos) {
> -                       if (pos->skippable)
> -                               evlist__remove(evlist, pos);
> -                       pos->core.idx = idx++;
> -               }
> -       }
>         if (symbol_conf.kptr_restrict && !evlist__exclude_kernel(evlist)) {
>                 pr_warning(
>  "WARNING: Kernel address maps (/proc/{kallsyms,modules}) are restricted,\n"
> --
> 2.34.1
>