[PATCH v2 34/48] perf evlist: Refactor evlist__for_each_cpu.
Namhyung Kim
namhyung at kernel.org
Tue Dec 28 16:58:57 PST 2021
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().
> +{
> + 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
>
More information about the linux-arm-kernel
mailing list