Re: [PATCH 4/9] perf/x86/intel: Support hardware TopDown metrics
From: Peter Zijlstra
Date: Wed May 29 2019 - 03:37:57 EST
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?