From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Andi Kleen <ak@linux.intel.com>
Cc: Riccardo Mancini <rickyman7@gmail.com>,
Ian Rogers <irogers@google.com>,
Namhyung Kim <namhyung@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Mark Rutland <mark.rutland@arm.com>, Jiri Olsa <jolsa@redhat.com>,
linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org
Subject: Re: [RFC PATCH v1 03/37] perf evlist: replace evsel__cpu_iter* functions with evsel__find_cpu()
Date: Fri, 8 Oct 2021 11:38:26 -0300 [thread overview]
Message-ID: <YWBX4slh2U7yNahE@kernel.org> (raw)
In-Reply-To: <5bfe13dd0985611285bb697987816ddc36e93e76.1629490974.git.rickyman7@gmail.com>
Em Sat, Aug 21, 2021 at 11:19:09AM +0200, Riccardo Mancini escreveu:
> Commit a8cbe40fe9f4ba49 ("perf evsel: Add iterator to iterate over events
> ordered by CPU") introduced an iterator for evsel.core.cpus inside evsel
> which is used to iterate over evlist one CPU at a time.
>
> However, this solution is hacky since it involves a mutable state in the
> evsel which is supposed to be unmodified during iteration.
> Sice checking that a CPU is within the evsel can be done quickly in
> O(logn) time, this patch replaces the aforementioned iterator with a
> simple check.
Andi, can you take a look at this and provide your Reviewed-by, please?
- Arnaldo
> Cc: Andi Kleen <ak@linux.intel.com>
> Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
> ---
> tools/perf/builtin-stat.c | 24 +++++++++--------
> tools/perf/util/evlist.c | 54 +++++++++++----------------------------
> tools/perf/util/evlist.h | 5 +---
> tools/perf/util/evsel.h | 1 -
> 4 files changed, 30 insertions(+), 54 deletions(-)
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 84de61795e67bbb9..62dcc001c8f0a78b 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -407,7 +407,7 @@ static int read_affinity_counters(struct timespec *rs)
> {
> struct evsel *counter;
> struct affinity affinity;
> - int i, ncpus, cpu;
> + int i, ncpus, cpu, cpu_idx;
>
> if (all_counters_use_bpf)
> return 0;
> @@ -424,13 +424,14 @@ static int read_affinity_counters(struct timespec *rs)
> affinity__set(&affinity, cpu);
>
> evlist__for_each_entry(evsel_list, counter) {
> - if (evsel__cpu_iter_skip(counter, cpu))
> + cpu_idx = evsel__find_cpu(counter, cpu);
> + if (cpu_idx < 0)
> continue;
> if (evsel__is_bpf(counter))
> continue;
> if (!counter->err) {
> counter->err = read_counter_cpu(counter, rs,
> - counter->cpu_iter - 1);
> + cpu_idx);
> }
> }
> }
> @@ -789,7 +790,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> const bool forks = (argc > 0);
> bool is_pipe = STAT_RECORD ? perf_stat.data.is_pipe : false;
> struct affinity affinity;
> - int i, cpu, err;
> + int i, cpu, cpu_idx, err;
> bool second_pass = false;
>
> if (forks) {
> @@ -823,7 +824,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> affinity__set(&affinity, cpu);
>
> evlist__for_each_entry(evsel_list, counter) {
> - if (evsel__cpu_iter_skip(counter, cpu))
> + cpu_idx = evsel__find_cpu(counter, cpu);
> + if (cpu_idx < 0)
> continue;
> if (counter->reset_group || counter->errored)
> continue;
> @@ -831,7 +833,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> continue;
> try_again:
> if (create_perf_stat_counter(counter, &stat_config, &target,
> - counter->cpu_iter - 1) < 0) {
> + cpu_idx) < 0) {
>
> /*
> * Weak group failed. We cannot just undo this here
> @@ -877,22 +879,24 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> evlist__for_each_entry(evsel_list, counter) {
> if (!counter->reset_group && !counter->errored)
> continue;
> - if (evsel__cpu_iter_skip_no_inc(counter, cpu))
> + cpu_idx = evsel__find_cpu(counter, cpu);
> + if (cpu_idx < 0)
> continue;
> - perf_evsel__close_cpu(&counter->core, counter->cpu_iter);
> + perf_evsel__close_cpu(&counter->core, cpu_idx);
> }
> /* Now reopen weak */
> evlist__for_each_entry(evsel_list, counter) {
> if (!counter->reset_group && !counter->errored)
> continue;
> - if (evsel__cpu_iter_skip(counter, cpu))
> + cpu_idx = evsel__find_cpu(counter, cpu);
> + if (cpu_idx < 0)
> continue;
> if (!counter->reset_group)
> continue;
> try_again_reset:
> pr_debug2("reopening weak %s\n", evsel__name(counter));
> if (create_perf_stat_counter(counter, &stat_config, &target,
> - counter->cpu_iter - 1) < 0) {
> + cpu_idx) < 0) {
>
> switch (stat_handle_error(counter)) {
> case COUNTER_FATAL:
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 47581a237c7a7848..3d55d9a53b9f4875 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -341,36 +341,9 @@ static int evlist__nr_threads(struct evlist *evlist, struct evsel *evsel)
> return perf_thread_map__nr(evlist->core.threads);
> }
>
> -void evlist__cpu_iter_start(struct evlist *evlist)
> +int evsel__find_cpu(struct evsel *ev, int cpu)
> {
> - struct evsel *pos;
> -
> - /*
> - * Reset the per evsel cpu_iter. This is needed because
> - * each evsel's cpumap may have a different index space,
> - * and some operations need the index to modify
> - * the FD xyarray (e.g. open, close)
> - */
> - evlist__for_each_entry(evlist, pos)
> - pos->cpu_iter = 0;
> -}
> -
> -bool evsel__cpu_iter_skip_no_inc(struct evsel *ev, int cpu)
> -{
> - if (ev->cpu_iter >= ev->core.cpus->nr)
> - return true;
> - if (cpu >= 0 && ev->core.cpus->map[ev->cpu_iter] != cpu)
> - return true;
> - return false;
> -}
> -
> -bool evsel__cpu_iter_skip(struct evsel *ev, int cpu)
> -{
> - if (!evsel__cpu_iter_skip_no_inc(ev, cpu)) {
> - ev->cpu_iter++;
> - return false;
> - }
> - return true;
> + return perf_cpu_map__idx(ev->core.cpus, cpu);
> }
>
> static int evsel__strcmp(struct evsel *pos, char *evsel_name)
> @@ -400,7 +373,7 @@ static void __evlist__disable(struct evlist *evlist, char *evsel_name)
> {
> struct evsel *pos;
> struct affinity affinity;
> - int cpu, i, imm = 0;
> + int cpu, i, imm = 0, cpu_idx;
> bool has_imm = false;
>
> if (affinity__setup(&affinity) < 0)
> @@ -414,7 +387,8 @@ static void __evlist__disable(struct evlist *evlist, char *evsel_name)
> evlist__for_each_entry(evlist, pos) {
> if (evsel__strcmp(pos, evsel_name))
> continue;
> - if (evsel__cpu_iter_skip(pos, cpu))
> + cpu_idx = evsel__find_cpu(pos, cpu);
> + if (cpu_idx < 0)
> continue;
> if (pos->disabled || !evsel__is_group_leader(pos) || !pos->core.fd)
> continue;
> @@ -422,7 +396,7 @@ static void __evlist__disable(struct evlist *evlist, char *evsel_name)
> has_imm = true;
> if (pos->immediate != imm)
> continue;
> - evsel__disable_cpu(pos, pos->cpu_iter - 1);
> + evsel__disable_cpu(pos, cpu_idx);
> }
> }
> if (!has_imm)
> @@ -462,7 +436,7 @@ static void __evlist__enable(struct evlist *evlist, char *evsel_name)
> {
> struct evsel *pos;
> struct affinity affinity;
> - int cpu, i;
> + int cpu, i, cpu_idx;
>
> if (affinity__setup(&affinity) < 0)
> return;
> @@ -473,11 +447,12 @@ static void __evlist__enable(struct evlist *evlist, char *evsel_name)
> evlist__for_each_entry(evlist, pos) {
> if (evsel__strcmp(pos, evsel_name))
> continue;
> - if (evsel__cpu_iter_skip(pos, cpu))
> + cpu_idx = evsel__find_cpu(pos, cpu);
> + if (cpu_idx < 0)
> continue;
> if (!evsel__is_group_leader(pos) || !pos->core.fd)
> continue;
> - evsel__enable_cpu(pos, pos->cpu_iter - 1);
> + evsel__enable_cpu(pos, cpu_idx);
> }
> }
> affinity__cleanup(&affinity);
> @@ -1264,7 +1239,7 @@ void evlist__close(struct evlist *evlist)
> {
> struct evsel *evsel;
> struct affinity affinity;
> - int cpu, i;
> + int cpu, i, cpu_idx;
>
> /*
> * With perf record core.cpus is usually NULL.
> @@ -1282,9 +1257,10 @@ void evlist__close(struct evlist *evlist)
> affinity__set(&affinity, cpu);
>
> evlist__for_each_entry_reverse(evlist, evsel) {
> - if (evsel__cpu_iter_skip(evsel, cpu))
> - continue;
> - perf_evsel__close_cpu(&evsel->core, evsel->cpu_iter - 1);
> + cpu_idx = evsel__find_cpu(evsel, cpu);
> + if (cpu_idx < 0)
> + continue;
> + perf_evsel__close_cpu(&evsel->core, cpu_idx);
> }
> }
> affinity__cleanup(&affinity);
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index 5c22383489ae4905..fde893170c7ba6d2 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -310,15 +310,12 @@ void evlist__to_front(struct evlist *evlist, struct evsel *move_evsel);
> __evlist__for_each_entry_safe(&(evlist)->core.entries, tmp, evsel)
>
> #define evlist__for_each_cpu(evlist, index, cpu) \
> - evlist__cpu_iter_start(evlist); \
> perf_cpu_map__for_each_cpu (cpu, index, (evlist)->core.all_cpus)
>
> struct evsel *evlist__get_tracking_event(struct evlist *evlist);
> void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_evsel);
>
> -void evlist__cpu_iter_start(struct evlist *evlist);
> -bool evsel__cpu_iter_skip(struct evsel *ev, int cpu);
> -bool evsel__cpu_iter_skip_no_inc(struct evsel *ev, int cpu);
> +int evsel__find_cpu(struct evsel *ev, int cpu);
>
> struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str);
>
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 80383096d51c5f00..eabccce406886320 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -119,7 +119,6 @@ struct evsel {
> bool errored;
> struct hashmap *per_pkg_mask;
> int err;
> - int cpu_iter;
> struct {
> evsel__sb_cb_t *cb;
> void *data;
> --
> 2.31.1
--
- Arnaldo
next prev parent reply other threads:[~2021-10-08 14:38 UTC|newest]
Thread overview: 63+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-21 9:19 [RFC PATCH v1 00/37] perf: use workqueue for evlist operations Riccardo Mancini
2021-08-21 9:19 ` [RFC PATCH v1 01/37] libperf cpumap: improve idx function Riccardo Mancini
2021-08-31 18:46 ` Arnaldo Carvalho de Melo
2021-10-08 14:29 ` Arnaldo Carvalho de Melo
2021-08-21 9:19 ` [RFC PATCH v1 02/37] libperf cpumap: improve max function Riccardo Mancini
2021-08-31 18:47 ` Arnaldo Carvalho de Melo
2021-08-31 19:16 ` Arnaldo Carvalho de Melo
2021-08-21 9:19 ` [RFC PATCH v1 03/37] perf evlist: replace evsel__cpu_iter* functions with evsel__find_cpu Riccardo Mancini
2021-10-08 14:38 ` Arnaldo Carvalho de Melo [this message]
2021-12-11 0:20 ` Ian Rogers
2021-08-21 9:19 ` [RFC PATCH v1 04/37] perf util: add mmap_cpu_mask__duplicate function Riccardo Mancini
2021-08-31 19:21 ` Arnaldo Carvalho de Melo
2021-08-21 9:19 ` [RFC PATCH v1 05/37] perf util/mmap: add missing bitops.h header Riccardo Mancini
2021-08-31 19:22 ` Arnaldo Carvalho de Melo
2021-08-21 9:19 ` [RFC PATCH v1 06/37] perf workqueue: add affinities to threadpool Riccardo Mancini
2021-08-21 9:19 ` [RFC PATCH v1 07/37] perf workqueue: add support for setting affinities to workers Riccardo Mancini
2021-08-21 9:19 ` [RFC PATCH v1 08/37] perf workqueue: add method to execute work on specific CPU Riccardo Mancini
2021-08-21 9:19 ` [RFC PATCH v1 09/37] perf python: add workqueue dependency Riccardo Mancini
2021-08-21 9:19 ` [RFC PATCH v1 10/37] perf evlist: add multithreading helper Riccardo Mancini
2021-08-21 9:19 ` [RFC PATCH v1 11/37] perf evlist: add multithreading to evlist__disable Riccardo Mancini
2021-08-21 9:19 ` [RFC PATCH v1 12/37] perf evlist: add multithreading to evlist__enable Riccardo Mancini
2021-08-21 9:19 ` [RFC PATCH v1 13/37] perf evlist: add multithreading to evlist__close Riccardo Mancini
2021-08-21 9:19 ` [RFC PATCH v1 14/37] perf evsel: remove retry_sample_id goto label Riccardo Mancini
2021-08-31 19:25 ` Arnaldo Carvalho de Melo
2021-08-21 9:19 ` [RFC PATCH v1 15/37] perf evsel: separate open preparation from open itself Riccardo Mancini
2021-08-31 19:27 ` Arnaldo Carvalho de Melo
2021-08-21 9:19 ` [RFC PATCH v1 16/37] perf evsel: save open flags in evsel Riccardo Mancini
2021-08-31 19:31 ` Arnaldo Carvalho de Melo
2021-08-21 9:19 ` [RFC PATCH v1 17/37] perf evsel: separate missing feature disabling from evsel__open_cpu Riccardo Mancini
2021-08-31 19:35 ` Arnaldo Carvalho de Melo
2021-08-21 9:19 ` [RFC PATCH v1 18/37] perf evsel: add evsel__prepare_open function Riccardo Mancini
2021-08-31 19:36 ` Arnaldo Carvalho de Melo
2021-08-21 9:19 ` [RFC PATCH v1 19/37] perf evsel: separate missing feature detection from evsel__open_cpu Riccardo Mancini
2021-08-31 19:39 ` Arnaldo Carvalho de Melo
2021-08-21 9:19 ` [RFC PATCH v1 20/37] perf evsel: separate rlimit increase " Riccardo Mancini
2021-08-31 19:41 ` Arnaldo Carvalho de Melo
2021-08-21 9:19 ` [RFC PATCH v1 21/37] perf evsel: move ignore_missing_thread to fallback code Riccardo Mancini
2021-08-31 19:44 ` Arnaldo Carvalho de Melo
2021-08-21 9:19 ` [RFC PATCH v1 22/37] perf evsel: move test_attr__open to success path in evsel__open_cpu Riccardo Mancini
2021-08-31 19:47 ` Arnaldo Carvalho de Melo
2021-08-21 9:19 ` [RFC PATCH v1 23/37] perf evsel: move bpf_counter__install_pe " Riccardo Mancini
2021-08-31 19:50 ` Arnaldo Carvalho de Melo
2021-08-21 9:19 ` [RFC PATCH v1 24/37] perf evsel: handle precise_ip fallback " Riccardo Mancini
2021-08-31 19:52 ` Arnaldo Carvalho de Melo
2021-08-21 9:19 ` [RFC PATCH v1 25/37] perf evsel: move event open in evsel__open_cpu to separate function Riccardo Mancini
2021-08-31 19:54 ` Arnaldo Carvalho de Melo
2021-09-03 21:52 ` Riccardo Mancini
2021-09-11 19:10 ` Arnaldo Carvalho de Melo
2021-08-21 9:19 ` [RFC PATCH v1 26/37] perf evsel: add evsel__open_per_cpu_no_fallback function Riccardo Mancini
2021-08-21 9:19 ` [RFC PATCH v1 27/37] perf evlist: add evlist__for_each_entry_from macro Riccardo Mancini
2021-08-31 20:06 ` Arnaldo Carvalho de Melo
2021-08-21 9:19 ` [RFC PATCH v1 28/37] perf evlist: add multithreading to evlist__open Riccardo Mancini
2021-08-21 9:19 ` [RFC PATCH v1 29/37] perf evlist: add custom fallback " Riccardo Mancini
2021-08-21 9:19 ` [RFC PATCH v1 30/37] perf record: use evlist__open_custom Riccardo Mancini
2021-08-21 9:19 ` [RFC PATCH v1 31/37] tools lib/subcmd: add OPT_UINTEGER_OPTARG option type Riccardo Mancini
2021-08-31 18:44 ` Arnaldo Carvalho de Melo
2021-08-21 9:19 ` [RFC PATCH v1 32/37] perf record: add --threads option Riccardo Mancini
2021-08-21 9:19 ` [RFC PATCH v1 33/37] perf record: pin threads to monitored cpus if enough threads available Riccardo Mancini
2021-08-21 9:19 ` [RFC PATCH v1 34/37] perf record: apply multithreading in init and fini phases Riccardo Mancini
2021-08-21 9:19 ` [RFC PATCH v1 35/37] perf test/evlist-open-close: add multithreading Riccardo Mancini
2021-08-21 9:19 ` [RFC PATCH v1 36/37] perf test/evlist-open-close: use inline func to convert timeval to usec Riccardo Mancini
2021-10-08 14:46 ` Arnaldo Carvalho de Melo
2021-08-21 9:19 ` [RFC PATCH v1 37/37] perf test/evlist-open-close: add detailed output mode Riccardo Mancini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YWBX4slh2U7yNahE@kernel.org \
--to=acme@kernel.org \
--cc=ak@linux.intel.com \
--cc=irogers@google.com \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=rickyman7@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.