Re: [PATCH v2 5/5] pdx: Add CONFIG_HAS_PDX_COMPRESSION as a common Kconfig option

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

Hi,

A few days have passed. May I suggest taking a step back?

On Tue, Aug 01, 2023 at 09:57:57AM +0200, Jan Beulich wrote:
> On 31.07.2023 19:38, Andrew Cooper wrote:
> > There's one system which never made its way into production,
> > support-for-which in the no-op case is causing a 10% perf hit in at
> > least one metric, 17% in another.
> > 
> > ... and your seriously arguing that we should continue to take this perf
> > hit?
> 
> Have I asked anywhere to keep this enabled by default? Have I been the
> one to first propose a way to reduce this overhead?
> 
> > It is very likely that said machine(s) aren't even powered on these
> > days, and even if it is on, the vendor can take the overhead of turning
> > PDX compression on until such time as they make a production system.
> > 
> > Furthermore, it is unrealistic to think that such a machine will ever
> > make its way into production.  Linux has never PDX compression, and
> > by-and-large if it doesn't run Linux, you can't sell it in the first place.
> 
> I'm sure you recall that Linux has much more VA space for its directmap,
> so aiui there simply was no immediate need there.
> 
> > It is utterly unreasonable to be carrying this overhead in the first
> > place.  PDX compression *should not* have been committed on-by-default
> > in the first place.  (Yes, I know there was no Kconfig back then, and
> > the review process was non-existent, but someone really should have said
> > no).
> 
> Are you suggesting no code should be committed supporting new hardware,
> ahead of that hardware going public?
> 
> > It is equally unreasonable to offer people (under Expert or not) an
> > ability to shoot themselves in the foot like this.
> 
> Shoot themselves in the foot? If you enable EXPERT, you better know
> what you're doing.
> 
> >>>   Indeed, Julien's
> >>>> quick metric shows how much performance we waste by having it enabled.
> >>> Further to this, bloat-o-meter says net -30k of code and there are
> >>> plenty of fastpaths getting a several cacheline reduction from this.
> >> A similar reduction was achieved
> > 
> > Really?  You think that replacing the innermost shift and masking with
> > an alternative that has a shorter instruction sequence gets you the same
> > net reduction in code?
> > 
> > I do not find that claim credible.
> 
> Did you ever seriously look at those patches?
> 
> >>  by the BMI2-alt-patching series I
> >> had put together, yet you weren't willing to come to consensus on
> >> it.
> > 
> > You have AMD machines, and your patch was alleged to be a performance
> > improvement.  So the fact you didn't spot the problems with PEXT/PDEP on
> > all AMD hardware prior to Fam19h suggests there was insufficient testing
> > for an alleged performance improvement.
> > 
> > The patch you posted:
> > 
> > 1) Added extra complexity via alternatives, and
> > 2) Reduced performance on AMD systems prior to Fam19h.
> > 
> > in an area of code which useless on all shipping x86 systems.
> > 
> > You literally micro-anti-optimised a no-op path to a more expensive (on
> > one vendor at least) no-op path, claiming it to be a performance
> > improvement.
> 
> You appear to forget the patch patching to MOV instructions ("x86: use
> MOV for PFN/PDX conversion when possible"). Are you saying MOV has
> performance problems on any CPU?
> 
> Jan

I think we can all agree that (a) the _current_ pdx code implies taking a
perf hit and (b) that we can't just remove PDX compression because it's in
current use in certain systems. The contentious points seem to be strictly
in the status it should hold by default and the means to reduce the perf
hit when it's on.

For the status:

I think it's clear it must remain at least on ARM. I believe Jan argued
about the choice of default in terms of "when" to turn it off, rather than
"if". Seeing that...

> On 18.07.2023 9:33, Jan Beulich wrote:
> I disagree with this choice of default for x86. To avoid surprising
> downstreams, this should at best be a two-step process: Keep the
> default as Y right now, and switch to N a couple of releases later.
... so I think a more productive discussion is following up on that,
looking at the "why", "when", "how" and "risks-involved" rather than going
in circles (more on this at the end of the email).

Now, for the perf hit reduction:

An alt-patching series can probably make it very close to the perf win of
this patch as long as it transforms the conversion hunks into no-ops when
there's no hole. I looked into the 2018 patch, and I don't think it tried
to go that far (afaics it's purpose is to inline the compression parameters
into the code stream). I highly suspect it would still noticiably
underperform compared to this one, but I admit it's guesswork and I'd be
happy to be proven wrong through benchmarks.

Regardless that's a lot more complexity than I was willing to tackle here
seeing that it's use is limited on x86.

Compiling-out and alt-patching compression are not necessarily incompatible
with each other though. It would, in fact, do wonders for ARM, where the
exact same binary might have to run on separate processors with different
memory map configurations. I do think someone ought to do it in the long
run as a performance improvement for that port (if only because
alt-patching is arch-specific), but I really struggle to justify the dev
effort of writing, benchmarking, testing and maintining all that
infrastructure when there's no (known) machine I can buy to test it on.

Summary:
  * While alt-patching is an attractive alternative this series doesn't do
    that and in the spirit of keeping things simple I'd really rather keep
    it that way. Does this sound reasonable?
  * For the topic of when to disable compression by default on x86, I
    genuinely think now's as good a time as any. If we were to do it in 2
    steps any project downstream may very well not notice until 2 releases
    down the line, at which point they simply must turn compression back
    on, which is what they would have to do now anyway.
  * For the topic of allowing or not the option to be selectable, I think
    it would be a mistake to preclude it because while we don't know of
    physical memory maps with big holes on (publicly available) x86, Xen
    may be itself virtualized with arbitrary memory maps. Unlikely and far
    fetched as it is, it's IMO worth being at least cautious about. Gating
    the feature on EXPERT=y and adding a big warning should be good enough
    to avoid foot-shootouts.

Thanks,
Alejandro