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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 29 Mar 2011 17:03:57 +1100
From:	Benjamin Herrenschmidt <benh@...nel.crashing.org>
To:	Eric B Munson <emunson@...bm.net>
Cc:	a.p.zijlstra@...llo.nl, paulus@...ba.org, mingo@...e.hu,
	acme@...stprotocols.net, linuxppc-dev@...ts.ozlabs.org,
	linux-kernel@...r.kernel.org, anton@...ba.org
Subject: Re: [PATCH] POWER: perf_event: Skip updating kernel counters if
 register value shrinks

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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/