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]

Message-ID: <20110329142519.GA3527@mgebm.net>
Date:	Tue, 29 Mar 2011 10:25:19 -0400
From:	Eric B Munson <emunson@...bm.net>
To:	Benjamin Herrenschmidt <benh@...nel.crashing.org>
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 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

Download attachment "signature.asc" of type "application/pgp-signature" (491 bytes)