[PATCH 1/3] events: Add note to update_event_times comment about holding ctx->lock

[PATCH 1/3] events: Add note to update_event_times comment about holding ctx->lock

[Eric B Munson]

Signed-off-by: Eric B Munson <emunson@mgebm.net>
---
 kernel/events/core.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 9efe710..a054964 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -731,6 +731,7 @@ static u64 perf_event_time(struct perf_event *event)
 
 /*
  * Update the total_time_enabled and total_time_running fields for a event.
+ * The caller of this function needs to hold the ctx->lock.
  */
 static void update_event_times(struct perf_event *event)
 {
-- 
1.7.4.1

[PATCH 2/3] events: Move lockless timer calculation into helper function

[Eric B Munson]

Take the timer calculation from perf_output_read and move it to a helper
function for any place that needs timer values but cannot take the ctx->lock.

Signed-off-by: Eric B Munson <emunson@mgebm.net>
---
 kernel/events/core.c |   22 +++++++++++++++-------
 1 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index a054964..9e9a7fa 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3359,6 +3359,18 @@ static int perf_event_index(struct perf_event *event)
 	return event->hw.idx + 1 - PERF_EVENT_INDEX_OFFSET;
 }
 
+static void calc_timer_values(struct perf_event *event,
+				u64 *running,
+				u64 *enabled)
+{
+	u64 now, ctx_time;
+
+	now = perf_clock();
+	ctx_time = event->shadow_ctx_time + now;
+	*enabled = ctx_time - event->tstamp_enabled;
+	*running = ctx_time - event->tstamp_running;
+}
+
 /*
  * Callers need to ensure there can be no nesting of this function, otherwise
  * the seqlock logic goes bad. We can not serialize this because the arch
@@ -4250,7 +4262,7 @@ static void perf_output_read_group(struct perf_output_handle *handle,
 static void perf_output_read(struct perf_output_handle *handle,
 			     struct perf_event *event)
 {
-	u64 enabled = 0, running = 0, now, ctx_time;
+	u64 enabled = 0, running = 0;
 	u64 read_format = event->attr.read_format;
 
 	/*
@@ -4262,12 +4274,8 @@ static void perf_output_read(struct perf_output_handle *handle,
 	 * because of locking issue as we are called in
 	 * NMI context
 	 */
-	if (read_format & PERF_FORMAT_TOTAL_TIMES) {
-		now = perf_clock();
-		ctx_time = event->shadow_ctx_time + now;
-		enabled = ctx_time - event->tstamp_enabled;
-		running = ctx_time - event->tstamp_running;
-	}
+	if (read_format & PERF_FORMAT_TOTAL_TIMES)
+		calc_timer_values(event, &enabled, &running);
 
 	if (event->attr.read_format & PERF_FORMAT_GROUP)
 		perf_output_read_group(handle, event, enabled, running);
-- 
1.7.4.1

[tip:perf/core] events: Move lockless timer calculation into helper function

[tip-bot for Eric B Munson]

Commit-ID:  c4794295917ebeda8013b6cb9c8d71ab4f74a1fa
Gitweb:     http://git.kernel.org/tip/c4794295917ebeda8013b6cb9c8d71ab4f74a1fa
Author:     Eric B Munson <emunson@mgebm.net>
AuthorDate: Thu, 23 Jun 2011 16:34:38 -0400
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 1 Jul 2011 11:06:33 +0200

events: Move lockless timer calculation into helper function

Take the timer calculation from perf_output_read and move it to a helper
function for any place that needs timer values but cannot take the ctx->lock.

Signed-off-by: Eric B Munson <emunson@mgebm.net>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1308861279-15216-2-git-send-email-emunson@mgebm.net
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/events/core.c |   22 +++++++++++++++-------
 1 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 2293e0b..c851d70 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3351,6 +3351,18 @@ static int perf_event_index(struct perf_event *event)
 	return event->hw.idx + 1 - PERF_EVENT_INDEX_OFFSET;
 }
 
+static void calc_timer_values(struct perf_event *event,
+				u64 *running,
+				u64 *enabled)
+{
+	u64 now, ctx_time;
+
+	now = perf_clock();
+	ctx_time = event->shadow_ctx_time + now;
+	*enabled = ctx_time - event->tstamp_enabled;
+	*running = ctx_time - event->tstamp_running;
+}
+
 /*
  * Callers need to ensure there can be no nesting of this function, otherwise
  * the seqlock logic goes bad. We can not serialize this because the arch
@@ -3816,7 +3828,7 @@ static void perf_output_read_group(struct perf_output_handle *handle,
 static void perf_output_read(struct perf_output_handle *handle,
 			     struct perf_event *event)
 {
-	u64 enabled = 0, running = 0, now, ctx_time;
+	u64 enabled = 0, running = 0;
 	u64 read_format = event->attr.read_format;
 
 	/*
@@ -3828,12 +3840,8 @@ static void perf_output_read(struct perf_output_handle *handle,
 	 * because of locking issue as we are called in
 	 * NMI context
 	 */
