Re: [PATCH V5 RESEND 00/14] TopDown metrics support for Icelake

From: Peter Zijlstra <peterz@infradead.org>
To: Stephane Eranian <eranian@google.com>
Cc: "Liang, Kan" <kan.liang@linux.intel.com>,
	Arnaldo Carvalho de Melo <acme@redhat.com>,
	Ingo Molnar <mingo@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Jiri Olsa <jolsa@kernel.org>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Andi Kleen <ak@linux.intel.com>
Subject: Re: [PATCH V5 RESEND 00/14] TopDown metrics support for Icelake
Date: Mon, 20 Apr 2020 19:02:03 +0200	[thread overview]
Message-ID: <20200420170203.GL20730@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <CABPqkBTSwoCOt_pOQvX4qSTy6D5DKYvZVXJd66Vf=2mGVsOpvQ@mail.gmail.com>

On Mon, Apr 20, 2020 at 09:00:56AM -0700, Stephane Eranian wrote:
> Hi,
> 
> On Fri, Jan 10, 2020 at 5:17 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Mon, Jan 06, 2020 at 12:29:05PM -0800, kan.liang@linux.intel.com wrote:
> > > From: Kan Liang <kan.liang@linux.intel.com>
> > >
> > > Icelake has support for measuring the level 1 TopDown metrics
> > > directly in hardware. This is implemented by an additional METRICS
> > > register, and a new Fixed Counter 3 that measures pipeline SLOTS.
> > >
> > > New in Icelake
> > > - Do not require generic counters. This allows to collect TopDown always
> > >   in addition to other events.
> > > - Measuring TopDown per thread/process instead of only per core
> > >
> > > For the Ice Lake implementation of performance metrics, the values in
> > > PERF_METRICS MSR are derived from fixed counter 3. Software should start
> > > both registers, PERF_METRICS and fixed counter 3, from zero.
> > > Additionally, software is recommended to periodically clear both
> > > registers in order to maintain accurate measurements. The latter is
> > > required for certain scenarios that involve sampling metrics at high
> > > rates. Software should always write fixed counter 3 before write to
> > > PERF_METRICS.
> >
> > Do we really have to support this trainwreck? This is such ill designed
> > hardware, I'm loath to support it, it might encourage more such
> > 'creative' things and we really don't need that.
> >
> Yes, we do because it provides important information per hyper-thread.
> 
> I understand that the hardware is convoluted to support because it
> introduces a new concept: a single counter computing multiple high
> level metrics. It is difficult to abstract cleanly especially when you
> add on top that it is connected with a new fixed counter (SLOTS).

It's not a new concept, it's just completely idiotic. It didn't need to
be this crazy. There is absolutely no sane reason for it to be this
crazy.

The 4 counters in a single msr thing is insane because it uses a
division.

Very much worse, it explicitly uses the exact value of another counter
(SLOTS) to drive that division, creating a tight coupling between the
registers and completely and utterly destroying the SLOTS counter.

Since it keeps internal 'shadow' counters for the 4 events anyway, it
might as well have kept a shadow counter for the SLOTS event and driven
it off of that, that would have kept the SLOTS counter sane, but nooo,
gotta wreck that.

> That also helps the kernel with a single WRMSR/RDMSR for all 4 metrics.

I also really don't buy that as a driver for all this insanity.
Optimizing MSRs to not be utterly stupid expensive would've been so much
saner and would've helped everyone.

This is just creating more wreckage.

What I really want to know is if future hardware is going to be as
stupid; or if there's going to be change. I really don't want to commit
to ABI here and then have to find out they fixed the hardware and we
can't do sane things anymore.

Obviously, future hardware is not something that is to be discussed, so
we're at a stand-still here.