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

From: Eric B Munson
Date: Tue Mar 29 2011 - 10:25:27 EST


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