Hi All,
Please review the following patch, the details of which are
described in the commit message below.
Basic perf commands (and memory tracing event '-e mem') were tested
to work fine with this patch and can be applied over commit
b1dab2f0409c478fd2d9e227c2c018524eca9603.
Kindly let me know if there could be a better approach or if there's a
need for further testing.
Thanks,
K.Prasad
---
While debugging a warning message on PowerPC while using hardware
breakpoints, it was discovered that when perf_event_disable is invoked
through hw_breakpoint_handler function with interrupts disabled, a
subsequent IPI in the code path would trigger a WARN_ON_ONCE message in
smp_call_function_single function.
This patch avoids the use of an IPI to invoke __perf_event_disable when
it is safe to do so i.e. when the process associated with the perf-event
would run on the current CPU and interrupts are disabled on the system.
Since interrupts are always disabled before hw_breakpoint_handler in
PowerPC, the warning message will no longer be seen.
Reported-by: Edjunior Barbosa Machado <[email protected]>
Signed-off-by: K.Prasad <[email protected]>
---
kernel/events/core.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index fd126f8..0e2c1eb 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1302,6 +1302,7 @@ static int __perf_event_disable(void *info)
*/
void perf_event_disable(struct perf_event *event)
{
+ int ret;
struct perf_event_context *ctx = event->ctx;
struct task_struct *task = ctx->task;
@@ -1314,6 +1315,17 @@ void perf_event_disable(struct perf_event *event)
}
retry:
+ /*
+ * perf_event_disable may be called when interrupts are disabled.
+ * For e.g. hw_breakpoint_handler exception in PowerPC. Hence using
+ * IPIs to invoke __perf_event_disable is not always suitable. When
+ * possible invoke __perf_event_disable directly.
+ */
+ if ((task_cpu(task) == smp_processor_id()) && irqs_disabled()) {
+ ret = __perf_event_disable(event);
+ if (!ret)
+ return;
+ }
if (!task_function_call(task, __perf_event_disable, event))
return;
On Mon, 2012-06-11 at 11:32 +0530, K.Prasad wrote:
> While debugging a warning message on PowerPC while using hardware
> breakpoints, it was discovered that when perf_event_disable is invoked
> through hw_breakpoint_handler function with interrupts disabled, a
> subsequent IPI in the code path would trigger a WARN_ON_ONCE message in
> smp_call_function_single function.
>
> This patch avoids the use of an IPI to invoke __perf_event_disable when
> it is safe to do so i.e. when the process associated with the perf-event
> would run on the current CPU and interrupts are disabled on the system.
> Since interrupts are always disabled before hw_breakpoint_handler in
> PowerPC, the warning message will no longer be seen.
>
> Reported-by: Edjunior Barbosa Machado <[email protected]>
> Signed-off-by: K.Prasad <[email protected]>
> ---
> kernel/events/core.c | 12 ++++++++++++
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index fd126f8..0e2c1eb 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -1302,6 +1302,7 @@ static int __perf_event_disable(void *info)
> */
> void perf_event_disable(struct perf_event *event)
> {
> + int ret;
> struct perf_event_context *ctx = event->ctx;
> struct task_struct *task = ctx->task;
>
> @@ -1314,6 +1315,17 @@ void perf_event_disable(struct perf_event *event)
> }
>
> retry:
> + /*
> + * perf_event_disable may be called when interrupts are disabled.
> + * For e.g. hw_breakpoint_handler exception in PowerPC. Hence using
> + * IPIs to invoke __perf_event_disable is not always suitable. When
> + * possible invoke __perf_event_disable directly.
> + */
> + if ((task_cpu(task) == smp_processor_id()) && irqs_disabled()) {
Urgh..
So what's the callchain for the ppc->hw_bp->perf that triggers this?
> + ret = __perf_event_disable(event);
> + if (!ret)
> + return;
> + }
> if (!task_function_call(task, __perf_event_disable, event))
> return;
>
>
On Mon, Jun 11, 2012 at 01:13:33PM +0200, Peter Zijlstra wrote:
> On Mon, 2012-06-11 at 11:32 +0530, K.Prasad wrote:
>
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index fd126f8..0e2c1eb 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -1302,6 +1302,7 @@ static int __perf_event_disable(void *info)
> > */
> > void perf_event_disable(struct perf_event *event)
> > {
> > + int ret;
> > struct perf_event_context *ctx = event->ctx;
> > struct task_struct *task = ctx->task;
> >
> > @@ -1314,6 +1315,17 @@ void perf_event_disable(struct perf_event *event)
> > }
> >
> > retry:
> > + /*
> > + * perf_event_disable may be called when interrupts are disabled.
> > + * For e.g. hw_breakpoint_handler exception in PowerPC. Hence using
> > + * IPIs to invoke __perf_event_disable is not always suitable. When
> > + * possible invoke __perf_event_disable directly.
> > + */
> > + if ((task_cpu(task) == smp_processor_id()) && irqs_disabled()) {
>
> Urgh..
>
> So what's the callchain for the ppc->hw_bp->perf that triggers this?
Hardware breakpoints for user-space have traditionally operated in a
one-shot mode i.e. breakpoint is disabled after the first hit by
invoking perf_event_disable from hw_breakpoint_handler.
PowerPC server class processors follow 'trigger-before-execute', wherein
the breakpoint exception is triggered before the instruction performing
the memory access is executed. So the one-shot mode prevents the
processor from entering an infinite loop and the debugging tools like
GDB re-enable the breakpoints explicitly after a SIGTRAP.
Thanks,
K.Prasad
On Tue, 2012-06-12 at 11:36 +0530, K.Prasad wrote:
> On Mon, Jun 11, 2012 at 01:13:33PM +0200, Peter Zijlstra wrote:
> > On Mon, 2012-06-11 at 11:32 +0530, K.Prasad wrote:
> >
> > > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > > index fd126f8..0e2c1eb 100644
> > > --- a/kernel/events/core.c
> > > +++ b/kernel/events/core.c
> > > @@ -1302,6 +1302,7 @@ static int __perf_event_disable(void *info)
> > > */
> > > void perf_event_disable(struct perf_event *event)
> > > {
> > > + int ret;
> > > struct perf_event_context *ctx = event->ctx;
> > > struct task_struct *task = ctx->task;
> > >
> > > @@ -1314,6 +1315,17 @@ void perf_event_disable(struct perf_event *event)
> > > }
> > >
> > > retry:
> > > + /*
> > > + * perf_event_disable may be called when interrupts are disabled.
> > > + * For e.g. hw_breakpoint_handler exception in PowerPC. Hence using
> > > + * IPIs to invoke __perf_event_disable is not always suitable. When
> > > + * possible invoke __perf_event_disable directly.
> > > + */
> > > + if ((task_cpu(task) == smp_processor_id()) && irqs_disabled()) {
> >
> > Urgh..
> >
> > So what's the callchain for the ppc->hw_bp->perf that triggers this?
>
> Hardware breakpoints for user-space have traditionally operated in a
> one-shot mode i.e. breakpoint is disabled after the first hit by
> invoking perf_event_disable from hw_breakpoint_handler.
I take it this is the same across architectures? So basically everybody
suffers this?
Hmm,. x86 doesn't seem to do this.. are you saying breakpoint semantics
differ across architectures? Really?
Oh man how I do hate this breakpoint crap.. I guess you might as well
use __perf_event_disable() directly, no point in butchering
perf_event_disable() or even task_function_call().