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