Re: [PATCH] POWER: perf_event: Skip updating kernel counters if register value shrinks

[Eric B Munson <emunson@mgebm.net>]

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

On Tue, 29 Mar 2011, Benjamin Herrenschmidt wrote:

> On Fri, 2011-03-25 at 09:28 -0400, Eric B Munson wrote:
> > It is possible on POWER7 for some perf events to have values decrease.  This
> > causes a problem with the way the kernel counters are updated.  Deltas are
> > computed and then stored in a 64 bit value while the registers are 32 bits
> > wide so if new value is smaller than previous value, the delta is a very
> > large positive value.  As a work around this patch skips updating the kernel
> > counter in when the new value is smaller than the previous.  This can lead to
> > a lack of precision in the coutner values, but from my testing the value is
> > typcially fewer than 10 samples at a time.
> 
> Unfortunately the patch isn't 100% correct I believe:
> 
> I think you don't deal with the rollover of the counters. The new value
> could be smaller than the previous one simply because the counter just
> rolled over.
> 
> In cases like this:
> 
> > @@ -449,8 +458,10 @@ static void freeze_limited_counters(struct cpu_hw_events *cpuhw,
> >  		val = (event->hw.idx == 5) ? pmc5 : pmc6;
> >  		prev = local64_read(&event->hw.prev_count);
> >  		event->hw.idx = 0;
> > -		delta = (val - prev) & 0xfffffffful;
> > -		local64_add(delta, &event->count);
> > +		if (val >= prev) {
> > +			delta = (val - prev) & 0xfffffffful;
> > +			local64_add(delta, &event->count);
> > +		}
> >  	}
> >  }
> 
> I wonder if it isn't easier to just define delta to be a s32, get rid
> of the mask and test if delta is positive, something like:
> 
> 		delta =  val - prev;
> 		if (delta > 0)
> 			local64_add(delta, &event->count);
> 
> Wouldn't that be simpler ? Or do I miss a reason why it wouldn't work ?

Here I made the assumption that the hardware would never remove more events in
a speculative roll back than it had added.  This is not a situation I
encoutered in my limited testing, so I didn't think underflow was possible.  I
will send out a V2 using the signed 32 bit delta and remeber to CC stable
this time.

Eric

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