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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.07.2023 19:38, Andrew Cooper wrote:
> 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?

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