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

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Dec 17, 2024 at 12:45:56PM -0500, Liang, Kan wrote:


> > Why can't you use something like the below -- that gives you a count
> > value matching the pmc value you put in, as long as it is 'near' the
> > current value.
> > 
> > ---
> > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> > index 8f218ac0d445..3cf8b4f2b2c1 100644
> > --- a/arch/x86/events/core.c
> > +++ b/arch/x86/events/core.c
> > @@ -154,6 +154,26 @@ u64 x86_perf_event_update(struct perf_event *event)
> >  	return new_raw_count;
> >  }
> >  
> > +u64 x86_perf_event_pmc_to_count(struct perf_event *event, u64 pmc)
> > +{
> > +	struct hw_perf_event *hwc = &event->hw;
> > +	int shift = 64 - x86_pmu.cntval_bits;
> > +	u64 prev_pmc, prev_count;
> > +	u64 delta;
> > +
> > +	do {
> > +		prev_pmc = local64_read(&hwc->prev_count);
> > +		barrier();
> > +		prev_count = local64_read(&event->count);
> > +		barrier();
> > +	} while (prev_pmc != local64_read(&hwc->prev_count));
> 
> Is the "while()" to handle PMI? But there should be no PMI, since the
> PMU has been disabled when draining the PEBS buffer.

Perhaps not in your case, but this way the function is more widely
usable.

> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index e06ac9a3cdf8..7f0b850f7277 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -1969,6 +1969,23 @@ static void adaptive_pebs_save_regs(struct
> pt_regs *regs,
> 
>  #define PEBS_LATENCY_MASK			0xffff
> 
> +void intel_perf_event_pmc_to_count(struct perf_event *event, u64 pmc)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	int shift = 64 - x86_pmu.cntval_bits;
> +	u64 prev_pmc;
> +	u64 delta;
> +
> +	prev_pmc = local64_read(&hwc->prev_count);
> +
> +	delta = (pmc << shift) - (prev_pmc << shift);
> +	delta >>= shift;
> +
> +	local64_add(delta, &event->count);
> +	local64_sub(delta, &hwc->period_left);
> +	local64_set(&hwc->prev_count, pmc);
> +}

This seems very fragile, at least keep the same store order and assert
you're in NMI/PMI context.