All of lore.kernel.org
 help / color / mirror / Atom feed
* [Patch][perf] Invoke __perf_event_disable without an IPI
@ 2012-06-11  6:02 K.Prasad
  2012-06-11 11:13 ` Peter Zijlstra
  0 siblings, 1 reply; 17+ messages in thread
From: K.Prasad @ 2012-06-11  6:02 UTC (permalink / raw)
  To: Peter Zijlstra, Paul Mackerras, Frederic Weisbecker
  Cc: Ingo Molnar, Arnaldo Carvalho de Melo, Linux Kernel Mailing List,
	Edjunior Barbosa Machado, Naveen N. Rao

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 <emachado@linux.vnet.ibm.com>
Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 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;
 


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [Patch][perf] Invoke __perf_event_disable without an IPI
  2012-06-11  6:02 [Patch][perf] Invoke __perf_event_disable without an IPI K.Prasad
@ 2012-06-11 11:13 ` Peter Zijlstra
  2012-06-12  6:06   ` K.Prasad
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Zijlstra @ 2012-06-11 11:13 UTC (permalink / raw)
  To: prasad
  Cc: Paul Mackerras, Frederic Weisbecker, Ingo Molnar,
	Arnaldo Carvalho de Melo, Linux Kernel Mailing List,
	Edjunior Barbosa Machado, Naveen N. Rao

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 <emachado@linux.vnet.ibm.com>
> Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
> ---
>  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;
>  
> 


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Patch][perf] Invoke __perf_event_disable without an IPI
  2012-06-11 11:13 ` Peter Zijlstra
@ 2012-06-12  6:06   ` K.Prasad
  2012-06-12  9:12     ` Peter Zijlstra
  0 siblings, 1 reply; 17+ messages in thread
From: K.Prasad @ 2012-06-12  6:06 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Paul Mackerras, Frederic Weisbecker, Ingo Molnar,
	Arnaldo Carvalho de Melo, Linux Kernel Mailing List,
	Edjunior Barbosa Machado, Naveen N. Rao

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Patch][perf] Invoke __perf_event_disable without an IPI
  2012-06-12  6:06   ` K.Prasad
@ 2012-06-12  9:12     ` Peter Zijlstra
  2012-07-06  9:52       ` [PATCH v2] Hardware breakpoints: Invoke __perf_event_disable() if interrupts are already disabled Naveen N. Rao
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Zijlstra @ 2012-06-12  9:12 UTC (permalink / raw)
  To: prasad
  Cc: Paul Mackerras, Frederic Weisbecker, Ingo Molnar,
	Arnaldo Carvalho de Melo, Linux Kernel Mailing List,
	Edjunior Barbosa Machado, Naveen N. Rao

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().

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2] Hardware breakpoints: Invoke __perf_event_disable() if interrupts are already disabled
  2012-06-12  9:12     ` Peter Zijlstra
@ 2012-07-06  9:52       ` Naveen N. Rao
  2012-07-06 10:18         ` Naveen N. Rao
  0 siblings, 1 reply; 17+ messages in thread
From: Naveen N. Rao @ 2012-07-06  9:52 UTC (permalink / raw)
  To: fweisbec, paulus, a.p.zijlstra
  Cc: emachado, mingo, linux-kernel, acme, prasad.krishnan

Please find v2 of the patch from Prasad, based on Peter Zijlstra's
feedback. This applies on top of v3.5-rc5. This has been tested and
found to work fine by Edjunior.

Regards,
Naveen

---
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 calls __perf_event_disable() when interrupts are already
disabled, instead of perf_event_disable().

Reported-by: Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com>
Signed-off-by: K.Prasad <Prasad.Krishnan@gmail.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 include/linux/perf_event.h    |    2 ++
 kernel/events/core.c          |    2 +-
 kernel/events/hw_breakpoint.c |   10 +++++++++-
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 45db49f..c289ba0 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1292,6 +1292,7 @@ extern int perf_swevent_get_recursion_context(void);
 extern void perf_swevent_put_recursion_context(int rctx);
 extern void perf_event_enable(struct perf_event *event);
 extern void perf_event_disable(struct perf_event *event);
+extern int __perf_event_disable(void *info);
 extern void perf_event_task_tick(void);
 #else
 static inline void
@@ -1330,6 +1331,7 @@ static inline int  perf_swevent_get_recursion_context(void)		{ return -1; }
 static inline void perf_swevent_put_recursion_context(int rctx)		{ }
 static inline void perf_event_enable(struct perf_event *event)		{ }
 static inline void perf_event_disable(struct perf_event *event)		{ }
+static inline int __perf_event_disable(void *info)			{ }
 static inline void perf_event_task_tick(void)				{ }
 #endif
 
diff --git a/kernel/events/core.c b/kernel/events/core.c
index d7d71d6..0ad0fc9 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1253,7 +1253,7 @@ retry:
 /*
  * Cross CPU call to disable a performance event
  */
