| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAP-5=fVdNov38srPj-ax-PHtnMBhGT+y+fp+NJHGFS0dxRchew@mail.gmail.com>
Date: Thu, 14 Nov 2024 15:35:35 -0800
From: Ian Rogers <irogers@...gle.com>
To: James Clark <james.clark@...aro.org>
Cc: linux-perf-users@...r.kernel.org, namhyung@...nel.org,
thomas.falcon@...el.com, Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>, Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>, "Liang, Kan" <kan.liang@...ux.intel.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] libperf: evlist: Keep evsel idx contiguous on removal
On Thu, Nov 14, 2024 at 8:07 AM James Clark <james.clark@...aro.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@...aro.org>
Reviewed-by: Ian Rogers <irogers@...gle.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
>
Powered by blists - more mailing lists