* [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: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-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-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.