-static int __perf_event_disable(void *info)
+int __perf_event_disable(void *info)
 {
 	struct perf_event *event = info;
 	struct perf_event_context *ctx = event->ctx;
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index bb38c4d..483f14a 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -453,7 +453,15 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
 	int old_type = bp->attr.bp_type;
 	int err = 0;
 
-	perf_event_disable(bp);
+	/*
+	 * modify_user_hw_breakpoint can be invoked with IRQs disabled and hence it
+	 * will not be possible to raise IPIs that invoke __perf_event_disable.
+	 * So call the function directly.
+	 */
+	if (irqs_disabled())
+		__perf_event_disable(bp);
+	else
+		perf_event_disable(bp);
 
 	bp->attr.bp_addr = attr->bp_addr;
 	bp->attr.bp_type = attr->bp_type;


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2] Hardware breakpoints: Invoke __perf_event_disable() if interrupts are already disabled
  2012-07-06  9:52       ` [PATCH v2] Hardware breakpoints: Invoke __perf_event_disable() if interrupts are already disabled Naveen N. Rao
@ 2012-07-06 10:18         ` Naveen N. Rao
  2012-07-18 10:30           ` [PATCH v2 RESEND] " Naveen N. Rao
  0 siblings, 1 reply; 17+ messages in thread
From: Naveen N. Rao @ 2012-07-06 10:18 UTC (permalink / raw)
  To: fweisbec, paulus, a.p.zijlstra
  Cc: emachado, mingo, linux-kernel, acme, prasad.krishnan

From: K.Prasad <Prasad.Krishnan@gmail.com>

My apologies. I missed the From header in my previous mail.

---
Please find v2 of the patch from Prasad, based on Peter Zijlstra's
feedback. This applies on top of v3.5-rc5. This has been tested and
found to work fine by Edjunior.

Regards,
Naveen

---
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 calls __perf_event_disable() when interrupts are already
disabled, instead of perf_event_disable().

Reported-by: Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com>
Signed-off-by: K.Prasad <Prasad.Krishnan@gmail.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 include/linux/perf_event.h    |    2 ++
 kernel/events/core.c          |    2 +-
 kernel/events/hw_breakpoint.c |   10 +++++++++-
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 45db49f..c289ba0 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1292,6 +1292,7 @@ extern int perf_swevent_get_recursion_context(void);
 extern void perf_swevent_put_recursion_context(int rctx);
 extern void perf_event_enable(struct perf_event *event);
 extern void perf_event_disable(struct perf_event *event);
+extern int __perf_event_disable(void *info);
 extern void perf_event_task_tick(void);
 #else
 static inline void
@@ -1330,6 +1331,7 @@ static inline int  perf_swevent_get_recursion_context(void)		{ return -1; }
 static inline void perf_swevent_put_recursion_context(int rctx)		{ }
 static inline void perf_event_enable(struct perf_event *event)		{ }
 static inline void perf_event_disable(struct perf_event *event)		{ }
+static inline int __perf_event_disable(void *info)			{ }
 static inline void perf_event_task_tick(void)				{ }
 #endif
 
diff --git a/kernel/events/core.c b/kernel/events/core.c
index d7d71d6..0ad0fc9 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1253,7 +1253,7 @@ retry:
 /*
  * Cross CPU call to disable a performance event
  */
-static int __perf_event_disable(void *info)
+int __perf_event_disable(void *info)
 {
 	struct perf_event *event = info;
 	struct perf_event_context *ctx = event->ctx;
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index bb38c4d..483f14a 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -453,7 +453,15 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
 	int old_type = bp->attr.bp_type;
 	int err = 0;
 
-	perf_event_disable(bp);
+	/*
+	 * modify_user_hw_breakpoint can be invoked with IRQs disabled and hence it
+	 * will not be possible to raise IPIs that invoke __perf_event_disable.
+	 * So call the function directly.
+	 */
+	if (irqs_disabled())
+		__perf_event_disable(bp);
+	else
+		perf_event_disable(bp);
 
 	bp->attr.bp_addr = attr->bp_addr;
 	bp->attr.bp_type = attr->bp_type;


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 RESEND] Hardware breakpoints: Invoke __perf_event_disable() if interrupts are already disabled
  2012-07-06 10:18         ` Naveen N. Rao
@ 2012-07-18 10:30           ` Naveen N. Rao
  2012-07-18 11:57             ` Frederic Weisbecker
  0 siblings, 1 reply; 17+ messages in thread
From: Naveen N. Rao @ 2012-07-18 10:30 UTC (permalink / raw)
  To: fweisbec, paulus, a.p.zijlstra
  Cc: linux-kernel, mingo, emachado, acme, prasad.krishnan

Please find v2 of the patch from Prasad, based on Peter Zijlstra's
feedback. This applies on top of v3.5-rc7. This has been tested and
found to work fine by Edjunior.

Regards,
Naveen
______

From: K.Prasad <Prasad.Krishnan@gmail.com>

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 calls __perf_event_disable() when interrupts are already
disabled, instead of perf_event_disable().

Reported-by: Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com>
Signed-off-by: K.Prasad <Prasad.Krishnan@gmail.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 include/linux/perf_event.h    |    2 ++
 kernel/events/core.c          |    2 +-
 kernel/events/hw_breakpoint.c |   10 +++++++++-
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 45db49f..c289ba0 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1292,6 +1292,7 @@ extern int perf_swevent_get_recursion_context(void);
 extern void perf_swevent_put_recursion_context(int rctx);
 extern void perf_event_enable(struct perf_event *event);
 extern void perf_event_disable(struct perf_event *event);
