linux-kernel - Re: [PATCH V5 4/4] perf/x86/intel: Support PEBS counters snapshotting

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: <20241218082404.GI11133@noisy.programming.kicks-ass.net>
Date: Wed, 18 Dec 2024 09:24:04 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: "Liang, Kan" <kan.liang@...ux.intel.com>
Cc: mingo@...hat.com, acme@...nel.org, namhyung@...nel.org,
	irogers@...gle.com, linux-kernel@...r.kernel.org,
	linux-perf-users@...r.kernel.org, ak@...ux.intel.com,
	eranian@...gle.com
Subject: Re: [PATCH V5 4/4] perf/x86/intel: Support PEBS counters snapshotting

On Tue, Dec 17, 2024 at 12:45:56PM -0500, Liang, Kan wrote:
> 
> 
> On 2024-12-17 8:37 a.m., Peter Zijlstra wrote:
> > On Mon, Dec 16, 2024 at 12:45:05PM -0800, kan.liang@...ux.intel.com wrote:
> > 
> >> @@ -2049,6 +2102,40 @@ static void setup_pebs_adaptive_sample_data(struct perf_event *event,
> >>  		}
> >>  	}
> >>  
> >> +	if (format_group & (PEBS_DATACFG_CNTR | PEBS_DATACFG_METRICS)) {
> >> +		struct pebs_cntr_header *cntr = next_record;
> >> +		int bit;
> >> +
> >> +		next_record += sizeof(struct pebs_cntr_header);
> >> +
> >> +		for_each_set_bit(bit, (unsigned long *)&cntr->cntr, INTEL_PMC_MAX_GENERIC) {
> >> +			x86_perf_event_update(cpuc->events[bit], (u64 *)next_record);
> >> +			next_record += sizeof(u64);
> >> +		}
> > 
> > I still don't much like any of this -- the next_record value might be in
> > the past relative to what is already in the event.
> >
> 
> I forgot to handle the read case. With the below patch, the next_record
> value should not contain a previous value, unless there is a HW bug.
> Because no matter it's read/pmi/context switch, perf always stop the
> PMU, drains the buffer, and the value is always from the PEBS record.

Consider this 3 counter case:

 A is our regular counter
 B is a PEBS event which reads A
 C is a PEBS event

For convencience, let A count the lines in our example (anything
incrementing at a faster rate will get the same result after all).

Then, afaict, the following can happen:

  B-assist			A=1
  C				A=2
  B-assist			A=3
  A-overflow-PMI		A=4
  C-assist-PMI (DS buffer)	A=5

So the A-PMI will update A->count to 4.
Then the C-PMI will process the DS buffer, which will:

  - adjust A->count to 1
  - adjust A->count to 3

Leaving us, in the end, with A going backwards.

How is that not a problem?

IIRC I've raised this point before, and your Changelog does not mention
anything about this issue at all :-(