Re: [PATCH V9 3/3] perf/x86/intel: Support PEBS counters snapshotting

[Peter Zijlstra]

On Thu, Jan 16, 2025 at 04:50:01PM -0500, Liang, Kan wrote:
> 
> 
> On 2025-01-16 3:56 p.m., Peter Zijlstra wrote:
> > On Thu, Jan 16, 2025 at 09:42:25PM +0100, Peter Zijlstra wrote:
> >> On Thu, Jan 16, 2025 at 10:55:46AM -0500, Liang, Kan wrote:
> >>
> >>>> Also, I think I found you another bug... Consider what happens to the
> >>>> counter value when we reschedule a HES_STOPPED counter, then we skip
> >>>> x86_pmu_start(RELOAD) on step2, which leave the counter value with
> >>>> 'random' crap from whatever was there last.
> >>>>
> >>>> But meanwhile you do program PEBS to sample it. That will happily sample
> >>>> this garbage.
> >>>>
> >>>> Hmm?
> >>>
> >>> I'm not quite sure I understand the issue.
> >>>
> >>> The HES_STOPPED counter should be a pre-existing counter. Just for some
> >>> reason, it's stopped, right? So perf doesn't need to re-configure the
> >>> PEBS__DATA_CFG, since the idx is not changed.
> >>
> >> Suppose you have your group {A, B, C} and lets suppose A is the PEBS
> >> event, further suppose that B is also a sampling event. Lets say they
> >> get hardware counters 1,2 and 3 respectively.
> >>
> >> Then lets say B gets throttled.
> >>
> >> While it is throttled, we get a new event D scheduled, and D gets placed
> >> on counter 2 -- where B lives, which gets moved over to counter 4.
> >>
> >> Then our loops will update and remove B from 2, but because
> >> throttled/HES_STOPPED it will not start it on counter 4.
> >>>> Meanwhile, we do have the PEBS_DATA_CFG thing updated to sample counter
> >> 1,3 and 4.
> >>
> >> PEBS assist happens, and samples the uninitialized counter 4.
> > > Also, by skipping x86_pmu_start() we miss the assignment of
> > cpuc->events[] so PEBS buffer decode can't even find the dodgy event.
> > 
> 
> Yes, counter 4 includes garbage before the B is started again.
> But the cpuc->events[counter 4] is NULL either.
> 
> The current implementation ignores the NULL cpuc->events[]. The stopped
> B should not be mistakenly updated.

Ah, indeed. I was so close.

One question though -- is this value ever exposed otherwise? I had a
quick look and I don't think we support PERF_SAMPLE_RAW for PEBS, but
what about PEBS-to-PT ? 

Anywya, let me go find this v10 thing :-)