Re: [PATCH 4/9] perf/x86/intel: Support hardware TopDown metrics

[Peter Zijlstra]

On Tue, May 28, 2019 at 02:24:14PM -0400, Liang, Kan wrote:

> > > +		if (!(txn_flags & PERF_PMU_TXN_REMOVE)) {
> > > +			/* Reset the metric value when reading
> > > +			 * The SLOTS register must be reset when PERF_METRICS reset,
> > > +			 * otherwise PERF_METRICS may has wrong output.
> > > +			 */
> > 
> > broken comment style.. (and grammer)
> 
> Missed a full stop.
> Should be "Reset the metric value for each read."

s/may has wrong/may have wrong/

> > > +			wrmsrl(MSR_PERF_METRICS, 0);
> > > +			wrmsrl(MSR_CORE_PERF_FIXED_CTR3, 0);
> > 
> > I don't get this, overflow happens on when we flip sign, so why is
> > programming 0 a sane thing to do?
> 
> Reset the counters (programming 0) don't trigger overflow.

Right, so why then do you allow creating this thing as
is_sampling_event() ?

> We have to reset both registers for each read to avoid the known
> PERF_METRICS issue.

'the known PERF_METRICS issue' is unknown to me and any other reader.

> > > +	metric = (cpuc->last_metric >> ((hwc->idx - INTEL_PMC_IDX_FIXED_METRIC_BASE)*8)) & 0xff;
> > > +	last = (((metric * 0xffff) >> 8) * cpuc->last_slots) >> 16;
> > 
> > How is that cpuc->last_* crap not broken for NMIs ?
> 
> There should be no NMI for slots or metric events at the moment, because the
> MSR_PERF_METRICS and MSR_CORE_PERF_FIXED_CTR3 are reset in first read.
> Other NMIs will not touch the codes here.

What happens if someone does: read(perf_fd) and then has the NMI hit?