From: Andi Kleen <[email protected]>
Restructure event enabling/disabling to use affinity, which
minimizes the number of IPIs needed.
Before on a large test case with 94 CPUs:
% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
54.65 1.899986 22 84812 660 ioctl
after:
39.21 0.930451 10 84796 644 ioctl
Signed-off-by: Andi Kleen <[email protected]>
---
v2: Use new iterator macros
v3: Use new iterator macros
v4: Update iterators again
---
tools/perf/util/evlist.c | 40 +++++++++++++++++++++++++++++++++++++---
1 file changed, 37 insertions(+), 3 deletions(-)
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 33080f79b977..571bb102b432 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -378,11 +378,28 @@ bool evsel__cpu_iter_skip(struct evsel *ev, int cpu)
void evlist__disable(struct evlist *evlist)
{
struct evsel *pos;
+ 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__cpu_iter_skip(pos, cpu))
+ continue;
+ if (pos->disabled || !perf_evsel__is_group_leader(pos) || !pos->core.fd)
+ continue;
+ evsel__disable_cpu(pos, pos->cpu_iter - 1);
+ }
+ }
+ affinity__cleanup(&affinity);
evlist__for_each_entry(evlist, pos) {
- if (pos->disabled || !perf_evsel__is_group_leader(pos) || !pos->core.fd)
+ if (!perf_evsel__is_group_leader(pos) || !pos->core.fd)
continue;
- evsel__disable(pos);
+ pos->disabled = true;
}
evlist->enabled = false;
@@ -391,11 +408,28 @@ void evlist__disable(struct evlist *evlist)
void evlist__enable(struct evlist *evlist)
{
struct evsel *pos;
+ 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__cpu_iter_skip(pos, cpu))
+ continue;
+ if (!perf_evsel__is_group_leader(pos) || !pos->core.fd)
+ continue;
+ evsel__enable_cpu(pos, pos->cpu_iter - 1);
+ }
+ }
+ affinity__cleanup(&affinity);
evlist__for_each_entry(evlist, pos) {
if (!perf_evsel__is_group_leader(pos) || !pos->core.fd)
continue;
- evsel__enable(pos);
+ pos->disabled = false;
}
evlist->enabled = true;
--
2.23.0
On Thu, Nov 07, 2019 at 10:16:46AM -0800, Andi Kleen wrote:
SNIP
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 33080f79b977..571bb102b432 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -378,11 +378,28 @@ bool evsel__cpu_iter_skip(struct evsel *ev, int cpu)
> void evlist__disable(struct evlist *evlist)
> {
> struct evsel *pos;
> + struct affinity affinity;
> + int cpu, i;
should we have the fallback to current code in here (and below) as well?
also for reading/openning?
jirka
> +
> + 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__cpu_iter_skip(pos, cpu))
> + continue;
> + if (pos->disabled || !perf_evsel__is_group_leader(pos) || !pos->core.fd)
> + continue;
> + evsel__disable_cpu(pos, pos->cpu_iter - 1);
> + }
> + }
> + affinity__cleanup(&affinity);
> evlist__for_each_entry(evlist, pos) {
> - if (pos->disabled || !perf_evsel__is_group_leader(pos) || !pos->core.fd)
> + if (!perf_evsel__is_group_leader(pos) || !pos->core.fd)
> continue;
> - evsel__disable(pos);
> + pos->disabled = true;
> }
>
> evlist->enabled = false;
> @@ -391,11 +408,28 @@ void evlist__disable(struct evlist *evlist)
> void evlist__enable(struct evlist *evlist)
> {
> struct evsel *pos;
> + 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__cpu_iter_skip(pos, cpu))
> + continue;
> + if (!perf_evsel__is_group_leader(pos) || !pos->core.fd)
> + continue;
> + evsel__enable_cpu(pos, pos->cpu_iter - 1);
> + }
> + }
> + affinity__cleanup(&affinity);
> evlist__for_each_entry(evlist, pos) {
> if (!perf_evsel__is_group_leader(pos) || !pos->core.fd)
> continue;
> - evsel__enable(pos);
> + pos->disabled = false;
> }
>
> evlist->enabled = true;
> --
> 2.23.0
>
On Mon, Nov 11, 2019 at 03:04:15PM +0100, Jiri Olsa wrote:
> On Thu, Nov 07, 2019 at 10:16:46AM -0800, Andi Kleen wrote:
>
> SNIP
>
> > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > index 33080f79b977..571bb102b432 100644
> > --- a/tools/perf/util/evlist.c
> > +++ b/tools/perf/util/evlist.c
> > @@ -378,11 +378,28 @@ bool evsel__cpu_iter_skip(struct evsel *ev, int cpu)
> > void evlist__disable(struct evlist *evlist)
> > {
> > struct evsel *pos;
> > + struct affinity affinity;
> > + int cpu, i;
>
> should we have the fallback to current code in here (and below) as well?
> also for reading/openning?
The return only happens when you're out of memory, when nothing
will work anyways.
-Andi
>
> jirka
>
> > +
> > + if (affinity__setup(&affinity) < 0)
> > + return;
On Mon, Nov 11, 2019 at 08:50:28AM -0800, Andi Kleen wrote:
> On Mon, Nov 11, 2019 at 03:04:15PM +0100, Jiri Olsa wrote:
> > On Thu, Nov 07, 2019 at 10:16:46AM -0800, Andi Kleen wrote:
> >
> > SNIP
> >
> > > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > > index 33080f79b977..571bb102b432 100644
> > > --- a/tools/perf/util/evlist.c
> > > +++ b/tools/perf/util/evlist.c
> > > @@ -378,11 +378,28 @@ bool evsel__cpu_iter_skip(struct evsel *ev, int cpu)
> > > void evlist__disable(struct evlist *evlist)
> > > {
> > > struct evsel *pos;
> > > + struct affinity affinity;
> > > + int cpu, i;
> >
> > should we have the fallback to current code in here (and below) as well?
> > also for reading/openning?
>
> The return only happens when you're out of memory, when nothing
> will work anyways.
then let's have some assert or BUG_ON on !all_cpus
and remove the fallback code from close path
jirka
>
> -Andi
>
> >
> > jirka
> >
> > > +
> > > + if (affinity__setup(&affinity) < 0)
> > > + return;
>
> then let's have some assert or BUG_ON on !all_cpus
> and remove the fallback code from close path
I tried it again, but in record mode evsel->cpus is usually NULL,
resulting in various crashes.
I think fixing this beyond the scope of this patchkit, so i will
keep the fallback checks for now. I'll add better comments though.
-Andi