Currently there's a CONFIG_HAS_PDX Kconfig option, but it's impossible to
disable it because the whole codebase performs unconditional
compression/decompression operations on addresses. This has the
unfortunate side effect that systems without a need for compression still
have to pay the performance impact of juggling bits on every pfn<->pdx
conversion (this requires reading several global variables). This series
attempts to:

  * Leave the state of pdx and pdx compression documented
  * Factor out compression so it _can_ be removed through Kconfig
  * Make it so compression is disabled on x86 and enabled on both Aarch32
    and Aarch64 by default.

Series summary:

Patch 1 documents the current general understanding of the pdx concept and
        pdx compression in particular
Patch 2 makes a clarification in ARM code to explain some discrepancies
        between the concept of "directmap" in arm32 and arm64 relevant to
        this series (i.e: why this series doesn't touch arm32 in directmap
        accesses).
Patch 3 Marks the pdx compression globals as ro_after_init
Patch 4 Gets rid of the current CONFIG_HAS_PDX option because (a) the name
        is misleading (b) cannot be removed in its current form.
Patch 5 Moves hard-coded compression-related logic to helper functions
Patch 6 Refactors all instances of regions being validated for pdx
        compression conformance so it's done through a helper
Patch 7 Non-functional reorder in order to simplify the patch 8 diff
Patch 8 Adds new Kconfig option to compile out PDX compression

Alejandro Vallejo (8):
  mm/pdx: Add comments throughout the codebase for pdx
  arm/mm: Document the differences between arm32 and arm64 directmaps
  pdx: Mark pdx hole description globals readonly after boot
  build: Remove CONFIG_HAS_PDX
  mm: Factor out the pdx compression logic in ma/va converters
  mm/pdx: Standardize region validation wrt pdx compression
  pdx: Reorder pdx.[ch]
  pdx: Add CONFIG_HAS_PDX_COMPRESSION as a Kconfig option

 xen/arch/arm/Kconfig                   |   1 -
 xen/arch/arm/include/asm/mm.h          |  30 ++-
 xen/arch/x86/Kconfig                   |   1 -
 xen/arch/x86/domain.c                  |  19 +-
 xen/arch/x86/include/asm/x86_64/page.h |  28 ++-
 xen/arch/x86/x86_64/mm.c               |   2 +-
 xen/common/Kconfig                     |  13 +-
 xen/common/Makefile                    |   2 +-
 xen/common/efi/boot.c                  |   6 +-
 xen/common/pdx.c                       | 120 +++++++++---
 xen/include/xen/mm.h                   |  11 ++
 xen/include/xen/pdx.h                  | 241 +++++++++++++++++++++++--
 12 files changed, 405 insertions(+), 69 deletions(-)

-- 
2.34.1

Hi Alejandro,

Great work!

On 17/07/2023 17:03, Alejandro Vallejo wrote:
> Currently there's a CONFIG_HAS_PDX Kconfig option, but it's impossible to
> disable it because the whole codebase performs unconditional
> compression/decompression operations on addresses. This has the
> unfortunate side effect that systems without a need for compression still
> have to pay the performance impact of juggling bits on every pfn<->pdx
> conversion (this requires reading several global variables). This series
> attempts to:
Just as a datapoint. I applied this to a tree with Live-Update support. 
 From the basic test I did, this is reducing the downtime by 10% :).

Cheers,

-- 
Julien Grall

On 20/07/2023 11:00 pm, Julien Grall wrote:
> Hi Alejandro,
>
> Great work!
>
> On 17/07/2023 17:03, Alejandro Vallejo wrote:
>> Currently there's a CONFIG_HAS_PDX Kconfig option, but it's
>> impossible to
>> disable it because the whole codebase performs unconditional
>> compression/decompression operations on addresses. This has the
>> unfortunate side effect that systems without a need for compression
>> still
>> have to pay the performance impact of juggling bits on every pfn<->pdx
>> conversion (this requires reading several global variables). This series
>> attempts to:
> Just as a datapoint. I applied this to a tree with Live-Update
> support. From the basic test I did, this is reducing the downtime by
> 10% :).

I'm not surprised in the slightest.

We've had many cases that prove that compression (of 0 bits, on all x86
systems) is a disaster perf wise, and its used in pretty much every
fastpath in Xen.

Look no further than c/s 564d261687c and the 10% improvements in general
PV runtime too, and that was optimising away one single instance in one
single fastpath.

It's also why I'm not entertaining the concept of leaving it active or
selectable on x86.

~Andrew