+extern int __perf_event_disable(void *info);
 extern void perf_event_task_tick(void);
 #else
 static inline void
@@ -1330,6 +1331,7 @@ static inline int  perf_swevent_get_recursion_context(void)		{ return -1; }
 static inline void perf_swevent_put_recursion_context(int rctx)		{ }
 static inline void perf_event_enable(struct perf_event *event)		{ }
 static inline void perf_event_disable(struct perf_event *event)		{ }
+static inline int __perf_event_disable(void *info)			{ }
 static inline void perf_event_task_tick(void)				{ }
 #endif
 
diff --git a/kernel/events/core.c b/kernel/events/core.c
index d7d71d6..0ad0fc9 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1253,7 +1253,7 @@ retry:
 /*
  * Cross CPU call to disable a performance event
  */
-static int __perf_event_disable(void *info)
+int __perf_event_disable(void *info)
 {
 	struct perf_event *event = info;
 	struct perf_event_context *ctx = event->ctx;
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index bb38c4d..483f14a 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -453,7 +453,15 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
 	int old_type = bp->attr.bp_type;
 	int err = 0;
 
-	perf_event_disable(bp);
+	/*
+	 * modify_user_hw_breakpoint can be invoked with IRQs disabled and hence it
+	 * will not be possible to raise IPIs that invoke __perf_event_disable.
+	 * So call the function directly.
+	 */
+	if (irqs_disabled())
+		__perf_event_disable(bp);
+	else
+		perf_event_disable(bp);
 
 	bp->attr.bp_addr = attr->bp_addr;
 	bp->attr.bp_type = attr->bp_type;


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 RESEND] Hardware breakpoints: Invoke __perf_event_disable() if interrupts are already disabled
  2012-07-18 10:30           ` [PATCH v2 RESEND] " Naveen N. Rao
@ 2012-07-18 11:57             ` Frederic Weisbecker
  2012-07-19 11:16               ` Naveen N. Rao
  0 siblings, 1 reply; 17+ messages in thread
From: Frederic Weisbecker @ 2012-07-18 11:57 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: paulus, a.p.zijlstra, linux-kernel, mingo, emachado, acme,
	prasad.krishnan

On Wed, Jul 18, 2012 at 04:00:46PM +0530, Naveen N. Rao wrote:
> Please find v2 of the patch from Prasad, based on Peter Zijlstra's
> feedback. This applies on top of v3.5-rc7. This has been tested and
> found to work fine by Edjunior.
> 
> Regards,
> Naveen
> ______
> 
> From: K.Prasad <Prasad.Krishnan@gmail.com>
> 
> 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 calls __perf_event_disable() when interrupts are already
> disabled, instead of perf_event_disable().
> 
> Reported-by: Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com>
> Signed-off-by: K.Prasad <Prasad.Krishnan@gmail.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  include/linux/perf_event.h    |    2 ++
>  kernel/events/core.c          |    2 +-
>  kernel/events/hw_breakpoint.c |   10 +++++++++-
>  3 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 45db49f..c289ba0 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -1292,6 +1292,7 @@ extern int perf_swevent_get_recursion_context(void);
>  extern void perf_swevent_put_recursion_context(int rctx);
>  extern void perf_event_enable(struct perf_event *event);
>  extern void perf_event_disable(struct perf_event *event);
> +extern int __perf_event_disable(void *info);
>  extern void perf_event_task_tick(void);
>  #else
>  static inline void
> @@ -1330,6 +1331,7 @@ static inline int  perf_swevent_get_recursion_context(void)		{ return -1; }
>  static inline void perf_swevent_put_recursion_context(int rctx)		{ }
>  static inline void perf_event_enable(struct perf_event *event)		{ }
>  static inline void perf_event_disable(struct perf_event *event)		{ }
> +static inline int __perf_event_disable(void *info)			{ }
>  static inline void perf_event_task_tick(void)				{ }
>  #endif
>  
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index d7d71d6..0ad0fc9 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -1253,7 +1253,7 @@ retry:
>  /*
>   * Cross CPU call to disable a performance event
>   */
> -static int __perf_event_disable(void *info)
> +int __perf_event_disable(void *info)
>  {
>  	struct perf_event *event = info;
>  	struct perf_event_context *ctx = event->ctx;
> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> index bb38c4d..483f14a 100644
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c
> @@ -453,7 +453,15 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
>  	int old_type = bp->attr.bp_type;
>  	int err = 0;
>  
> -	perf_event_disable(bp);
> +	/*
> +	 * modify_user_hw_breakpoint can be invoked with IRQs disabled and hence it
> +	 * will not be possible to raise IPIs that invoke __perf_event_disable.
> +	 * So call the function directly.
> +	 */
> +	if (irqs_disabled())
> +		__perf_event_disable(bp);
> +	else
> +		perf_event_disable(bp);

This only works if we are sure the bp is on the current CPU. Do we
have that guarantee?

>  
>  	bp->attr.bp_addr = attr->bp_addr;
>  	bp->attr.bp_type = attr->bp_type;
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 RESEND] Hardware breakpoints: Invoke __perf_event_disable() if interrupts are already disabled
  2012-07-18 11:57             ` Frederic Weisbecker
