Messages in this thread | | | From | Namhyung Kim <> | Date | Tue, 28 Dec 2021 16:58:57 -0800 | Subject | Re: [PATCH v2 34/48] perf evlist: Refactor evlist__for_each_cpu. |
| |
On Wed, Dec 22, 2021 at 11:47 PM Ian Rogers <irogers@google.com> wrote: > > Previously evlist__for_each_cpu needed to iterate over the evlist in an > inner loop and call "skip" routines. Refactor this so that the iteratr > is smarter and the next function can update both the current CPU and > evsel. > > By using a cpu map index, fix apparent off-by-1 in __run_perf_stat's > call to perf_evsel__close_cpu. > > Signed-off-by: Ian Rogers <irogers@google.com> > --- [SNIP] > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > index 5f92319ce258..39d294f6c321 100644 > --- a/tools/perf/util/evlist.c > +++ b/tools/perf/util/evlist.c > @@ -342,36 +342,65 @@ 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) > -{ > - 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; > -} > +struct evlist_cpu_iterator evlist__cpu_begin(struct evlist *evlist, struct affinity *affinity)
I think we can rename it to evlist_cpu_iterator__begin().
> +{ > + struct evlist_cpu_iterator itr = { > + .container = evlist, > + .evsel = evlist__first(evlist), > + .cpu_map_idx = 0, > + .evlist_cpu_map_idx = 0, > + .evlist_cpu_map_nr = perf_cpu_map__nr(evlist->core.all_cpus), > + .cpu = -1, > + .affinity = affinity, > + }; > > -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; > + if (itr.affinity) { > + itr.cpu = perf_cpu_map__cpu(evlist->core.all_cpus, 0); > + affinity__set(itr.affinity, itr.cpu); > + itr.cpu_map_idx = perf_cpu_map__idx(itr.evsel->core.cpus, itr.cpu);
This doesn't match to the evlist_cpu_iterator__next() when affinity is NULL. It should set the cpu and cpu_map_idx.
Other than that, I like this! :)
Thanks, Namhyung
> + /* > + * If this CPU isn't in the evsel's cpu map then advance through > + * the list. > + */ > + if (itr.cpu_map_idx == -1) > + evlist_cpu_iterator__next(&itr); > + } > + return itr; > +} > + > +void evlist_cpu_iterator__next(struct evlist_cpu_iterator *evlist_cpu_itr) > +{ > + while (evlist_cpu_itr->evsel != evlist__last(evlist_cpu_itr->container)) { > + evlist_cpu_itr->evsel = evsel__next(evlist_cpu_itr->evsel); > + evlist_cpu_itr->cpu_map_idx = > + perf_cpu_map__idx(evlist_cpu_itr->evsel->core.cpus, > + evlist_cpu_itr->cpu); > + if (evlist_cpu_itr->cpu_map_idx != -1) > + return; > + } > + evlist_cpu_itr->evlist_cpu_map_idx++; > + if (evlist_cpu_itr->evlist_cpu_map_idx < evlist_cpu_itr->evlist_cpu_map_nr) { > + evlist_cpu_itr->evsel = evlist__first(evlist_cpu_itr->container); > + evlist_cpu_itr->cpu = > + perf_cpu_map__cpu(evlist_cpu_itr->container->core.all_cpus, > + evlist_cpu_itr->evlist_cpu_map_idx); > + if (evlist_cpu_itr->affinity) > + affinity__set(evlist_cpu_itr->affinity, evlist_cpu_itr->cpu); > + evlist_cpu_itr->cpu_map_idx = > + perf_cpu_map__idx(evlist_cpu_itr->evsel->core.cpus, > + evlist_cpu_itr->cpu); > + /* > + * If this CPU isn't in the evsel's cpu map then advance through > + * the list. > + */ > + if (evlist_cpu_itr->cpu_map_idx == -1) > + evlist_cpu_iterator__next(evlist_cpu_itr); > + } > } > > -bool evsel__cpu_iter_skip(struct evsel *ev, int cpu) > +bool evlist_cpu_iterator__end(const struct evlist_cpu_iterator *evlist_cpu_itr) > { > - if (!evsel__cpu_iter_skip_no_inc(ev, cpu)) { > - ev->cpu_iter++; > - return false; > - } > - return true; > + return evlist_cpu_itr->evlist_cpu_map_idx >= evlist_cpu_itr->evlist_cpu_map_nr; > } > > static int evsel__strcmp(struct evsel *pos, char *evsel_name) > @@ -400,31 +429,26 @@ static int evlist__is_enabled(struct evlist *evlist) > static void __evlist__disable(struct evlist *evlist, char *evsel_name) > { > struct evsel *pos; > + struct evlist_cpu_iterator evlist_cpu_itr; > struct affinity affinity; > - int cpu, i, imm = 0; > bool has_imm = false; > > if (affinity__setup(&affinity) < 0) > return; > > /* Disable 'immediate' events last */ > - for (imm = 0; imm <= 1; imm++) { > - evlist__for_each_cpu(evlist, i, cpu) { > - affinity__set(&affinity, cpu); > - > - evlist__for_each_entry(evlist, pos) { > - if (evsel__strcmp(pos, evsel_name)) > - continue; > - if (evsel__cpu_iter_skip(pos, cpu)) > - continue; > - if (pos->disabled || !evsel__is_group_leader(pos) || !pos->core.fd) > - continue; > - if (pos->immediate) > - has_imm = true; > - if (pos->immediate != imm) > - continue; > - evsel__disable_cpu(pos, pos->cpu_iter - 1); > - } > + for (int imm = 0; imm <= 1; imm++) { > + evlist__for_each_cpu(evlist_cpu_itr, evlist, &affinity) { > + pos = evlist_cpu_itr.evsel; > + if (evsel__strcmp(pos, evsel_name)) > + continue; > + if (pos->disabled || !evsel__is_group_leader(pos) || !pos->core.fd) > + continue; > + if (pos->immediate) > + has_imm = true; > + if (pos->immediate != imm) > + continue; > + evsel__disable_cpu(pos, evlist_cpu_itr.cpu_map_idx); > } > if (!has_imm) > break; > @@ -462,24 +486,19 @@ void evlist__disable_evsel(struct evlist *evlist, char *evsel_name) > static void __evlist__enable(struct evlist *evlist, char *evsel_name) > { > struct evsel *pos; > + struct evlist_cpu_iterator evlist_cpu_itr; > struct affinity affinity; > - int cpu, i; > > if (affinity__setup(&affinity) < 0) > return; > > - evlist__for_each_cpu(evlist, i, cpu) { > - affinity__set(&affinity, cpu); > - > - evlist__for_each_entry(evlist, pos) { > - if (evsel__strcmp(pos, evsel_name)) > - continue; > - if (evsel__cpu_iter_skip(pos, cpu)) > - continue; > - if (!evsel__is_group_leader(pos) || !pos->core.fd) > - continue; > - evsel__enable_cpu(pos, pos->cpu_iter - 1); > - } > + evlist__for_each_cpu(evlist_cpu_itr, evlist, &affinity) { > + pos = evlist_cpu_itr.evsel; > + if (evsel__strcmp(pos, evsel_name)) > + continue; > + if (!evsel__is_group_leader(pos) || !pos->core.fd) > + continue; > + evsel__enable_cpu(pos, evlist_cpu_itr.cpu_map_idx); > } > affinity__cleanup(&affinity); > evlist__for_each_entry(evlist, pos) { > @@ -1264,8 +1283,8 @@ void evlist__set_selected(struct evlist *evlist, struct evsel *evsel) > void evlist__close(struct evlist *evlist) > { > struct evsel *evsel; > + struct evlist_cpu_iterator evlist_cpu_itr; > struct affinity affinity; > - int cpu, i; > > /* > * With perf record core.cpus is usually NULL. > @@ -1279,15 +1298,12 @@ void evlist__close(struct evlist *evlist) > > if (affinity__setup(&affinity) < 0) > return; > - evlist__for_each_cpu(evlist, i, cpu) { > - 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); > - } > + evlist__for_each_cpu(evlist_cpu_itr, evlist, &affinity) { > + perf_evsel__close_cpu(&evlist_cpu_itr.evsel->core, > + evlist_cpu_itr.cpu_map_idx); > } > + > affinity__cleanup(&affinity); > evlist__for_each_entry_reverse(evlist, evsel) { > perf_evsel__free_fd(&evsel->core); > diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h > index 27594900a052..57828ebfcb61 100644 > --- a/tools/perf/util/evlist.h > +++ b/tools/perf/util/evlist.h > @@ -327,17 +327,53 @@ void evlist__to_front(struct evlist *evlist, struct evsel *move_evsel); > #define evlist__for_each_entry_safe(evlist, tmp, 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) > +/** Iterator state for evlist__for_each_cpu */ > +struct evlist_cpu_iterator { > + /** The list being iterated through. */ > + struct evlist *container; > + /** The current evsel of the iterator. */ > + struct evsel *evsel; > + /** The CPU map index corresponding to the evsel->core.cpus for the current CPU. */ > + int cpu_map_idx; > + /** > + * The CPU map index corresponding to evlist->core.all_cpus for the > + * current CPU. Distinct from cpu_map_idx as the evsel's cpu map may > + * contain fewer entries. > + */ > + int evlist_cpu_map_idx; > + /** The number of CPU map entries in evlist->core.all_cpus. */ > + int evlist_cpu_map_nr; > + /** The current CPU of the iterator. */ > + int cpu; > + /** If present, used to set the affinity when switching between CPUs. */ > + struct affinity *affinity; > +}; > + > +/** > + * evlist__for_each_cpu - without affinity, iterate over the evlist. With > + * affinity, iterate over all CPUs and then the evlist > + * for each evsel on that CPU. When switching between > + * CPUs the affinity is set to the CPU to avoid IPIs > + * during syscalls. > + * @evlist_cpu_itr: the iterator instance. > + * @evlist: evlist instance to iterate. > + * @affinity: NULL or used to set the affinity to the current CPU. > + */ > +#define evlist__for_each_cpu(evlist_cpu_itr, evlist, affinity) \ > + for ((evlist_cpu_itr) = evlist__cpu_begin(evlist, affinity); \ > + !evlist_cpu_iterator__end(&evlist_cpu_itr); \ > + evlist_cpu_iterator__next(&evlist_cpu_itr)) > + > +/** Returns an iterator set to the first CPU/evsel of evlist. */ > +struct evlist_cpu_iterator evlist__cpu_begin(struct evlist *evlist, struct affinity *affinity); > +/** Move to next element in iterator, updating CPU, evsel and the affinity. */ > +void evlist_cpu_iterator__next(struct evlist_cpu_iterator *evlist_cpu_itr); > +/** Returns true when iterator is at the end of the CPUs and evlist. */ > +bool evlist_cpu_iterator__end(const struct evlist_cpu_iterator *evlist_cpu_itr); > > 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); > - > struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str); > > struct evsel *evlist__event2evsel(struct evlist *evlist, union perf_event *event); > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h > index 99aa3363def7..7cb7c9c77ab0 100644 > --- a/tools/perf/util/evsel.h > +++ b/tools/perf/util/evsel.h > @@ -121,7 +121,6 @@ struct evsel { > bool errored; > struct hashmap *per_pkg_mask; > int err; > - int cpu_iter; > struct { > evsel__sb_cb_t *cb; > void *data; > -- > 2.34.1.307.g9b7440fafd-goog >
| |