From: Jiri Olsa <jolsa@redhat.com>
To: Andi Kleen <andi@firstfloor.org>
Cc: acme@kernel.org, jolsa@kernel.org, eranian@google.com,
linux-kernel@vger.kernel.org, Andi Kleen <ak@linux.intel.com>
Subject: Re: [PATCH v3 5/7] perf stat: Use affinity for opening events
Date: Wed, 30 Oct 2019 11:06:00 +0100 [thread overview]
Message-ID: <20191030100600.GF20826@krava> (raw)
In-Reply-To: <20191025181417.10670-6-andi@firstfloor.org>
On Fri, Oct 25, 2019 at 11:14:15AM -0700, Andi Kleen wrote:
SNIP
>
> +enum counter_recovery {
> + COUNTER_SKIP,
> + COUNTER_RETRY,
> + COUNTER_FATAL,
> +};
> +
> +static enum counter_recovery stat_handle_error(struct evsel *counter)
> +{
> + char msg[BUFSIZ];
> + /*
> + * PPC returns ENXIO for HW counters until 2.6.37
> + * (behavior changed with commit b0a873e).
> + */
> + if (errno == EINVAL || errno == ENOSYS ||
> + errno == ENOENT || errno == EOPNOTSUPP ||
> + errno == ENXIO) {
> + if (verbose > 0)
> + ui__warning("%s event is not supported by the kernel.\n",
> + perf_evsel__name(counter));
> + counter->supported = false;
> + counter->errored = true;
> +
> + if ((counter->leader != counter) ||
> + !(counter->leader->core.nr_members > 1))
> + return COUNTER_SKIP;
> + } else if (perf_evsel__fallback(counter, errno, msg, sizeof(msg))) {
> + if (verbose > 0)
> + ui__warning("%s\n", msg);
> + return COUNTER_RETRY;
> + } else if (target__has_per_thread(&target) &&
> + evsel_list->core.threads &&
> + evsel_list->core.threads->err_thread != -1) {
> + /*
> + * For global --per-thread case, skip current
> + * error thread.
> + */
> + if (!thread_map__remove(evsel_list->core.threads,
> + evsel_list->core.threads->err_thread)) {
> + evsel_list->core.threads->err_thread = -1;
> + return COUNTER_RETRY;
> + }
> + }
> +
> + perf_evsel__open_strerror(counter, &target,
> + errno, msg, sizeof(msg));
> + ui__error("%s\n", msg);
> +
> + if (child_pid != -1)
> + kill(child_pid, SIGTERM);
> + return COUNTER_FATAL;
> +}
there's lot of code movement and other factoring together with
affinity changes, please separate those into separate patches
thanks,
jirka
> +
> static int __run_perf_stat(int argc, const char **argv, int run_idx)
> {
> int interval = stat_config.interval;
> @@ -428,11 +481,15 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> char msg[BUFSIZ];
> unsigned long long t0, t1;
> struct evsel *counter;
> + struct perf_cpu_map *cpus;
> struct timespec ts;
> size_t l;
> int status = 0;
> const bool forks = (argc > 0);
> bool is_pipe = STAT_RECORD ? perf_stat.data.is_pipe : false;
> + struct affinity affinity;
> + int i, cpu;
> + bool second_pass = false;
>
> if (interval) {
> ts.tv_sec = interval / USEC_PER_MSEC;
> @@ -457,61 +514,109 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> if (group)
> perf_evlist__set_leader(evsel_list);
>
> - evlist__for_each_entry(evsel_list, counter) {
> + if (affinity__setup(&affinity) < 0)
> + return -1;
> +
> + cpus = evlist__cpu_iter_start(evsel_list);
> + cpumap__for_each_cpu (cpus, i, cpu) {
> + affinity__set(&affinity, cpu);
> +
> + evlist__for_each_entry(evsel_list, counter) {
> + if (evlist__cpu_iter_skip(counter, cpu))
> + continue;
> + if (counter->reset_group || counter->errored)
> + continue;
> + evlist__cpu_iter_next(counter);
> try_again:
> - if (create_perf_stat_counter(counter, &stat_config, &target) < 0) {
> -
> - /* Weak group failed. Reset the group. */
> - if ((errno == EINVAL || errno == EBADF) &&
> - counter->leader != counter &&
> - counter->weak_group) {
> - counter = perf_evlist__reset_weak_group(evsel_list, counter);
> - goto try_again;
> - }
> + if (create_perf_stat_counter(counter, &stat_config, &target,
> + counter->cpu_index - 1) < 0) {
>
> - /*
> - * PPC returns ENXIO for HW counters until 2.6.37
> - * (behavior changed with commit b0a873e).
> - */
> - if (errno == EINVAL || errno == ENOSYS ||
> - errno == ENOENT || errno == EOPNOTSUPP ||
> - errno == ENXIO) {
> - if (verbose > 0)
> - ui__warning("%s event is not supported by the kernel.\n",
> - perf_evsel__name(counter));
> - counter->supported = false;
> -
> - if ((counter->leader != counter) ||
> - !(counter->leader->core.nr_members > 1))
> - continue;
> - } else if (perf_evsel__fallback(counter, errno, msg, sizeof(msg))) {
> - if (verbose > 0)
> - ui__warning("%s\n", msg);
> - goto try_again;
> - } else if (target__has_per_thread(&target) &&
> - evsel_list->core.threads &&
> - evsel_list->core.threads->err_thread != -1) {
> /*
> - * For global --per-thread case, skip current
> - * error thread.
> + * Weak group failed. We cannot just undo this here
> + * because earlier CPUs might be in group mode, and the kernel
> + * doesn't support mixing group and non group reads. Defer
> + * it to later.
> + * Don't close here because we're in the wrong affinity.
> */
> - if (!thread_map__remove(evsel_list->core.threads,
> - evsel_list->core.threads->err_thread)) {
> - evsel_list->core.threads->err_thread = -1;
> + if ((errno == EINVAL || errno == EBADF) &&
> + counter->leader != counter &&
> + counter->weak_group) {
> + perf_evlist__reset_weak_group(evsel_list, counter, false);
> + assert(counter->reset_group);
> + second_pass = true;
> + continue;
> + }
> +
> + switch (stat_handle_error(counter)) {
> + case COUNTER_FATAL:
> + return -1;
> + case COUNTER_RETRY:
> goto try_again;
> + case COUNTER_SKIP:
> + continue;
> + default:
> + break;
> }
> +
> }
> + counter->supported = true;
> + }
> + }
>
> - perf_evsel__open_strerror(counter, &target,
> - errno, msg, sizeof(msg));
> - ui__error("%s\n", msg);
> + if (second_pass) {
> + /*
> + * Now redo all the weak group after closing them,
> + * and also close errored counters.
> + */
>
> - if (child_pid != -1)
> - kill(child_pid, SIGTERM);
> + cpus = evlist__cpu_iter_start(evsel_list);
> + cpumap__for_each_cpu (cpus, i, cpu) {
> + affinity__set(&affinity, cpu);
> + /* First close errored or weak retry */
> + evlist__for_each_entry(evsel_list, counter) {
> + if (!counter->reset_group && !counter->errored)
> + continue;
> + if (evlist__cpu_iter_skip(counter, cpu))
> + continue;
> + perf_evsel__close_cpu(&counter->core, counter->cpu_index);
> + }
> + /* Now reopen weak */
> + evlist__for_each_entry(evsel_list, counter) {
> + if (!counter->reset_group)
> + continue;
> + if (evlist__cpu_iter_skip(counter, cpu))
> + continue;
> + evlist__cpu_iter_next(counter);
> +try_again_reset:
> + pr_debug2("reopening weak %s\n", perf_evsel__name(counter));
> + if (create_perf_stat_counter(counter, &stat_config, &target,
> + counter->cpu_index - 1) < 0) {
> +
> + switch (stat_handle_error(counter)) {
> + case COUNTER_FATAL:
> + return -1;
> + case COUNTER_RETRY:
> + goto try_again_reset;
> + case COUNTER_SKIP:
> + continue;
> + default:
> + break;
> + }
> + }
> + counter->supported = true;
> + }
> + }
> + }
> + affinity__cleanup(&affinity);
>
> - return -1;
> + evlist__for_each_entry(evsel_list, counter) {
> + if (!counter->supported) {
> + perf_evsel__free_fd(&counter->core);
> + continue;
> }
> - counter->supported = true;
> + /* Must have consumed all map indexes */
> + assert(!counter->errored &&
> + counter->cpu_index == counter->core.cpus->nr);
>
> l = strlen(counter->unit);
> if (l > stat_config.unit_width)
> diff --git a/tools/perf/tests/event-times.c b/tools/perf/tests/event-times.c
> index 1ee8704e2284..1e8a9f5c356d 100644
> --- a/tools/perf/tests/event-times.c
> +++ b/tools/perf/tests/event-times.c
> @@ -125,7 +125,7 @@ static int attach__cpu_disabled(struct evlist *evlist)
>
> evsel->core.attr.disabled = 1;
>
> - err = perf_evsel__open_per_cpu(evsel, cpus);
> + err = perf_evsel__open_per_cpu(evsel, cpus, -1);
> if (err) {
> if (err == -EACCES)
> return TEST_SKIP;
> @@ -152,7 +152,7 @@ static int attach__cpu_enabled(struct evlist *evlist)
> return -1;
> }
>
> - err = perf_evsel__open_per_cpu(evsel, cpus);
> + err = perf_evsel__open_per_cpu(evsel, cpus, -1);
> if (err == -EACCES)
> return TEST_SKIP;
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index aeb82de36043..ca9b06979fc0 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1635,7 +1635,8 @@ void perf_evlist__force_leader(struct evlist *evlist)
> }
>
> struct evsel *perf_evlist__reset_weak_group(struct evlist *evsel_list,
> - struct evsel *evsel)
> + struct evsel *evsel,
> + bool close)
> {
> struct evsel *c2, *leader;
> bool is_open = true;
> @@ -1652,10 +1653,11 @@ struct evsel *perf_evlist__reset_weak_group(struct evlist *evsel_list,
> if (c2 == evsel)
> is_open = false;
> if (c2->leader == leader) {
> - if (is_open)
> + if (is_open && close)
> perf_evsel__close(&c2->core);
> c2->leader = c2;
> c2->core.nr_members = 0;
> + c2->reset_group = true;
> }
> }
> return leader;
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index c1deb8ebdcea..d9174d565db3 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -351,5 +351,6 @@ bool perf_evlist__exclude_kernel(struct evlist *evlist);
> void perf_evlist__force_leader(struct evlist *evlist);
>
> struct evsel *perf_evlist__reset_weak_group(struct evlist *evlist,
> - struct evsel *evsel);
> + struct evsel *evsel,
> + bool close);
> #endif /* __PERF_EVLIST_H */
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index d4451846af93..7106f9a067df 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1569,8 +1569,9 @@ static int perf_event_open(struct evsel *evsel,
> return fd;
> }
>
> -int evsel__open(struct evsel *evsel, struct perf_cpu_map *cpus,
> - struct perf_thread_map *threads)
> +static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
> + struct perf_thread_map *threads,
> + int start_cpu, int end_cpu)
> {
> int cpu, thread, nthreads;
> unsigned long flags = PERF_FLAG_FD_CLOEXEC;
> @@ -1647,7 +1648,7 @@ int evsel__open(struct evsel *evsel, struct perf_cpu_map *cpus,
>
> display_attr(&evsel->core.attr);
>
> - for (cpu = 0; cpu < cpus->nr; cpu++) {
> + for (cpu = start_cpu; cpu < end_cpu; cpu++) {
>
> for (thread = 0; thread < nthreads; thread++) {
> int fd, group_fd;
> @@ -1825,6 +1826,12 @@ int evsel__open(struct evsel *evsel, struct perf_cpu_map *cpus,
> return err;
> }
>
> +int evsel__open(struct evsel *evsel, struct perf_cpu_map *cpus,
> + struct perf_thread_map *threads)
> +{
> + return evsel__open_cpu(evsel, cpus, threads, 0, cpus ? cpus->nr : 1);
> +}
> +
> void evsel__close(struct evsel *evsel)
> {
> perf_evsel__close(&evsel->core);
> @@ -1832,9 +1839,10 @@ void evsel__close(struct evsel *evsel)
> }
>
> int perf_evsel__open_per_cpu(struct evsel *evsel,
> - struct perf_cpu_map *cpus)
> + struct perf_cpu_map *cpus,
> + int cpu)
> {
> - return evsel__open(evsel, cpus, NULL);
> + return evsel__open_cpu(evsel, cpus, NULL, cpu, cpu + 1);
> }
>
> int perf_evsel__open_per_thread(struct evsel *evsel,
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 2e3b011ed09e..d5440a928745 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -94,6 +94,8 @@ struct evsel {
> struct evsel *metric_leader;
> bool collect_stat;
> bool weak_group;
> + bool reset_group;
> + bool errored;
> bool percore;
> int cpu_index;
> const char *pmu_name;
> @@ -223,7 +225,8 @@ int evsel__enable(struct evsel *evsel);
> int evsel__disable(struct evsel *evsel);
>
> int perf_evsel__open_per_cpu(struct evsel *evsel,
> - struct perf_cpu_map *cpus);
> + struct perf_cpu_map *cpus,
> + int cpu);
> int perf_evsel__open_per_thread(struct evsel *evsel,
> struct perf_thread_map *threads);
> int evsel__open(struct evsel *evsel, struct perf_cpu_map *cpus,
> diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c
> index 6822e4ffe224..3aebe732e886 100644
> --- a/tools/perf/util/stat.c
> +++ b/tools/perf/util/stat.c
> @@ -463,7 +463,8 @@ size_t perf_event__fprintf_stat_config(union perf_event *event, FILE *fp)
>
> int create_perf_stat_counter(struct evsel *evsel,
> struct perf_stat_config *config,
> - struct target *target)
> + struct target *target,
> + int cpu)
> {
> struct perf_event_attr *attr = &evsel->core.attr;
> struct evsel *leader = evsel->leader;
> @@ -517,7 +518,7 @@ int create_perf_stat_counter(struct evsel *evsel,
> }
>
> if (target__has_cpu(target) && !target__has_per_thread(target))
> - return perf_evsel__open_per_cpu(evsel, evsel__cpus(evsel));
> + return perf_evsel__open_per_cpu(evsel, evsel__cpus(evsel), cpu);
>
> return perf_evsel__open_per_thread(evsel, evsel->core.threads);
> }
> diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h
> index 081c4a5113c6..4c9a7b68c3e7 100644
> --- a/tools/perf/util/stat.h
> +++ b/tools/perf/util/stat.h
> @@ -213,7 +213,8 @@ size_t perf_event__fprintf_stat_config(union perf_event *event, FILE *fp);
>
> int create_perf_stat_counter(struct evsel *evsel,
> struct perf_stat_config *config,
> - struct target *target);
> + struct target *target,
> + int cpu);
> void
> perf_evlist__print_counters(struct evlist *evlist,
> struct perf_stat_config *config,
> --
> 2.21.0
>
next prev parent reply other threads:[~2019-10-30 10:06 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-25 18:14 Optimize perf stat for large number of events/cpus v3 Andi Kleen
2019-10-25 18:14 ` [PATCH v3 1/7] perf pmu: Use file system cache to optimize sysfs access Andi Kleen
2019-10-28 22:01 ` Jiri Olsa
2019-10-29 2:14 ` Andi Kleen
2019-10-25 18:14 ` [PATCH v3 2/7] perf affinity: Add infrastructure to save/restore affinity Andi Kleen
2019-10-25 18:14 ` [PATCH v3 3/7] perf evsel: Add iterator to iterate over events ordered by CPU Andi Kleen
2019-10-30 10:05 ` Jiri Olsa
2019-10-30 10:06 ` Jiri Olsa
2019-10-30 15:51 ` Andi Kleen
2019-10-30 18:15 ` Jiri Olsa
2019-10-30 19:03 ` Andi Kleen
2019-11-01 8:38 ` Jiri Olsa
2019-10-25 18:14 ` [PATCH v3 4/7] perf stat: Use affinity for closing file descriptors Andi Kleen
2019-10-30 10:05 ` Jiri Olsa
2019-11-04 23:35 ` Andi Kleen
2019-10-25 18:14 ` [PATCH v3 5/7] perf stat: Use affinity for opening events Andi Kleen
2019-10-30 10:06 ` Jiri Olsa [this message]
2019-10-25 18:14 ` [PATCH v3 6/7] perf stat: Use affinity for reading Andi Kleen
2019-10-25 18:14 ` [PATCH v3 7/7] perf stat: Use affinity for enabling/disabling events Andi Kleen
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=20191030100600.GF20826@krava \
--to=jolsa@redhat.com \
--cc=acme@kernel.org \
--cc=ak@linux.intel.com \
--cc=andi@firstfloor.org \
--cc=eranian@google.com \
--cc=jolsa@kernel.org \
--cc=linux-kernel@vger.kernel.org \
/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.