linux-kernel - Re: [PATCH V3 1/5] perf/x86/intel: fix event update for auto-reload

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: <20180209140905.GG25181@hirez.programming.kicks-ass.net>
Date:   Fri, 9 Feb 2018 15:09:05 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     "Liang, Kan" <kan.liang@...ux.intel.com>
Cc:     mingo@...hat.com, linux-kernel@...r.kernel.org, acme@...nel.org,
        tglx@...utronix.de, jolsa@...hat.com, eranian@...gle.com,
        ak@...ux.intel.com
Subject: Re: [PATCH V3 1/5] perf/x86/intel: fix event update for auto-reload

On Tue, Feb 06, 2018 at 12:58:23PM -0500, Liang, Kan wrote:
> 
> 
> > With the exception of handling 'empty' buffers, I ended up with the
> > below. Please try again.
> > 
> 
> There are two small errors. After fixing them, the patch works well.

Well, it still doesn't do A, two read()s without PEBS record in between.
So that needs fixing. What 3/5 does, call x86_perf_event_update() after
drain_pebs() is actively wrong after this patch.

> > +
> > +	/*
> > +	 * Careful, not all hw sign-extends above the physical width
> > +	 * of the counter.
> > +	 */
> > +	delta = (new_raw_count << shift) - (prev_raw_count << shift);
> > +	delta >>= shift;
> 
> new_raw_count could be smaller than prev_raw_count.
> The sign bit will be set. The delta>> could be wrong.
> 
> I think we can add a period here to prevent it.
> +       delta = (period << shift) + (new_raw_count << shift) -
> +               (prev_raw_count << shift);
> +       delta >>= shift;
> ......
> +       local64_add(delta + period * (count - 1), &event->count);
> 

Right it does, but that wrecks case A again, because then we get here
with !@...nt.

Maybe something like:


	s64 new, old;

	new = ((s64)(new_raw_count << shift) >> shift);
	old = ((s64)(old_raw_count << shift) >> shift);

	local64_add(new - old + count * period, &event->count);


And then make intel_pmu_drain_pebs_*(), call this function even when !n.