-	if (read_format & PERF_FORMAT_TOTAL_TIMES) {
-		now = perf_clock();
-		ctx_time = event->shadow_ctx_time + now;
-		enabled = ctx_time - event->tstamp_enabled;
-		running = ctx_time - event->tstamp_running;
-	}
+	if (read_format & PERF_FORMAT_TOTAL_TIMES)
+		calc_timer_values(event, &enabled, &running);
 
 	if (event->attr.read_format & PERF_FORMAT_GROUP)
 		perf_output_read_group(handle, event, enabled, running);

[PATCH 3/3] events: Ensure that timers are updated without requiring read() call

[Eric B Munson]

The event tracing infrastructure exposes two timers which should be updated
each time the value of the counter is updated.  Currently, these counters are
only updated when userspace calls read() on the fd associated with an event.
This means that counters which are read via the mmap'd page exclusively never
have their timers updated.  This patch adds ensures that the timers are updated
each time the values in the mmap'd page are updated.

Signed-off-by: Eric B Munson <emunson@mgebm.net>
---
 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 9e9a7fa..e3be175 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3382,6 +3382,18 @@ void perf_event_update_userpage(struct perf_event *event)
 	struct perf_buffer *buffer;
 
 	rcu_read_lock();
+	/*
+	 * compute total_time_enabled, total_time_running
+	 * based on snapshot values taken when the event
+	 * was last scheduled in.
+	 *
+	 * we cannot simply called update_context_time()
+	 * because of locking issue as we are called in
+	 * NMI context
+	 */
+	calc_timer_values(event,
+				&event->total_time_enabled,
+				&event->total_time_running);
 	buffer = rcu_dereference(event->buffer);
 	if (!buffer)
 		goto unlock;
-- 
1.7.4.1

Re: [PATCH 3/3] events: Ensure that timers are updated without requiring read() call

[Peter Zijlstra]

On Thu, 2011-06-23 at 16:34 -0400, Eric B Munson wrote:
> The event tracing infrastructure exposes two timers which should be updated
> each time the value of the counter is updated.  Currently, these counters are
> only updated when userspace calls read() on the fd associated with an event.
> This means that counters which are read via the mmap'd page exclusively never
> have their timers updated.  This patch adds ensures that the timers are updated
> each time the values in the mmap'd page are updated.
> 
> Signed-off-by: Eric B Munson <emunson@mgebm.net>
> ---
>  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 9e9a7fa..e3be175 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -3382,6 +3382,18 @@ void perf_event_update_userpage(struct perf_event *event)
>  	struct perf_buffer *buffer;
>  
>  	rcu_read_lock();
> +	/*
> +	 * compute total_time_enabled, total_time_running
> +	 * based on snapshot values taken when the event
> +	 * was last scheduled in.
> +	 *
> +	 * we cannot simply called update_context_time()
> +	 * because of locking issue as we are called in

s/are/can be/

> +	 * NMI context
> +	 */
> +	calc_timer_values(event,
> +				&event->total_time_enabled,
> +				&event->total_time_running);

I'm not sure writing those from NMI context is a sane thing to do, best
is to compute the values into a local variable and use that variable
below.

Took the first two patches.

>  	buffer = rcu_dereference(event->buffer);
>  	if (!buffer)
>  		goto unlock;

Re: [PATCH 3/3] events: Ensure that timers are updated without requiring read() call

[Peter Zijlstra]

On Fri, 2011-06-24 at 11:44 +0200, Peter Zijlstra wrote:
> > +     calc_timer_values(event,
> > +                             &event->total_time_enabled,
> > +                             &event->total_time_running);
> 
> I'm not sure writing those from NMI context is a sane thing to do, best
> is to compute the values into a local variable and use that variable
> below. 

To clarify, on 32bit architectures the NMI might come in the middle of
writing the two words of one of those, writing them again from the NMI
handler will result in overlapping writes, which might lead to some
weird end-results.

Re: [PATCH 3/3] events: Ensure that timers are updated without requiring read() call

[Eric B Munson]

