[PATCH 1/2] perf: Fix the software events state check

[PATCH 1/2] perf: Fix the software events state check

[Frederic Weisbecker]

Fix the mistakenly inverted check of events state.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Stephane Eranian <eranian@google.com>
---
 kernel/perf_event.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index ed253aa..974e2e6 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -5122,7 +5122,7 @@ static int perf_exclude_event(struct perf_event *event,
 			      struct pt_regs *regs)
 {
 	if (event->hw.state & PERF_HES_STOPPED)
-		return 0;
+		return 1;
 
 	if (regs) {
 		if (event->attr.exclude_user && user_mode(regs))
-- 
1.7.3.2

[PATCH 2/2] perf: Handle stopped state with tracepoints

[Frederic Weisbecker]

We toggle the state from start and stop callbacks but actually
don't check it when the event triggers. Do it so that
these callbacks actually work.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Stephane Eranian <eranian@google.com>
---
 kernel/perf_event.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 974e2e6..533f715 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -5478,6 +5478,8 @@ static int perf_tp_event_match(struct perf_event *event,
 				struct perf_sample_data *data,
 				struct pt_regs *regs)
 {
+	if (event->hw.state & PERF_HES_STOPPED)
+		return 0;
 	/*
 	 * All tracepoints are from kernel-space.
 	 */
-- 
1.7.3.2

Re: [PATCH 1/2] perf: Fix the software events state check

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Fix the mistakenly inverted check of events state.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Stephane Eranian <eranian@google.com>
> ---
>  kernel/perf_event.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index ed253aa..974e2e6 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -5122,7 +5122,7 @@ static int perf_exclude_event(struct perf_event *event,
>  			      struct pt_regs *regs)
>  {
>  	if (event->hw.state & PERF_HES_STOPPED)
> -		return 0;
> +		return 1;

Just wondering, what was/is the practical effect of this bug, and how far back does 
it go (any need for a -stable tag)?

Thanks,

	Ingo

Re: [PATCH 1/2] perf: Fix the software events state check

[Frederic Weisbecker]

On Wed, Mar 09, 2011 at 10:28:18AM +0100, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > Fix the mistakenly inverted check of events state.
> > 
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Cc: Paul Mackerras <paulus@samba.org>
> > Cc: Stephane Eranian <eranian@google.com>
> > ---
> >  kernel/perf_event.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> > index ed253aa..974e2e6 100644
> > --- a/kernel/perf_event.c
> > +++ b/kernel/perf_event.c
> > @@ -5122,7 +5122,7 @@ static int perf_exclude_event(struct perf_event *event,
> >  			      struct pt_regs *regs)
> >  {
> >  	if (event->hw.state & PERF_HES_STOPPED)
> > -		return 0;
> > +		return 1;
> 
> Just wondering, what was/is the practical effect of this bug, and how far back does 
> it go (any need for a -stable tag)?

I wasn't sure about that so I haven't told about the impact in the
changelog.

But now that I put a deeper look into this:

It seems that ->stop() / ->start() are called from perf_adjust_period()
to update the hardware with the new settings of period_left. The events
are stopped to avoid any race with events triggering with a stale period_left
in the hardware level when the software one has been updated, I guess.

So it doesn't seem to fix any existing bug because for ->stop() and ->start()
are only useful for hardware events right now. But we may call ->stop() and
->start() for further purpose later. In fact that paves the way for the event
exclusion patchset I'm about to post.

So it should be .39 material. But a confirmation from Peter would be nice.

Re: [PATCH 1/2] perf: Fix the software events state check

[Peter Zijlstra]

On Wed, 2011-03-09 at 14:52 +0100, Frederic Weisbecker wrote:
> 
> It seems that ->stop() / ->start() are called from perf_adjust_period()
> to update the hardware with the new settings of period_left. The events
> are stopped to avoid any race with events triggering with a stale period_left
> in the hardware level when the software one has been updated, I guess.
> 
> So it doesn't seem to fix any existing bug because for ->stop() and ->start()
> are only useful for hardware events right now. But we may call ->stop() and
> ->start() for further purpose later. In fact that paves the way for the event
> exclusion patchset I'm about to post.
> 
> So it should be .39 material. But a confirmation from Peter would be nice. 

Yeah, that sounds about right, I've already queued these patches
for .39.

[tip:perf/urgent] perf: Fix the software events state check

[tip-bot for Frederic Weisbecker]

Commit-ID:  91b2f482e62ad0d444222253026a5cbca28c4ab9
Gitweb:     http://git.kernel.org/tip/91b2f482e62ad0d444222253026a5cbca28c4ab9
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Mon, 7 Mar 2011 21:27:08 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 16 Mar 2011 14:04:13 +0100

perf: Fix the software events state check

Fix the mistakenly inverted check of events state.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: <stable@kernel.org>
LKML-Reference: <1299529629-18280-1-git-send-email-fweisbec@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/perf_event.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index ed253aa..974e2e6 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -5122,7 +5122,7 @@ static int perf_exclude_event(struct perf_event *event,
 			      struct pt_regs *regs)
 {
 	if (event->hw.state & PERF_HES_STOPPED)
-		return 0;
+		return 1;
 
 	if (regs) {
 		if (event->attr.exclude_user && user_mode(regs))

[tip:perf/urgent] perf: Handle stopped state with tracepoints

[tip-bot for Frederic Weisbecker]

Commit-ID:  a0f7d0f7fc02465bb9758501f611f63381792996
Gitweb:     http://git.kernel.org/tip/a0f7d0f7fc02465bb9758501f611f63381792996
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Mon, 7 Mar 2011 21:27:09 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 16 Mar 2011 14:04:14 +0100

perf: Handle stopped state with tracepoints

We toggle the state from start and stop callbacks but actually
don't check it when the event triggers. Do it so that
these callbacks actually work.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: <stable@kernel.org>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <1299529629-18280-2-git-send-email-fweisbec@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/perf_event.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 974e2e6..533f715 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -5478,6 +5478,8 @@ static int perf_tp_event_match(struct perf_event *event,
 				struct perf_sample_data *data,
 				struct pt_regs *regs)
 {
+	if (event->hw.state & PERF_HES_STOPPED)
+		return 0;
 	/*
 	 * All tracepoints are from kernel-space.
 	 */