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>
---
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.1On 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
>
© 2016 - 2024 Red Hat, Inc.