Re: [PATCH v2 4/9] perf affinity: Add infrastructure to save/restore affinity
On 23.10.2019 20:19, Andi Kleen wrote:
> On Wed, Oct 23, 2019 at 07:16:13PM +0300, Alexey Budankov wrote:
>>
>> On 23.10.2019 17:52, Andi Kleen wrote:
>>> On Wed, Oct 23, 2019 at 04:30:49PM +0200, Jiri Olsa wrote:
On Wed, Oct 23, 2019 at 06:02:35AM -0700, Andi Kleen wrote:
> On Wed, Oct 23, 2019 at 11:59:11AM +0200, Jiri Olsa wrote:
>> On Sun, Oct 20, 2019 at 10:51:57AM -0700, Andi Kleen wrote:
>>
>> SNIP
>>
>>> +}
>>> diff --git a/tools/perf/util/affinity.h b/tools/perf/util/affinity.h
>>> new file mode 100644
>>> index ..e56148607e33
>>> --- /dev/null
>>> +++ b/tools/perf/util/affinity.h
>>> @@ -0,0 +1,15 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +#ifndef AFFINITY_H
>>> +#define AFFINITY_H 1
>>> +
>>> +struct affinity {
>>> + unsigned char *orig_cpus;
>>> + unsigned char *sched_cpus;
>>
>> why not use cpu_set_t directly?
>
> Because it's too small in glibc (only 1024 CPUs) and perf already
> supports more.
nice, we're using it all over the place.. how about using bitmap_alloc?
>>>
>>> Okay.
>>>
>>> The other places is mainly perf record from Alexey's recent affinity
>>> changes.
>>> These probably need to be fixed.
>>>
>>> +Alexey
>>
>> Despite the issue indeed looks generic for stat and record modes,
>> have you already observed record startup overhead somewhere in your setups?
>> I would, first, prefer to reproduce the overhead, to have stable use case
>> for evaluation and then, possibly, improvement.
>
> What I meant the cpu_set usages you added in
>
> commit 9d2ed64587c045304efe8872b0258c30803d370c
> Author: Alexey Budankov
> Date: Tue Jan 22 20:47:43 2019 +0300
>
> perf record: Allocate affinity masks
>
> need to be fixed to allocate dynamically, or at least use MAX_NR_CPUs to
> support systems with >1024CPUs. That's an independent functionality
> problem.
Oh, it is clear now. Thanks for pointing this out. For that to move from
cpu_mask_t to new custom struct affinity type its API requires extension
to provide mask operations similar to the ones that cpu_mask_t provides:
CPU_ZERO(), CPU_SET(), CPU_EQUAL(), CPU_OR().
For example it could be like: affinity__mask_zero(), affinity__mask_set(),
affinity__mask_equal(), affinity__mask_or() and then the collecting part
of record could also be moved to struct affinity type and overcome >1024CPUs
limitation.
~Alexey
>
> I haven't seen any large enough perf record usage to run
> into the IPI problems for record.
>
> -Andi
>
Re: [PATCH v2 4/9] perf affinity: Add infrastructure to save/restore affinity
On Wed, Oct 23, 2019 at 07:16:13PM +0300, Alexey Budankov wrote:
>
> On 23.10.2019 17:52, Andi Kleen wrote:
> > On Wed, Oct 23, 2019 at 04:30:49PM +0200, Jiri Olsa wrote:
> >> On Wed, Oct 23, 2019 at 06:02:35AM -0700, Andi Kleen wrote:
> >>> On Wed, Oct 23, 2019 at 11:59:11AM +0200, Jiri Olsa wrote:
> On Sun, Oct 20, 2019 at 10:51:57AM -0700, Andi Kleen wrote:
>
> SNIP
>
> > +}
> > diff --git a/tools/perf/util/affinity.h b/tools/perf/util/affinity.h
> > new file mode 100644
> > index ..e56148607e33
> > --- /dev/null
> > +++ b/tools/perf/util/affinity.h
> > @@ -0,0 +1,15 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#ifndef AFFINITY_H
> > +#define AFFINITY_H 1
> > +
> > +struct affinity {
> > + unsigned char *orig_cpus;
> > + unsigned char *sched_cpus;
>
> why not use cpu_set_t directly?
> >>>
> >>> Because it's too small in glibc (only 1024 CPUs) and perf already
> >>> supports more.
> >>
> >> nice, we're using it all over the place.. how about using bitmap_alloc?
> >
> > Okay.
> >
> > The other places is mainly perf record from Alexey's recent affinity
> > changes.
> > These probably need to be fixed.
> >
> > +Alexey
>
> Despite the issue indeed looks generic for stat and record modes,
> have you already observed record startup overhead somewhere in your setups?
> I would, first, prefer to reproduce the overhead, to have stable use case
> for evaluation and then, possibly, improvement.
What I meant the cpu_set usages you added in
commit 9d2ed64587c045304efe8872b0258c30803d370c
Author: Alexey Budankov
Date: Tue Jan 22 20:47:43 2019 +0300
perf record: Allocate affinity masks
need to be fixed to allocate dynamically, or at least use MAX_NR_CPUs to
support systems with >1024CPUs. That's an independent functionality
problem.
I haven't seen any large enough perf record usage to run
into the IPI problems for record.
-Andi
Re: [PATCH v2 4/9] perf affinity: Add infrastructure to save/restore affinity
On 23.10.2019 17:52, Andi Kleen wrote:
> On Wed, Oct 23, 2019 at 04:30:49PM +0200, Jiri Olsa wrote:
>> On Wed, Oct 23, 2019 at 06:02:35AM -0700, Andi Kleen wrote:
>>> On Wed, Oct 23, 2019 at 11:59:11AM +0200, Jiri Olsa wrote:
On Sun, Oct 20, 2019 at 10:51:57AM -0700, Andi Kleen wrote:
SNIP
> +}
> diff --git a/tools/perf/util/affinity.h b/tools/perf/util/affinity.h
> new file mode 100644
> index ..e56148607e33
> --- /dev/null
> +++ b/tools/perf/util/affinity.h
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#ifndef AFFINITY_H
> +#define AFFINITY_H 1
> +
> +struct affinity {
> + unsigned char *orig_cpus;
> + unsigned char *sched_cpus;
why not use cpu_set_t directly?
>>>
>>> Because it's too small in glibc (only 1024 CPUs) and perf already
>>> supports more.
>>
>> nice, we're using it all over the place.. how about using bitmap_alloc?
>
> Okay.
>
> The other places is mainly perf record from Alexey's recent affinity changes.
> These probably need to be fixed.
>
> +Alexey
Despite the issue indeed looks generic for stat and record modes,
have you already observed record startup overhead somewhere in your setups?
I would, first, prefer to reproduce the overhead, to have stable use case
for evaluation and then, possibly, improvement.
~Alexey
Re: [PATCH v2 4/9] perf affinity: Add infrastructure to save/restore affinity
On Wed, Oct 23, 2019 at 04:30:49PM +0200, Jiri Olsa wrote:
> On Wed, Oct 23, 2019 at 06:02:35AM -0700, Andi Kleen wrote:
> > On Wed, Oct 23, 2019 at 11:59:11AM +0200, Jiri Olsa wrote:
> > > On Sun, Oct 20, 2019 at 10:51:57AM -0700, Andi Kleen wrote:
> > >
> > > SNIP
> > >
> > > > +}
> > > > diff --git a/tools/perf/util/affinity.h b/tools/perf/util/affinity.h
> > > > new file mode 100644
> > > > index ..e56148607e33
> > > > --- /dev/null
> > > > +++ b/tools/perf/util/affinity.h
> > > > @@ -0,0 +1,15 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +#ifndef AFFINITY_H
> > > > +#define AFFINITY_H 1
> > > > +
> > > > +struct affinity {
> > > > + unsigned char *orig_cpus;
> > > > + unsigned char *sched_cpus;
> > >
> > > why not use cpu_set_t directly?
> >
> > Because it's too small in glibc (only 1024 CPUs) and perf already
> > supports more.
>
> nice, we're using it all over the place.. how about using bitmap_alloc?
Okay.
The other places is mainly perf record from Alexey's recent affinity changes.
These probably need to be fixed.
+Alexey
And some stuff in bench/*. That's more nice to have.
-Andi
Re: [PATCH v2 4/9] perf affinity: Add infrastructure to save/restore affinity
On Wed, Oct 23, 2019 at 06:02:35AM -0700, Andi Kleen wrote:
> On Wed, Oct 23, 2019 at 11:59:11AM +0200, Jiri Olsa wrote:
> > On Sun, Oct 20, 2019 at 10:51:57AM -0700, Andi Kleen wrote:
> >
> > SNIP
> >
> > > +}
> > > diff --git a/tools/perf/util/affinity.h b/tools/perf/util/affinity.h
> > > new file mode 100644
> > > index ..e56148607e33
> > > --- /dev/null
> > > +++ b/tools/perf/util/affinity.h
> > > @@ -0,0 +1,15 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +#ifndef AFFINITY_H
> > > +#define AFFINITY_H 1
> > > +
> > > +struct affinity {
> > > + unsigned char *orig_cpus;
> > > + unsigned char *sched_cpus;
> >
> > why not use cpu_set_t directly?
>
> Because it's too small in glibc (only 1024 CPUs) and perf already
> supports more.
nice, we're using it all over the place.. how about using bitmap_alloc?
jirka
Re: [PATCH v2 4/9] perf affinity: Add infrastructure to save/restore affinity
On Wed, Oct 23, 2019 at 11:59:11AM +0200, Jiri Olsa wrote:
> On Sun, Oct 20, 2019 at 10:51:57AM -0700, Andi Kleen wrote:
>
> SNIP
>
> > +}
> > diff --git a/tools/perf/util/affinity.h b/tools/perf/util/affinity.h
> > new file mode 100644
> > index ..e56148607e33
> > --- /dev/null
> > +++ b/tools/perf/util/affinity.h
> > @@ -0,0 +1,15 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#ifndef AFFINITY_H
> > +#define AFFINITY_H 1
> > +
> > +struct affinity {
> > + unsigned char *orig_cpus;
> > + unsigned char *sched_cpus;
>
> why not use cpu_set_t directly?
Because it's too small in glibc (only 1024 CPUs) and perf already
supports more.
-andi
Re: [PATCH v2 4/9] perf affinity: Add infrastructure to save/restore affinity
On Sun, Oct 20, 2019 at 10:51:57AM -0700, Andi Kleen wrote:
SNIP
> +}
> diff --git a/tools/perf/util/affinity.h b/tools/perf/util/affinity.h
> new file mode 100644
> index ..e56148607e33
> --- /dev/null
> +++ b/tools/perf/util/affinity.h
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#ifndef AFFINITY_H
> +#define AFFINITY_H 1
> +
> +struct affinity {
> + unsigned char *orig_cpus;
> + unsigned char *sched_cpus;
why not use cpu_set_t directly?
jirka
[PATCH v2 4/9] perf affinity: Add infrastructure to save/restore affinity
From: Andi Kleen
The kernel perf subsystem has to IPI to the target CPU for many
operations. On systems with many CPUs and when managing many events the
overhead can be dominated by lots of IPIs.
An alternative is to set up CPU affinity in the perf tool, then set up
all the events for that CPU, and then move on to the next CPU.
Add some affinity management infrastructure to enable such a model.
Used in followon patches.
Signed-off-by: Andi Kleen
---
tools/perf/util/Build | 1 +
tools/perf/util/affinity.c | 71 ++
tools/perf/util/affinity.h | 15
3 files changed, 87 insertions(+)
create mode 100644 tools/perf/util/affinity.c
create mode 100644 tools/perf/util/affinity.h
diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index 2c1504fe924c..c7d4eab017e5 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -76,6 +76,7 @@ perf-y += sort.o
perf-y += hist.o
perf-y += util.o
perf-y += cpumap.o
+perf-y += affinity.o
perf-y += cputopo.o
perf-y += cgroup.o
perf-y += target.o
diff --git a/tools/perf/util/affinity.c b/tools/perf/util/affinity.c
new file mode 100644
index ..c42a6b9d63f0
--- /dev/null
+++ b/tools/perf/util/affinity.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Manage affinity to optimize IPIs inside the kernel perf API. */
+#define _GNU_SOURCE 1
+#include
+#include
+#include
+#include "perf.h"
+#include "cpumap.h"
+#include "affinity.h"
+
+static int get_cpu_set_size(void)
+{
+ int sz = (cpu__max_cpu() + 64 - 1) / 64;
+ /*
+* sched_getaffinity doesn't like masks smaller than the kernel.
+* Hopefully that's big enough.
+*/
+ if (sz < 4096/8)
+ sz = 4096/8;
+ return sz;
+}
+
+int affinity__setup(struct affinity *a)
+{
+ int cpu_set_size = get_cpu_set_size();
+
+ a->orig_cpus = malloc(cpu_set_size);
+ if (!a->orig_cpus)
+ return -1;
+ sched_getaffinity(0, cpu_set_size, (cpu_set_t *)a->orig_cpus);
+ a->sched_cpus = zalloc(cpu_set_size);
+ if (!a->sched_cpus) {
+ free(a->orig_cpus);
+ return -1;
+ }
+ a->changed = false;
+ return 0;
+}
+
+/*
+ * perf_event_open does an IPI internally to the target CPU.
+ * It is more efficient to change perf's affinity to the target
+ * CPU and then set up all events on that CPU, so we amortize
+ * CPU communication.
+ */
+void affinity__set(struct affinity *a, int cpu)
+{
+ int cpu_set_size = get_cpu_set_size();
+
+ if (cpu == -1)
+ return;
+ a->changed = true;
+ a->sched_cpus[cpu / 8] |= 1 << (cpu % 8);
+ /*
+* We ignore errors because affinity is just an optimization.
+* This could happen for example with isolated CPUs or cpusets.
+* In this case the IPIs inside the kernel's perf API still work.
+*/
+ sched_setaffinity(0, cpu_set_size, (cpu_set_t *)a->sched_cpus);
+ a->sched_cpus[cpu / 8] ^= 1 << (cpu % 8);
+}
+
+void affinity__cleanup(struct affinity *a)
+{
+ int cpu_set_size = get_cpu_set_size();
+
+ if (a->changed)
+ sched_setaffinity(0, cpu_set_size, (cpu_set_t *)a->orig_cpus);
+ free(a->sched_cpus);
+ free(a->orig_cpus);
+}
diff --git a/tools/perf/util/affinity.h b/tools/perf/util/affinity.h
new file mode 100644
index ..e56148607e33
--- /dev/null
+++ b/tools/perf/util/affinity.h
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+#ifndef AFFINITY_H
+#define AFFINITY_H 1
+
+struct affinity {
+ unsigned char *orig_cpus;
+ unsigned char *sched_cpus;
+ bool changed;
+};
+
+void affinity__cleanup(struct affinity *a);
+void affinity__set(struct affinity *a, int cpu);
+int affinity__setup(struct affinity *a);
+
+#endif
--
2.21.0
