[PATCH v2 34/48] perf evlist: Refactor evlist__for_each_cpu.
Ian Rogers
irogers at google.com
Thu Dec 30 08:10:08 PST 2021
On Tue, Dec 28, 2021 at 4:59 PM Namhyung Kim <namhyung at kernel.org> wrote:
>
> On Wed, Dec 22, 2021 at 11:47 PM Ian Rogers <irogers at 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 at 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().
Apologies, I missed this with v3. The begin/end is intended to match
how STL will have a begin/end on the thing you foreach loop on.
> > +{
> > + 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.
Agreed this is complex, the pattern is trying to match current usage.
When the affinity is NULL in !per-CPU and per-thread mode, the CPU is
intentionally always -1 and the cpu maps of the events is expected to
just match the dummy (ie an array containing just -1). Next will
advance through the evsels keeping the CPU as -1 and the cpu_map_idx
as 0. When the end of the evlist is reached in next, the cpu_map_idx
will become 1, which is larger than the dummy map nr, and end will
become true.
So I think the code is correct here. It could more explicitly state
the dummy map assumptions when !affinity. Setting CPU doesn't really
make sense for the !per-CPU and per-thread mode case and the map
lookup of index 0 is correct here.
Thanks,
Ian
> 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
> >
More information about the linux-arm-kernel
mailing list