* [patch] inherited events not signalling parent on overflow
@ 2015-05-28 19:06 Vince Weaver
2015-05-28 19:15 ` Peter Zijlstra
2015-05-29 6:36 ` Ingo Molnar
0 siblings, 2 replies; 9+ messages in thread
From: Vince Weaver @ 2015-05-28 19:06 UTC (permalink / raw)
To: linux-kernel
Cc: mingo, peterz, eranian, Paul Mackerras, Arnaldo Carvalho de Melo
We're trying to get self-monitoring multi-threaded sampling working in
PAPI. Fun times.
Is this even possible?
Ideally in your parent thread you could perf_event_open() with
inherit set. Then your program (say an OpenMP program) would do its thing
and all of the samples would get written back to the parent thread's
mmap() buffer.
But this won't work as mmap'ing an inherited event is explicitly
disasllowed in events.c due to "performance reasons".
Which is believable, it's just there's not really a good alternative that
doesn't involve having to somehow manually instrument every single
possible thread.
on a related note, I turned up the following issue when working on this
issue. I don't know if this is the proper fix but it makes my simple test
case behave as expected.
If we inherit events, we inherit the signal state but not the fasync
state, so overflows in inherited children will never trigger the signal
handler.
Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 1a3bf48..7df4cf5 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8626,6 +8630,8 @@ inherit_event(struct perf_event *parent_event,
child_event->overflow_handler_context
= parent_event->overflow_handler_context;
+ child_event->fasync = parent_event->fasync;
+
/*
* Precalculate sample_data sizes
*/
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [patch] inherited events not signalling parent on overflow
2015-05-28 19:06 [patch] inherited events not signalling parent on overflow Vince Weaver
@ 2015-05-28 19:15 ` Peter Zijlstra
2015-05-29 16:45 ` Vince Weaver
2015-05-29 6:36 ` Ingo Molnar
1 sibling, 1 reply; 9+ messages in thread
From: Peter Zijlstra @ 2015-05-28 19:15 UTC (permalink / raw)
To: Vince Weaver
Cc: linux-kernel, mingo, eranian, Paul Mackerras, Arnaldo Carvalho de Melo
On Thu, 2015-05-28 at 15:06 -0400, Vince Weaver wrote:
> We're trying to get self-monitoring multi-threaded sampling working in
> PAPI. Fun times.
>
> Is this even possible?
>
> Ideally in your parent thread you could perf_event_open() with
> inherit set. Then your program (say an OpenMP program) would do its thing
> and all of the samples would get written back to the parent thread's
> mmap() buffer.
>
> But this won't work as mmap'ing an inherited event is explicitly
> disasllowed in events.c due to "performance reasons".
>
> Which is believable, it's just there's not really a good alternative that
> doesn't involve having to somehow manually instrument every single
> possible thread.
What could maybe work -- I'd have to check the code -- is open a
per-task-per-cpu counter for every cpu. Those we could inherit -- if we
currently allow it I'm not sure of.
The 'problem' is having multiple CPUs write into the same buffer, that's
bad for performance because cacheline contention and the requirement for
using atomic operations.
Using per-task-per-cpu events side steps that. Of course, then you get
to deal with nr_cpus buffers.
> on a related note, I turned up the following issue when working on this
> issue. I don't know if this is the proper fix but it makes my simple test
> case behave as expected.
>
>
>
> If we inherit events, we inherit the signal state but not the fasync
> state, so overflows in inherited children will never trigger the signal
> handler.
>
> Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 1a3bf48..7df4cf5 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8626,6 +8630,8 @@ inherit_event(struct perf_event *parent_event,
> child_event->overflow_handler_context
> = parent_event->overflow_handler_context;
>
> + child_event->fasync = parent_event->fasync;
> +
> /*
> * Precalculate sample_data sizes
> */
Sounds right; but I've forgotten everything there is to forget about
fasync. I'll go dig through those details again.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch] inherited events not signalling parent on overflow
2015-05-28 19:06 [patch] inherited events not signalling parent on overflow Vince Weaver
2015-05-28 19:15 ` Peter Zijlstra
@ 2015-05-29 6:36 ` Ingo Molnar
2015-06-11 4:30 ` Vince Weaver
1 sibling, 1 reply; 9+ messages in thread
From: Ingo Molnar @ 2015-05-29 6:36 UTC (permalink / raw)
To: Vince Weaver
Cc: linux-kernel, peterz, eranian, Paul Mackerras, Arnaldo Carvalho de Melo
* Vince Weaver <vincent.weaver@maine.edu> wrote:
> We're trying to get self-monitoring multi-threaded sampling working in PAPI.
> Fun times.
>
> Is this even possible?
>
> Ideally in your parent thread you could perf_event_open() with inherit set.
> Then your program (say an OpenMP program) would do its thing and all of the
> samples would get written back to the parent thread's mmap() buffer.
>
> But this won't work as mmap'ing an inherited event is explicitly disasllowed in
> events.c due to "performance reasons".
>
> Which is believable, it's just there's not really a good alternative that
> doesn't involve having to somehow manually instrument every single possible
> thread.
>
> on a related note, I turned up the following issue when working on this issue.
> I don't know if this is the proper fix but it makes my simple test case behave
> as expected.
>
> If we inherit events, we inherit the signal state but not the fasync state, so
> overflows in inherited children will never trigger the signal handler.
>
> Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 1a3bf48..7df4cf5 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8626,6 +8630,8 @@ inherit_event(struct perf_event *parent_event,
> child_event->overflow_handler_context
> = parent_event->overflow_handler_context;
>
> + child_event->fasync = parent_event->fasync;
> +
> /*
> * Precalculate sample_data sizes
> */
Btw., if we do this (sensible looking) ABI fix, could we make it a new attr bit,
so that PAPI can essentially query the kernel whether this gets propagated
properly?
That way old kernels 'intentionally' don't inherit the fasync handler and tooling
can deterministically make use of this 'feature' on new kernels.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch] inherited events not signalling parent on overflow
2015-05-28 19:15 ` Peter Zijlstra
@ 2015-05-29 16:45 ` Vince Weaver
0 siblings, 0 replies; 9+ messages in thread
From: Vince Weaver @ 2015-05-29 16:45 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Vince Weaver, linux-kernel, mingo, eranian, Paul Mackerras,
Arnaldo Carvalho de Melo
On Thu, 28 May 2015, Peter Zijlstra wrote:
> What could maybe work -- I'd have to check the code -- is open a
> per-task-per-cpu counter for every cpu. Those we could inherit -- if we
> currently allow it I'm not sure of.
yes, that seems to work. It's a bit of a pain having to juggle so many
buffers, but that's much better than not being able to measure at all.
Vince
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch] inherited events not signalling parent on overflow
2015-05-29 6:36 ` Ingo Molnar
@ 2015-06-11 4:30 ` Vince Weaver
2015-06-11 8:32 ` Peter Zijlstra
0 siblings, 1 reply; 9+ messages in thread
From: Vince Weaver @ 2015-06-11 4:30 UTC (permalink / raw)
To: Ingo Molnar
Cc: Vince Weaver, linux-kernel, peterz, eranian, Paul Mackerras,
Arnaldo Carvalho de Melo
On Fri, 29 May 2015, Ingo Molnar wrote:
> * Vince Weaver <vincent.weaver@maine.edu> wrote:
> > If we inherit events, we inherit the signal state but not the fasync state, so
> > overflows in inherited children will never trigger the signal handler.
> >
> > Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>
> >
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 1a3bf48..7df4cf5 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -8626,6 +8630,8 @@ inherit_event(struct perf_event *parent_event,
> > child_event->overflow_handler_context
> > = parent_event->overflow_handler_context;
> >
> > + child_event->fasync = parent_event->fasync;
> > +
> > /*
> > * Precalculate sample_data sizes
> > */
This patch, while it does work well enough to enable self-monitored-sampling
of OpenMP programs, falls apart under fuzzing.
You end up with lots of
[25592.289382] kill_fasync: bad magic number in fasync_struct!
warnings and eventually I managed to lock up the system that way.
> Btw., if we do this (sensible looking) ABI fix, could we make it a new attr bit,
> so that PAPI can essentially query the kernel whether this gets propagated
> properly?
>
> That way old kernels 'intentionally' don't inherit the fasync handler and tooling
> can deterministically make use of this 'feature' on new kernels.
That would be useful. PAPI typically has to guess about feature support
(for workarounds) by using the kernel version number as a reference, and
this falls apart on kernels such as RHEL which backport a lot of
perf_event fixes/functionality.
Vince
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch] inherited events not signalling parent on overflow
2015-06-11 4:30 ` Vince Weaver
@ 2015-06-11 8:32 ` Peter Zijlstra
2015-07-31 4:42 ` Vince Weaver
2015-08-04 8:51 ` [tip:perf/urgent] perf: Fix fasync handling on inherited events tip-bot for Peter Zijlstra
0 siblings, 2 replies; 9+ messages in thread
From: Peter Zijlstra @ 2015-06-11 8:32 UTC (permalink / raw)
To: Vince Weaver
Cc: Ingo Molnar, linux-kernel, eranian, Paul Mackerras,
Arnaldo Carvalho de Melo
On Thu, 2015-06-11 at 00:30 -0400, Vince Weaver wrote:
> On Fri, 29 May 2015, Ingo Molnar wrote:
>
> > * Vince Weaver <vincent.weaver@maine.edu> wrote:
>
> > > If we inherit events, we inherit the signal state but not the fasync state, so
> > > overflows in inherited children will never trigger the signal handler.
> > >
> > > Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>
> > >
> > > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > > index 1a3bf48..7df4cf5 100644
> > > --- a/kernel/events/core.c
> > > +++ b/kernel/events/core.c
> > > @@ -8626,6 +8630,8 @@ inherit_event(struct perf_event *parent_event,
> > > child_event->overflow_handler_context
> > > = parent_event->overflow_handler_context;
> > >
> > > + child_event->fasync = parent_event->fasync;
> > > +
> > > /*
> > > * Precalculate sample_data sizes
> > > */
>
> This patch, while it does work well enough to enable self-monitored-sampling
> of OpenMP programs, falls apart under fuzzing.
>
> You end up with lots of
>
> [25592.289382] kill_fasync: bad magic number in fasync_struct!
>
> warnings and eventually I managed to lock up the system that way.
Right, I had a peek earlier at how fasync worked but came away confused.
Today I seem to have had better luck. Installing fasync allocates memory
and sets filp->f_flags |= FASYNC, which upon the demise of the file
descriptor ensures the allocation is freed.
Now for perf, we can have the events stick around for a while after the
original FD is dead because of references from child events. With the
above patch these events would still have a pointer into this free'd
fasync. This is bad.
A further problem with the patch is that if the parent changes its
fasync state the children might lag and again have pointers into dead
space.
All is not lost though; does something like the below work?
---
kernel/events/core.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 1e33b9141f03..057f599ae0dc 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4742,12 +4742,20 @@ static const struct file_operations perf_fops = {
* to user-space before waking everybody up.
*/
+static inline struct fasync_struct **perf_event_fasync(struct perf_event *event)
+{
+ /* only the parent has fasync state */
+ if (event->parent)
+ event = event->parent;
+ return &event->fasync;
+}
+
void perf_event_wakeup(struct perf_event *event)
{
ring_buffer_wakeup(event);
if (event->pending_kill) {
- kill_fasync(&event->fasync, SIGIO, event->pending_kill);
+ kill_fasync(perf_event_fasync(event), SIGIO, event->pending_kill);
event->pending_kill = 0;
}
}
@@ -6126,7 +6134,7 @@ static int __perf_event_overflow(struct perf_event *event,
else
perf_event_output(event, data, regs);
- if (event->fasync && event->pending_kill) {
+ if (*perf_event_fasync(event) && event->pending_kill) {
event->pending_wakeup = 1;
irq_work_queue(&event->pending);
}
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [patch] inherited events not signalling parent on overflow
2015-06-11 8:32 ` Peter Zijlstra
@ 2015-07-31 4:42 ` Vince Weaver
2015-07-31 9:26 ` Peter Zijlstra
2015-08-04 8:51 ` [tip:perf/urgent] perf: Fix fasync handling on inherited events tip-bot for Peter Zijlstra
1 sibling, 1 reply; 9+ messages in thread
From: Vince Weaver @ 2015-07-31 4:42 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, linux-kernel, eranian, Paul Mackerras,
Arnaldo Carvalho de Melo
On Thu, 11 Jun 2015, Peter Zijlstra wrote:
> Right, I had a peek earlier at how fasync worked but came away confused.
>
> Today I seem to have had better luck. Installing fasync allocates memory
> and sets filp->f_flags |= FASYNC, which upon the demise of the file
> descriptor ensures the allocation is freed.
>
> Now for perf, we can have the events stick around for a while after the
> original FD is dead because of references from child events. With the
> above patch these events would still have a pointer into this free'd
> fasync. This is bad.
>
> A further problem with the patch is that if the parent changes its
> fasync state the children might lag and again have pointers into dead
> space.
>
> All is not lost though; does something like the below work?
I had meant to reply to this earlier but maybe I forgot.
I've been running with this patch for a month now and haven't had
problems, and it fixes the issue of inherited signals. So it no one else
has issues with the patch it would be nice if it could be pushed upstream.
Thanks,
Vince
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch] inherited events not signalling parent on overflow
2015-07-31 4:42 ` Vince Weaver
@ 2015-07-31 9:26 ` Peter Zijlstra
0 siblings, 0 replies; 9+ messages in thread
From: Peter Zijlstra @ 2015-07-31 9:26 UTC (permalink / raw)
To: Vince Weaver
Cc: Ingo Molnar, linux-kernel, eranian, Paul Mackerras,
Arnaldo Carvalho de Melo
On Fri, Jul 31, 2015 at 12:42:06AM -0400, Vince Weaver wrote:
> On Thu, 11 Jun 2015, Peter Zijlstra wrote:
>
> > Right, I had a peek earlier at how fasync worked but came away confused.
> >
> > Today I seem to have had better luck. Installing fasync allocates memory
> > and sets filp->f_flags |= FASYNC, which upon the demise of the file
> > descriptor ensures the allocation is freed.
> >
> > Now for perf, we can have the events stick around for a while after the
> > original FD is dead because of references from child events. With the
> > above patch these events would still have a pointer into this free'd
> > fasync. This is bad.
> >
> > A further problem with the patch is that if the parent changes its
> > fasync state the children might lag and again have pointers into dead
> > space.
> >
> > All is not lost though; does something like the below work?
>
> I had meant to reply to this earlier but maybe I forgot.
>
> I've been running with this patch for a month now and haven't had
> problems, and it fixes the issue of inherited signals. So it no one else
> has issues with the patch it would be nice if it could be pushed upstream.
Great, thanks for testing. I'll go queue it.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [tip:perf/urgent] perf: Fix fasync handling on inherited events
2015-06-11 8:32 ` Peter Zijlstra
2015-07-31 4:42 ` Vince Weaver
@ 2015-08-04 8:51 ` tip-bot for Peter Zijlstra
1 sibling, 0 replies; 9+ messages in thread
From: tip-bot for Peter Zijlstra @ 2015-08-04 8:51 UTC (permalink / raw)
To: linux-tip-commits
Cc: acme, tglx, peterz, vincent.weaver, stable, torvalds,
linux-kernel, mingo, hpa
Commit-ID: fed66e2cdd4f127a43fd11b8d92a99bdd429528c
Gitweb: http://git.kernel.org/tip/fed66e2cdd4f127a43fd11b8d92a99bdd429528c
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Thu, 11 Jun 2015 10:32:01 +0200
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 4 Aug 2015 09:57:52 +0200
perf: Fix fasync handling on inherited events
Vince reported that the fasync signal stuff doesn't work proper for
inherited events. So fix that.
Installing fasync allocates memory and sets filp->f_flags |= FASYNC,
which upon the demise of the file descriptor ensures the allocation is
freed and state is updated.
Now for perf, we can have the events stick around for a while after the
original FD is dead because of references from child events. So we
cannot copy the fasync pointer around. We can however consistently use
the parent's fasync, as that will be updated.
Reported-and-Tested-by: Vince Weaver <vincent.weaver@maine.edu>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: <stable@vger.kernel.org>
Cc: Arnaldo Carvalho deMelo <acme@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: eranian@google.com
Link: http://lkml.kernel.org/r/1434011521.1495.71.camel@twins
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
kernel/events/core.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 10d076b..072b8a6 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4740,12 +4740,20 @@ static const struct file_operations perf_fops = {
* to user-space before waking everybody up.
*/
+static inline struct fasync_struct **perf_event_fasync(struct perf_event *event)
+{
+ /* only the parent has fasync state */
+ if (event->parent)
+ event = event->parent;
+ return &event->fasync;
+}
+
void perf_event_wakeup(struct perf_event *event)
{
ring_buffer_wakeup(event);
if (event->pending_kill) {
- kill_fasync(&event->fasync, SIGIO, event->pending_kill);
+ kill_fasync(perf_event_fasync(event), SIGIO, event->pending_kill);
event->pending_kill = 0;
}
}
@@ -6124,7 +6132,7 @@ static int __perf_event_overflow(struct perf_event *event,
else
perf_event_output(event, data, regs);
- if (event->fasync && event->pending_kill) {
+ if (*perf_event_fasync(event) && event->pending_kill) {
event->pending_wakeup = 1;
irq_work_queue(&event->pending);
}
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-08-04 8:52 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-28 19:06 [patch] inherited events not signalling parent on overflow Vince Weaver
2015-05-28 19:15 ` Peter Zijlstra
2015-05-29 16:45 ` Vince Weaver
2015-05-29 6:36 ` Ingo Molnar
2015-06-11 4:30 ` Vince Weaver
2015-06-11 8:32 ` Peter Zijlstra
2015-07-31 4:42 ` Vince Weaver
2015-07-31 9:26 ` Peter Zijlstra
2015-08-04 8:51 ` [tip:perf/urgent] perf: Fix fasync handling on inherited events tip-bot for Peter Zijlstra
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.