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

[Eric B Munson]

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@xxxxxxxxx>
> > ---
> >  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: signature.asc
Description: Digital signature