Re: [patch] inherited events not signalling parent on overflow
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] inherited events not signalling parent on overflow
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] inherited events not signalling parent on overflow
On Thu, 2015-06-11 at 00:30 -0400, Vince Weaver wrote:
> On Fri, 29 May 2015, Ingo Molnar wrote:
>
> > * Vince Weaver 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
> > >
> > > 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);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] inherited events not signalling parent on overflow
On Fri, 29 May 2015, Ingo Molnar wrote: > * Vince Weaver 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 > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] inherited events not signalling parent on overflow
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] inherited events not signalling parent on overflow
* 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. > > 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 > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] inherited events not signalling parent on overflow
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 > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