@ 2012-07-19 11:16               ` Naveen N. Rao
  2012-07-25 11:32                 ` Naveen N. Rao
  0 siblings, 1 reply; 17+ messages in thread
From: Naveen N. Rao @ 2012-07-19 11:16 UTC (permalink / raw)
  To: Frederic Weisbecker
  Cc: paulus, a.p.zijlstra, linux-kernel, mingo, emachado, acme,
	prasad.krishnan

On 07/18/2012 05:27 PM, Frederic Weisbecker wrote:
> On Wed, Jul 18, 2012 at 04:00:46PM +0530, Naveen N. Rao wrote:
>> Please find v2 of the patch from Prasad, based on Peter Zijlstra's
>> feedback. This applies on top of v3.5-rc7. This has been tested and
>> found to work fine by Edjunior.
>>
>> Regards,
>> Naveen
>> ______
>>
>> From: K.Prasad <Prasad.Krishnan@gmail.com>
>>
>> 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 calls __perf_event_disable() when interrupts are already
>> disabled, instead of perf_event_disable().
>>
>> Reported-by: Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com>
>> Signed-off-by: K.Prasad <Prasad.Krishnan@gmail.com>
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>   include/linux/perf_event.h    |    2 ++
>>   kernel/events/core.c          |    2 +-
>>   kernel/events/hw_breakpoint.c |   10 +++++++++-
>>   3 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>> index 45db49f..c289ba0 100644
>> --- a/include/linux/perf_event.h
>> +++ b/include/linux/perf_event.h
>> @@ -1292,6 +1292,7 @@ extern int perf_swevent_get_recursion_context(void);
>>   extern void perf_swevent_put_recursion_context(int rctx);
>>   extern void perf_event_enable(struct perf_event *event);
>>   extern void perf_event_disable(struct perf_event *event);
>> +extern int __perf_event_disable(void *info);
>>   extern void perf_event_task_tick(void);
>>   #else
>>   static inline void
>> @@ -1330,6 +1331,7 @@ static inline int  perf_swevent_get_recursion_context(void)		{ return -1; }
>>   static inline void perf_swevent_put_recursion_context(int rctx)		{ }
>>   static inline void perf_event_enable(struct perf_event *event)		{ }
>>   static inline void perf_event_disable(struct perf_event *event)		{ }
>> +static inline int __perf_event_disable(void *info)			{ }
>>   static inline void perf_event_task_tick(void)				{ }
>>   #endif
>>
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index d7d71d6..0ad0fc9 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -1253,7 +1253,7 @@ retry:
>>   /*
>>    * Cross CPU call to disable a performance event
>>    */
>> -static int __perf_event_disable(void *info)
>> +int __perf_event_disable(void *info)
>>   {
>>   	struct perf_event *event = info;
>>   	struct perf_event_context *ctx = event->ctx;
>> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
>> index bb38c4d..483f14a 100644
>> --- a/kernel/events/hw_breakpoint.c
>> +++ b/kernel/events/hw_breakpoint.c
>> @@ -453,7 +453,15 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
>>   	int old_type = bp->attr.bp_type;
>>   	int err = 0;
>>
>> -	perf_event_disable(bp);
>> +	/*
>> +	 * modify_user_hw_breakpoint can be invoked with IRQs disabled and hence it
>> +	 * will not be possible to raise IPIs that invoke __perf_event_disable.
>> +	 * So call the function directly.
>> +	 */
>> +	if (irqs_disabled())
>> +		__perf_event_disable(bp);
>> +	else
>> +		perf_event_disable(bp);
>
> This only works if we are sure the bp is on the current CPU. Do we
> have that guarantee?

Yes. This is being hit during bp exception processing and is specific to 
ppc where we disable interrupts: 
hw_breakpoint_handler->perf_bp_event->ptrace_triggered->modify_user_hw_breakpoint()


Regards,
Naveen

>
>>
>>   	bp->attr.bp_addr = attr->bp_addr;
>>   	bp->attr.bp_type = attr->bp_type;
>>
>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 RESEND] Hardware breakpoints: Invoke __perf_event_disable() if interrupts are already disabled
  2012-07-19 11:16               ` Naveen N. Rao
@ 2012-07-25 11:32                 ` Naveen N. Rao
  2012-07-31 13:41                   ` Frederic Weisbecker
  0 siblings, 1 reply; 17+ messages in thread
From: Naveen N. Rao @ 2012-07-25 11:32 UTC (permalink / raw)
  To: Frederic Weisbecker
  Cc: paulus, a.p.zijlstra, linux-kernel, mingo, emachado, acme,
	prasad.krishnan

On 07/19/2012 04:46 PM, Naveen N. Rao wrote:
> On 07/18/2012 05:27 PM, Frederic Weisbecker wrote:
>> On Wed, Jul 18, 2012 at 04:00:46PM +0530, Naveen N. Rao wrote:
>>> Please find v2 of the patch from Prasad, based on Peter Zijlstra's
>>> feedback. This applies on top of v3.5-rc7. This has been tested and
>>> found to work fine by Edjunior.
>>>
>>> Regards,
>>> Naveen
>>> ______
>>>
>>> From: K.Prasad <Prasad.Krishnan@gmail.com>
>>>
>>> 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 calls __perf_event_disable() when interrupts are already
>>> disabled, instead of perf_event_disable().
>>>
>>> Reported-by: Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com>
>>> Signed-off-by: K.Prasad <Prasad.Krishnan@gmail.com>
>>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>>> ---
>>>   include/linux/perf_event.h    |    2 ++
>>>   kernel/events/core.c          |    2 +-
>>>   kernel/events/hw_breakpoint.c |   10 +++++++++-
>>>   3 files changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>>> index 45db49f..c289ba0 100644
>>> --- a/include/linux/perf_event.h
>>> +++ b/include/linux/perf_event.h
>>> @@ -1292,6 +1292,7 @@ extern int
>>> perf_swevent_get_recursion_context(void);
>>>   extern void perf_swevent_put_recursion_context(int rctx);
>>>   extern void perf_event_enable(struct perf_event *event);
>>>   extern void perf_event_disable(struct perf_event *event);
>>> +extern int __perf_event_disable(void *info);
>>>   extern void perf_event_task_tick(void);
>>>   #else
>>>   static inline void
>>> @@ -1330,6 +1331,7 @@ static inline int
>>> perf_swevent_get_recursion_context(void)        { return -1; }
>>>   static inline void perf_swevent_put_recursion_context(int
>>> rctx)        { }
>>>   static inline void perf_event_enable(struct perf_event
>>> *event)        { }
>>>   static inline void perf_event_disable(struct perf_event
>>> *event)        { }
>>> +static inline int __perf_event_disable(void *info)            { }
>>>   static inline void perf_event_task_tick(void)                { }
>>>   #endif
>>>
>>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>>> index d7d71d6..0ad0fc9 100644
>>> --- a/kernel/events/core.c
>>> +++ b/kernel/events/core.c
>>> @@ -1253,7 +1253,7 @@ retry:
>>>   /*
>>>    * Cross CPU call to disable a performance event
>>>    */
>>> -static int __perf_event_disable(void *info)
>>> +int __perf_event_disable(void *info)
>>>   {
>>>       struct perf_event *event = info;
>>>       struct perf_event_context *ctx = event->ctx;
>>> diff --git a/kernel/events/hw_breakpoint.c
>>> b/kernel/events/hw_breakpoint.c
>>> index bb38c4d..483f14a 100644
>>> --- a/kernel/events/hw_breakpoint.c
>>> +++ b/kernel/events/hw_breakpoint.c
>>> @@ -453,7 +453,15 @@ int modify_user_hw_breakpoint(struct perf_event
>>> *bp, struct perf_event_attr *att
>>>       int old_type = bp->attr.bp_type;
>>>       int err = 0;
>>>
>>> -    perf_event_disable(bp);
>>> +    /*
>>> +     * modify_user_hw_breakpoint can be invoked with IRQs disabled
>>> and hence it
>>> +     * will not be possible to raise IPIs that invoke
>>> __perf_event_disable.
>>> +     * So call the function directly.
>>> +     */
>>> +    if (irqs_disabled())
>>> +        __perf_event_disable(bp);
>>> +    else
>>> +        perf_event_disable(bp);
>>
>> This only works if we are sure the bp is on the current CPU. Do we
>> have that guarantee?
>
> Yes. This is being hit during bp exception processing and is specific to
> ppc where we disable interrupts:
> hw_breakpoint_handler->perf_bp_event->ptrace_triggered->modify_user_hw_breakpoint()

Frederick,
Is this acceptable, or do you have other scenarios where this won't 
work? I can add a check to ensure we call __perf_event_disable only if 
the task is on the current CPU, but the above scenario is the only one 
where we're seeing this issue.

Thanks,
Naveen


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 RESEND] Hardware breakpoints: Invoke __perf_event_disable() if interrupts are already disabled
  2012-07-25 11:32                 ` Naveen N. Rao
