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

[PATCH] 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 |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 9efe710..3dd4ebe 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3369,6 +3369,7 @@ void perf_event_update_userpage(struct perf_event *event)
 	struct perf_buffer *buffer;
 
 	rcu_read_lock();
+	update_event_times(event);
 	buffer = rcu_dereference(event->buffer);
 	if (!buffer)
 		goto unlock;
-- 
1.7.4.1

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

[Peter Zijlstra]

On Thu, 2011-06-23 at 10:15 -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 |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 9efe710..3dd4ebe 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -3369,6 +3369,7 @@ void perf_event_update_userpage(struct perf_event *event)
>  	struct perf_buffer *buffer;
>  
>  	rcu_read_lock();
> +	update_event_times(event);
>  	buffer = rcu_dereference(event->buffer);
>  	if (!buffer)
>  		goto unlock;

Not that easy, perf_event_update_userpage() can happen from NMI context,
and update_event_times() needs to be done under ctx->lock.

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

[Eric B Munson]

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

On Thu, 23 Jun 2011, Peter Zijlstra wrote:

> On Thu, 2011-06-23 at 10:15 -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 |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 9efe710..3dd4ebe 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -3369,6 +3369,7 @@ void perf_event_update_userpage(struct perf_event *event)
> >  	struct perf_buffer *buffer;
> >  
> >  	rcu_read_lock();
> > +	update_event_times(event);
> >  	buffer = rcu_dereference(event->buffer);
> >  	if (!buffer)
> >  		goto unlock;
> 
> Not that easy, perf_event_update_userpage() can happen from NMI context,
> and update_event_times() needs to be done under ctx->lock.
> 

I was afraid that might be the case, is there an easy-ish way to accomplish
the timer update?

Eric

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

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

[Peter Zijlstra]

On Thu, 2011-06-23 at 10:38 -0400, Eric B Munson wrote:
> On Thu, 23 Jun 2011, Peter Zijlstra wrote:
> 
> > On Thu, 2011-06-23 at 10:15 -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 |    1 +
> > >  1 files changed, 1 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > > index 9efe710..3dd4ebe 100644
> > > --- a/kernel/events/core.c
> > > +++ b/kernel/events/core.c
> > > @@ -3369,6 +3369,7 @@ void perf_event_update_userpage(struct perf_event *event)
> > >  	struct perf_buffer *buffer;
> > >  
> > >  	rcu_read_lock();
> > > +	update_event_times(event);
> > >  	buffer = rcu_dereference(event->buffer);
> > >  	if (!buffer)
> > >  		goto unlock;
> > 
> > Not that easy, perf_event_update_userpage() can happen from NMI context,
> > and update_event_times() needs to be done under ctx->lock.
> > 
> 
> I was afraid that might be the case, is there an easy-ish way to accomplish
> the timer update?

IIRC Stephane did something along these lines, he kept a stamp and then
computed a delta in NMI context using perf_clock(). Let me try and find
where that lives.

See perf_output_read().