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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 31/07/2023 9:00 am, Jan Beulich wrote:
> On 28.07.2023 18:58, Andrew Cooper wrote:
>> On 28/07/2023 5:36 pm, Andrew Cooper wrote:
>>> On 28/07/2023 8:59 am, Alejandro Vallejo wrote:
>>>> Adds a new compile-time flag to allow disabling pdx compression and
>>>> compiles out compression-related code/data. It also shorts the pdx<->pfn
>>>> conversion macros and creates stubs for masking fucntions.
>>>>
>>>> While at it, removes the old arch-defined CONFIG_HAS_PDX flag, as it was
>>>> not removable in practice.
>>>>
>>>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
>>>> ---
>>>> v2:
>>>>   * Merged v1/patch2: Removal of CONFIG_HAS_PDX here (Jan)
>>> This series is now looking fine, except for the Kconfig aspect.
>>>
>>> This is not something any user or developer should ever be queried
>>> about.  The feedback on the documentation patches alone show that it's
>>> not understood well by the maintainers, even if the principle is accepted.
>>>
>>> There is never any reason to have this active on x86.
> We can of course continue to disagree here. At least with EXPERT=y
> selecting this option ought to remain possible for x86. Whether or
> not the original systems this scheme was developed for ever went
> public, such systems did (do) exist, and hence running Xen sensibly
> on them (without losing all memory except that on node 0) ought to
> be possible.

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?

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.


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).

It is equally unreasonable to offer people (under Expert or not) an
ability to shoot themselves in the foot like this.


If in the very unlikely case that such a system does come into
existence, we can consider re-enabling PDX compression (and by that, I
mean doing it in a less invasive way in the common case), but there's
very little chance this will ever be a path we need to take.

>>   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.

>  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.

There is no possible way any form of your patch can ever beat
Alejandro's work of just compiling-out the useless logic wholesale.

~Andrew