@ 2012-07-31 13:41                   ` Frederic Weisbecker
  2012-08-02  8:16                     ` [PATCH v3] " Naveen N. Rao
  0 siblings, 1 reply; 17+ messages in thread
From: Frederic Weisbecker @ 2012-07-31 13:41 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: paulus, a.p.zijlstra, linux-kernel, mingo, emachado, acme,
	prasad.krishnan

On Wed, Jul 25, 2012 at 05:02:56PM +0530, Naveen N. Rao wrote:
> >>>@@ -453,7 +453,15 @@ int modify_user_hw_breakpoint(struct perf_event
> >>>*bp, struct perf_event_attr *att
> >>>      int old_type = bp->attr.bp_type;
> >>>      int err = 0;
> >>>
> >>>-    perf_event_disable(bp);
> >>>+    /*
> >>>+     * modify_user_hw_breakpoint can be invoked with IRQs disabled
> >>>and hence it
> >>>+     * will not be possible to raise IPIs that invoke
> >>>__perf_event_disable.
> >>>+     * So call the function directly.
> >>>+     */
> >>>+    if (irqs_disabled())
> >>>+        __perf_event_disable(bp);
> >>>+    else
> >>>+        perf_event_disable(bp);
> >>
> >>This only works if we are sure the bp is on the current CPU. Do we
> >>have that guarantee?
> >
> >Yes. This is being hit during bp exception processing and is specific to
> >ppc where we disable interrupts:
> >hw_breakpoint_handler->perf_bp_event->ptrace_triggered->modify_user_hw_breakpoint()
> 
> Frederick,
> Is this acceptable, or do you have other scenarios where this won't
> work? I can add a check to ensure we call __perf_event_disable only
> if the task is on the current CPU, but the above scenario is the
> only one where we're seeing this issue.

Yeah, please make sure that the targeted task is "current".

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3] Hardware breakpoints: Invoke __perf_event_disable() if interrupts are already disabled
  2012-07-31 13:41                   ` Frederic Weisbecker
@ 2012-08-02  8:16                     ` Naveen N. Rao
  2012-08-15 17:37                       ` Naveen N. Rao
  2012-09-04 18:53                       ` [tip:perf/urgent] perf/hwpb: " tip-bot for K.Prasad
  0 siblings, 2 replies; 17+ messages in thread
