[patch] inherited events not signalling parent on overflow

[patch] inherited events not signalling parent on overflow

[Vince Weaver]

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
 	 */

Re: [patch] inherited events not signalling parent on overflow

[Peter Zijlstra]

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.

Re: [patch] inherited events not signalling parent on overflow

[Ingo Molnar]

* 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

Re: [patch] inherited events not signalling parent on overflow

[Vince Weaver]

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

Re: [patch] inherited events not signalling parent on overflow

[Vince Weaver]

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

Re: [patch] inherited events not signalling parent on overflow

[Peter Zijlstra]

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);
 	}

Re: [patch] inherited events not signalling parent on overflow

[Vince Weaver]

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

Re: [patch] inherited events not signalling parent on overflow

[Peter Zijlstra]

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.

[tip:perf/urgent] perf: Fix fasync handling on inherited events

[tip-bot for Peter Zijlstra]

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);
 	}