On 17.07.2023 18:03, Alejandro Vallejo wrote:
> Currently there's a CONFIG_HAS_PDX Kconfig option, but it's impossible to
> disable it because the whole codebase performs unconditional
> compression/decompression operations on addresses. This has the
> unfortunate side effect that systems without a need for compression still
> have to pay the performance impact of juggling bits on every pfn<->pdx
> conversion (this requires reading several global variables). This series
> attempts to:
> 
>   * Leave the state of pdx and pdx compression documented
>   * Factor out compression so it _can_ be removed through Kconfig
>   * Make it so compression is disabled on x86 and enabled on both Aarch32
>     and Aarch64 by default.

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.

But that's only the simple / mechanical side. Considering my earlier
effort to reduce / remove the involved overhead dynamically at
runtime (which you may or may not be aware of; see [2]), I view a
compile time choice as less desirable. At the very least I would
expect some justification towards this build time choice being
acceptable / reasonable despite the earlier effort towards greater
flexibility. Only such would be likely to have me merely defer to
other x86 maintainers, rather than outright objecting.

Jan

[2] https://lists.xenproject.org/archives/html/xen-devel/2018-09/msg01616.html

On Tue, Jul 18, 2023 at 11:33:03AM +0200, Jan Beulich wrote:
> On 17.07.2023 18:03, Alejandro Vallejo wrote:
> > Currently there's a CONFIG_HAS_PDX Kconfig option, but it's impossible to
> > disable it because the whole codebase performs unconditional
> > compression/decompression operations on addresses. This has the
> > unfortunate side effect that systems without a need for compression still
> > have to pay the performance impact of juggling bits on every pfn<->pdx
> > conversion (this requires reading several global variables). This series
> > attempts to:
> > 
> >   * Leave the state of pdx and pdx compression documented
> >   * Factor out compression so it _can_ be removed through Kconfig
> >   * Make it so compression is disabled on x86 and enabled on both Aarch32
> >     and Aarch64 by default.
> 
> 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.
I'm not particularly worried about the default, as that's easy to change
locally for our customers. That said, my .02 on the matter is that it would
be easier to accept (leaving it as Y) if there was some documented case of
the feature being triggered at present on x86. I'd rather turn it off
unless we have strong evidence that it's actually used. Forcing Xen users
to pay the cost of a feature they don't benefit from just feels wrong.

> 
> But that's only the simple / mechanical side. Considering my earlier
> effort to reduce / remove the involved overhead dynamically at
> runtime (which you may or may not be aware of; see [2]),
I wasn't. I'll have a look at the end of the day[3]. Regardless, most of this
series is about containment of compression into known helpers and hopefully
that should be undeniably positive irrespective of how the overhead
reduction is achieved.

> I view a
> compile time choice as less desirable. At the very least I would
> expect some justification towards this build time choice being
> acceptable / reasonable despite the earlier effort towards greater
> flexibility. Only such would be likely to have me merely defer to
> other x86 maintainers, rather than outright objecting.
> 
> Jan
> 
> [2] https://lists.xenproject.org/archives/html/xen-devel/2018-09/msg01616.html

I believe the burden of proof is reversed. Features bring complexity, and
complexity increases the chances of introducing bugs. It's the presence of
a feature that ought to be backed by a proof-of-requirement, not its
absence.

As far as data goes, we're aware of several ARM systems with compressible
memory banks, but no publicly available x86 ones. I'm purposefully leaving
RISC-V and PPC out of this equation, as they may or may not require this,
it's something the maintainers of those ports will have to assess in due
time.

Alejandro

[3] Note that I send this email before reading carefully your 2018 series.

On 18.07.2023 14:58, Alejandro Vallejo wrote:
> I believe the burden of proof is reversed. Features bring complexity, and
> complexity increases the chances of introducing bugs. It's the presence of
> a feature that ought to be backed by a proof-of-requirement, not its
> absence.

The feature was introduced to support hardware a partner of ours was
working on at the time. Xen wouldn't have worked very well there
without these additions. It is beyond my control or knowledge whether
any such system has ever made it into the public. So at the time of
its introduction, the need for this code was well justified imo.

Jan

On Tue, Jul 18, 2023 at 03:06:26PM +0200, Jan Beulich wrote:
> On 18.07.2023 14:58, Alejandro Vallejo wrote:
> > I believe the burden of proof is reversed. Features bring complexity, and
> > complexity increases the chances of introducing bugs. It's the presence of
> > a feature that ought to be backed by a proof-of-requirement, not its
> > absence.
> 
> The feature was introduced to support hardware a partner of ours was
> working on at the time. Xen wouldn't have worked very well there
> without these additions. It is beyond my control or knowledge whether
> any such system has ever made it into the public. So at the time of
> its introduction, the need for this code was well justified imo.
> 
> Jan
Oh, of course. I don't question the legitimacy of its introduction at all,
nor do I question the matter of its optional presence. I do question the
default considering the public data we have available.

Alejandro