From: Naveen N. Rao @ 2012-08-02  8:16 UTC (permalink / raw)
  To: fweisbec
  Cc: a.p.zijlstra, prasad.krishnan, linux-kernel, mingo, paulus, acme,
	emachado

Hi Frederick,
I've added a check to make sure we are targeting the current task. This
applies on top of v3.5. Kindly review.

Thanks,
Naveen

History:
v3: Added check to make sure we only target "current" task.
v2: Use __perf_event_disable() directly
______

From: K.Prasad <Prasad.Krishnan@gmail.com>

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 calls __perf_event_disable() when interrupts are already
disabled, instead of perf_event_disable().

Reported-by: Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com>
Signed-off-by: K.Prasad <Prasad.Krishnan@gmail.com>
[naveen.n.rao@linux.vnet.ibm.com: v3: Check to make sure we target current task]
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 include/linux/perf_event.h    |    2 ++
 kernel/events/core.c          |    2 +-
 kernel/events/hw_breakpoint.c |   11 ++++++++++-
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 45db49f..c289ba0 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1292,6 +1292,7 @@ extern int perf_swevent_get_recursion_context(void);
 extern void perf_swevent_put_recursion_context(int rctx);
 extern void perf_event_enable(struct perf_event *event);
 extern void perf_event_disable(struct perf_event *event);
+extern int __perf_event_disable(void *info);
 extern void perf_event_task_tick(void);
 #else
 static inline void
@@ -1330,6 +1331,7 @@ static inline int  perf_swevent_get_recursion_context(void)		{ return -1; }
 static inline void perf_swevent_put_recursion_context(int rctx)		{ }
 static inline void perf_event_enable(struct perf_event *event)		{ }
 static inline void perf_event_disable(struct perf_event *event)		{ }
+static inline int __perf_event_disable(void *info)			{ }
 static inline void perf_event_task_tick(void)				{ }
 #endif
 
diff --git a/kernel/events/core.c b/kernel/events/core.c
index d7d71d6..0ad0fc9 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1253,7 +1253,7 @@ retry:
 /*
  * Cross CPU call to disable a performance event
  */
-static int __perf_event_disable(void *info)
+int __perf_event_disable(void *info)
 {
 	struct perf_event *event = info;
 	struct perf_event_context *ctx = event->ctx;
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index bb38c4d..9a7b487 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -453,7 +453,16 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
 	int old_type = bp->attr.bp_type;
 	int err = 0;
 
-	perf_event_disable(bp);
+	/*
+	 * modify_user_hw_breakpoint can be invoked with IRQs disabled and hence it
+	 * will not be possible to raise IPIs that invoke __perf_event_disable.
+	 * So call the function directly after making sure we are targeting the
+	 * current task.
+	 */
+	if (irqs_disabled() && bp->ctx && bp->ctx->task == current)
+		__perf_event_disable(bp);
+	else
+		perf_event_disable(bp);
 
 	bp->attr.bp_addr = attr->bp_addr;
 	bp->attr.bp_type = attr->bp_type;


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] Hardware breakpoints: Invoke __perf_event_disable() if interrupts are already disabled
  2012-08-02  8:16                     ` [PATCH v3] " Naveen N. Rao
@ 2012-08-15 17:37                       ` Naveen N. Rao
  2012-08-15 18:42                         ` Frederic Weisbecker
  2012-09-04 18:53                       ` [tip:perf/urgent] perf/hwpb: " tip-bot for K.Prasad
  1 sibling, 1 reply; 17+ messages in thread
From: Naveen N. Rao @ 2012-08-15 17:37 UTC (permalink / raw)
  To: fweisbec
  Cc: a.p.zijlstra, prasad.krishnan, linux-kernel, mingo, paulus, acme,
	emachado

Hi Frederick,
Did you get a chance to take a look at this?

Regards,
Naveen