[-- Attachment #1: Type: text/plain, Size: 1786 bytes --]

On Fri, 24 Jun 2011, Peter Zijlstra wrote:

> On Thu, 2011-06-23 at 16:34 -0400, Eric B Munson wrote:
> > The event tracing infrastructure exposes two timers which should be updated
> > each time the value of the counter is updated.  Currently, these counters are
> > only updated when userspace calls read() on the fd associated with an event.
> > This means that counters which are read via the mmap'd page exclusively never
> > have their timers updated.  This patch adds ensures that the timers are updated
> > each time the values in the mmap'd page are updated.
> > 
> > Signed-off-by: Eric B Munson <emunson@mgebm.net>
> > ---
> >  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 9e9a7fa..e3be175 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -3382,6 +3382,18 @@ void perf_event_update_userpage(struct perf_event *event)
> >  	struct perf_buffer *buffer;
> >  
> >  	rcu_read_lock();
> > +	/*
> > +	 * compute total_time_enabled, total_time_running
> > +	 * based on snapshot values taken when the event
> > +	 * was last scheduled in.
> > +	 *
> > +	 * we cannot simply called update_context_time()
> > +	 * because of locking issue as we are called in
> 
> s/are/can be/
> 
> > +	 * NMI context
> > +	 */
> > +	calc_timer_values(event,
> > +				&event->total_time_enabled,
> > +				&event->total_time_running);
> 
> I'm not sure writing those from NMI context is a sane thing to do, best
> is to compute the values into a local variable and use that variable
> below.
> 
> Took the first two patches.
> 

Thanks, I will send out an updated patch once I get it tested.

Eric

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH 3/3] events: Ensure that timers are updated without requiring read() call

[Eric B Munson]

[-- Attachment #1: Type: text/plain, Size: 2083 bytes --]

On Fri, 24 Jun 2011, Peter Zijlstra wrote:

> On Thu, 2011-06-23 at 16:34 -0400, Eric B Munson wrote:
> > The event tracing infrastructure exposes two timers which should be updated
> > each time the value of the counter is updated.  Currently, these counters are
> > only updated when userspace calls read() on the fd associated with an event.
> > This means that counters which are read via the mmap'd page exclusively never
> > have their timers updated.  This patch adds ensures that the timers are updated
> > each time the values in the mmap'd page are updated.
> > 
> > Signed-off-by: Eric B Munson <emunson@mgebm.net>
> > ---
> >  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 9e9a7fa..e3be175 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -3382,6 +3382,18 @@ void perf_event_update_userpage(struct perf_event *event)
> >  	struct perf_buffer *buffer;
> >  
> >  	rcu_read_lock();
> > +	/*
> > +	 * compute total_time_enabled, total_time_running
> > +	 * based on snapshot values taken when the event
> > +	 * was last scheduled in.
> > +	 *
> > +	 * we cannot simply called update_context_time()
> > +	 * because of locking issue as we are called in
> 
> s/are/can be/
> 
> > +	 * NMI context
> > +	 */
> > +	calc_timer_values(event,
> > +				&event->total_time_enabled,
> > +				&event->total_time_running);
> 
> I'm not sure writing those from NMI context is a sane thing to do, best
> is to compute the values into a local variable and use that variable
> below.
> 
> Took the first two patches.
> 

Now that I think about it, this will just mask the problem.  I have a test
program uses the mmap'd user space page to access event counters (it never
calls read()).  In this case, the timer values in the event will never be
updated.  It will display "properly" but the structure won't ever be correct.
Given that, how can we keep the event values current?

Eric

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH 3/3] events: Ensure that timers are updated without requiring read() call

[Peter Zijlstra]

On Fri, 2011-06-24 at 08:49 -0400, Eric B Munson wrote:

> Now that I think about it, this will just mask the problem.  I have a test
> program uses the mmap'd user space page to access event counters (it never
> calls read()).  In this case, the timer values in the event will never be
> updated.  It will display "properly" but the structure won't ever be correct.
> Given that, how can we keep the event values current?

Well the idea is that you do a userspace read of the counter, on x86
that would be using rdpmc and use the provided event count as base. See
the comment in struct perf_event_mmap_page.

Currently we don't have rdpmc support for x86, but it shouldn't be hard.
We should poke at CR4 in our CPU_STARTING callback, and fix up the mess
called perf_event_index() to deal with the strange and wonderful
encoding rdpmc needs (might want an event->pmu->index callback or so).

Currently all this only works on PowerPC.

[tip:perf/core] events: Add note to update_event_times comment about holding ctx->lock

[tip-bot for Eric B Munson]

Commit-ID:  b7526f0ca6dc68f57ca467ce503151b1d476a3e4
Gitweb:     http://git.kernel.org/tip/b7526f0ca6dc68f57ca467ce503151b1d476a3e4
Author:     Eric B Munson <emunson@mgebm.net>
AuthorDate: Thu, 23 Jun 2011 16:34:37 -0400
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 1 Jul 2011 11:06:33 +0200

events: Add note to update_event_times comment about holding ctx->lock

Signed-off-by: Eric B Munson <emunson@mgebm.net>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1308861279-15216-1-git-send-email-emunson@mgebm.net
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/events/core.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index e4aee51..2293e0b 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -748,6 +748,7 @@ static u64 perf_event_time(struct perf_event *event)
 
 /*
  * Update the total_time_enabled and total_time_running fields for a event.
+ * The caller of this function needs to hold the ctx->lock.
  */
 static void update_event_times(struct perf_event *event)
 {