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

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

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 ?

Cheers,
Ben.