On 08/02/2012 01:46 PM, Naveen N. Rao wrote:
> Hi Frederick,
> I've added a check to make sure we are targeting the current task. This
> applies on top of v3.5. Kindly review.
>
> Thanks,
> Naveen
>
> History:
> v3: Added check to make sure we only target "current" task.
> v2: Use __perf_event_disable() directly
> ______
>
> From: K.Prasad <Prasad.Krishnan@gmail.com>
>
> 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 calls __perf_event_disable() when interrupts are already
> disabled, instead of perf_event_disable().
>
> Reported-by: Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com>
> Signed-off-by: K.Prasad <Prasad.Krishnan@gmail.com>
> [naveen.n.rao@linux.vnet.ibm.com: v3: Check to make sure we target current task]
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>   include/linux/perf_event.h    |    2 ++
>   kernel/events/core.c          |    2 +-
>   kernel/events/hw_breakpoint.c |   11 ++++++++++-
>   3 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 45db49f..c289ba0 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -1292,6 +1292,7 @@ extern int perf_swevent_get_recursion_context(void);
>   extern void perf_swevent_put_recursion_context(int rctx);
>   extern void perf_event_enable(struct perf_event *event);
>   extern void perf_event_disable(struct perf_event *event);
> +extern int __perf_event_disable(void *info);
>   extern void perf_event_task_tick(void);
>   #else
>   static inline void
> @@ -1330,6 +1331,7 @@ static inline int  perf_swevent_get_recursion_context(void)		{ return -1; }
>   static inline void perf_swevent_put_recursion_context(int rctx)		{ }
>   static inline void perf_event_enable(struct perf_event *event)		{ }
>   static inline void perf_event_disable(struct perf_event *event)		{ }
> +static inline int __perf_event_disable(void *info)			{ }
>   static inline void perf_event_task_tick(void)				{ }
>   #endif
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index d7d71d6..0ad0fc9 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -1253,7 +1253,7 @@ retry:
>   /*
>    * Cross CPU call to disable a performance event
>    */
> -static int __perf_event_disable(void *info)
> +int __perf_event_disable(void *info)
>   {
>   	struct perf_event *event = info;
>   	struct perf_event_context *ctx = event->ctx;
> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> index bb38c4d..9a7b487 100644
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c
> @@ -453,7 +453,16 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
>   	int old_type = bp->attr.bp_type;
>   	int err = 0;
>
> -	perf_event_disable(bp);
> +	/*
> +	 * modify_user_hw_breakpoint can be invoked with IRQs disabled and hence it
> +	 * will not be possible to raise IPIs that invoke __perf_event_disable.
> +	 * So call the function directly after making sure we are targeting the
> +	 * current task.
> +	 */
> +	if (irqs_disabled() && bp->ctx && bp->ctx->task == current)
> +		__perf_event_disable(bp);
> +	else
> +		perf_event_disable(bp);
>
>   	bp->attr.bp_addr = attr->bp_addr;
>   	bp->attr.bp_type = attr->bp_type;
>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] Hardware breakpoints: Invoke __perf_event_disable() if interrupts are already disabled
  2012-08-15 17:37                       ` Naveen N. Rao
@ 2012-08-15 18:42                         ` Frederic Weisbecker
  2012-08-16  8:16                           ` Peter Zijlstra
  0 siblings, 1 reply; 17+ messages in thread
From: Frederic Weisbecker @ 2012-08-15 18:42 UTC (permalink / raw)
  To: Naveen N. Rao, Peter Zijlstra
  Cc: a.p.zijlstra, prasad.krishnan, linux-kernel, mingo, paulus, acme,
	emachado

On Wed, Aug 15, 2012 at 11:07:01PM +0530, Naveen N. Rao wrote:
> Hi Frederick,
> Did you get a chance to take a look at this?
> 
> Regards,
> Naveen

Yeah, I'm ok with the patch. Peter, are you ok with it?

Thanks.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] Hardware breakpoints: Invoke __perf_event_disable() if interrupts are already disabled
  2012-08-15 18:42                         ` Frederic Weisbecker
@ 2012-08-16  8:16                           ` Peter Zijlstra
  2012-08-29  2:45                             ` Naveen N. Rao
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Zijlstra @ 2012-08-16  8:16 UTC (permalink / raw)
  To: Frederic Weisbecker
  Cc: Naveen N. Rao, prasad.krishnan, linux-kernel, mingo, paulus,
	acme, emachado

On Wed, 2012-08-15 at 20:42 +0200, Frederic Weisbecker wrote:
> On Wed, Aug 15, 2012 at 11:07:01PM +0530, Naveen N. Rao wrote:
> > Hi Frederick,
> > Did you get a chance to take a look at this?
> > 
> > Regards,
> > Naveen
> 
> Yeah, I'm ok with the patch. Peter, are you ok with it?

Yeah, queued, it. sorry for loosing track etc..

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] Hardware breakpoints: Invoke __perf_event_disable() if interrupts are already disabled
  2012-08-16  8:16                           ` Peter Zijlstra
@ 2012-08-29  2:45                             ` Naveen N. Rao
  0 siblings, 0 replies; 17+ messages in thread
From: Naveen N. Rao @ 2012-08-29  2:45 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Frederic Weisbecker, prasad.krishnan, linux-kernel, mingo,
	paulus, acme, emachado

On 08/16/2012 01:46 PM, Peter Zijlstra wrote:
> On Wed, 2012-08-15 at 20:42 +0200, Frederic Weisbecker wrote:
>> On Wed, Aug 15, 2012 at 11:07:01PM +0530, Naveen N. Rao wrote:
>>> Hi Frederick,
>>> Did you get a chance to take a look at this?
>>>
>>> Regards,
>>> Naveen
>>
>> Yeah, I'm ok with the patch. Peter, are you ok with it?
>
> Yeah, queued, it. sorry for loosing track etc..
>

