Messages in this thread | | | Date | Tue, 29 Mar 2011 10:25:19 -0400 | From | Eric B Munson <> | Subject | Re: [PATCH] POWER: perf_event: Skip updating kernel counters if register value shrinks |
| |
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 [unhandled content-type:application/pgp-signature] | |