Hi Peter,
Can you please let me know if you plan to push this for v3.6 or if this 
has been queued for the next kernel release?

Thanks,
Naveen


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [tip:perf/urgent] perf/hwpb: Invoke __perf_event_disable() if interrupts are already disabled
  2012-08-02  8:16                     ` [PATCH v3] " Naveen N. Rao
  2012-08-15 17:37                       ` Naveen N. Rao
@ 2012-09-04 18:53                       ` tip-bot for K.Prasad
  1 sibling, 0 replies; 17+ messages in thread
From: tip-bot for K.Prasad @ 2012-09-04 18:53 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, hpa, mingo, a.p.zijlstra, Prasad.Krishnan,
	emachado, fweisbec, naveen.n.rao, tglx

Commit-ID:  500ad2d8b01390c98bc6dce068bccfa9534b8212
Gitweb:     http://git.kernel.org/tip/500ad2d8b01390c98bc6dce068bccfa9534b8212
Author:     K.Prasad <Prasad.Krishnan@gmail.com>
AuthorDate: Thu, 2 Aug 2012 13:46:35 +0530
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 4 Sep 2012 17:29:53 +0200

perf/hwpb: Invoke __perf_event_disable() if interrupts are already disabled

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 calls __perf_event_disable() when interrupts are already
disabled, instead of perf_event_disable().

Reported-by: Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com>
Signed-off-by: K.Prasad <Prasad.Krishnan@gmail.com>
[naveen.n.rao@linux.vnet.ibm.com: v3: Check to make sure we target current task]
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/20120802081635.5811.17737.stgit@localhost.localdomain
[ Fixed build error on MIPS. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/perf_event.h    |    2 ++
 kernel/events/core.c          |    2 +-
 kernel/events/hw_breakpoint.c |   11 ++++++++++-
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index ad04dfc..33ed9d6 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1296,6 +1296,7 @@ extern int perf_swevent_get_recursion_context(void);
 extern void perf_swevent_put_recursion_context(int rctx);
 extern void perf_event_enable(struct perf_event *event);
 extern void perf_event_disable(struct perf_event *event);
+extern int __perf_event_disable(void *info);
 extern void perf_event_task_tick(void);
 #else
 static inline void
@@ -1334,6 +1335,7 @@ static inline int  perf_swevent_get_recursion_context(void)		{ return -1; }
 static inline void perf_swevent_put_recursion_context(int rctx)		{ }
 static inline void perf_event_enable(struct perf_event *event)		{ }
 static inline void perf_event_disable(struct perf_event *event)		{ }
+static inline int __perf_event_disable(void *info)			{ return -1; }
 static inline void perf_event_task_tick(void)				{ }
 #endif
 
diff --git a/kernel/events/core.c b/kernel/events/core.c
index efef428..7fee567 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1253,7 +1253,7 @@ retry:
 /*
  * Cross CPU call to disable a performance event
  */
-static int __perf_event_disable(void *info)
+int __perf_event_disable(void *info)
 {
 	struct perf_event *event = info;
 	struct perf_event_context *ctx = event->ctx;
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index bb38c4d..9a7b487 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -453,7 +453,16 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
 	int old_type = bp->attr.bp_type;
 	int err = 0;
 
-	perf_event_disable(bp);
+	/*
+	 * modify_user_hw_breakpoint can be invoked with IRQs disabled and hence it
+	 * will not be possible to raise IPIs that invoke __perf_event_disable.
+	 * So call the function directly after making sure we are targeting the
+	 * current task.
+	 */
+	if (irqs_disabled() && bp->ctx && bp->ctx->task == current)
+		__perf_event_disable(bp);
+	else
+		perf_event_disable(bp);
 
 	bp->attr.bp_addr = attr->bp_addr;
 	bp->attr.bp_type = attr->bp_type;

^ permalink raw reply related	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2012-09-04 18:53 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-06-11  6:02 [Patch][perf] Invoke __perf_event_disable without an IPI K.Prasad
2012-06-11 11:13 ` Peter Zijlstra
2012-06-12  6:06   ` K.Prasad
2012-06-12  9:12     ` Peter Zijlstra
2012-07-06  9:52       ` [PATCH v2] Hardware breakpoints: Invoke __perf_event_disable() if interrupts are already disabled Naveen N. Rao
2012-07-06 10:18         ` Naveen N. Rao
2012-07-18 10:30           ` [PATCH v2 RESEND] " Naveen N. Rao
2012-07-18 11:57             ` Frederic Weisbecker
2012-07-19 11:16               ` Naveen N. Rao
2012-07-25 11:32                 ` Naveen N. Rao
2012-07-31 13:41                   ` Frederic Weisbecker
2012-08-02  8:16                     ` [PATCH v3] " Naveen N. Rao
2012-08-15 17:37                       ` Naveen N. Rao
2012-08-15 18:42                         ` Frederic Weisbecker
2012-08-16  8:16                           ` Peter Zijlstra
2012-08-29  2:45                             ` Naveen N. Rao
2012-09-04 18:53                       ` [tip:perf/urgent] perf/hwpb: " tip-bot for K.Prasad

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.