Re: [PATCH 4/8] build: Remove CONFIG_HAS_PDX

[Alejandro Vallejo <alejandro.vallejo@cloud.com>]

On Tue, Jul 18, 2023 at 11:38:14AM +0200, Jan Beulich wrote:
> On 18.07.2023 11:35, Andrew Cooper wrote:
> > On 18/07/2023 10:19 am, Jan Beulich wrote:
> >> On 17.07.2023 18:03, Alejandro Vallejo wrote:
> >>> It's set everywhere and can't be turned off because it's presence is
> >>> assumed in several parts of the codebase. This is an initial patch towards
> >>> adding a more fine-grained CONFIG_HAS_PDX_COMPRESSION that can actually be
> >>> disabled on systems that don't typically benefit from it.
> >>>
> >>> No functional change.
> >>>
> >>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> >> On its own I don't think this is okay, as it's unclear whether it would
> >> affect RISC-V or PPC in a negative way.
> > 
> > Neither could compile without layering violations of PDX logic in common
> > code, and neither have got that far yet.
> > 
> > Now is an excellent time to be doing this, because it removes work for
> > RISC-V/PPC...
> > 
> >>  If at all, it should only go in
> >> together with the later re-introduction of a CONFIG_*.
I could merge this patch with patch 8. I do feel they tackle different
matters, but when HAS_PDX goes away doesn't matter.

> >> Still I question
> >> whether that new CONFIG_HAS_PDX_COMPRESSION (which imo ought to be
> >> CONFIG_PDX_COMPRESSION)
Sure, I'll change that in v2.

> >> then wouldn't better depend on the arch-selected
> >> HAS_PDX, such that it wouldn't wrongly be offered to choose a value when
> >> compression isn't supported in the first place.
There are several concepts to consider:

  * Frame numbers (mfn)
  * Frame table indices (pdx)
  * Mapping between the 2 (pdx_to_pfn/pfn_to_pdx)

  (I purposefully ignore the directmap. There's a similar argument for it)

An arch opting out of pdx and an arch opting out of pdx compression are in
the exact same situation, except the later has the ability to easily toggle
it back on. Using pdx (_not_ compression) as a first-class type has several
advantages.

  1. It allows common code to deal with pdx too. There are some conversions
     to/from pdx that have to do with arch-specific code calling common
     code or vice-versa. This is particularly bad in the presence of
     compression This is particularly bad in the presence of compression
     because it implies fairly frequent reads of global state.
  2. It allows a port to get pdx-compression for free if it opts into it;
     and more importantly, the ability to toggle it.
  3. Simplifies the compression removal logic. Otherwise #ifdefs need to be
     sprinkled around any architecture that may want to toggle it and
     that's several orders of magnitude more difficult to read.

TL;DR: There is not a benefit in a new port purposefuilly avoiding PDX.
It's just a name for a particular index. It's _compression_ that makes it
have a whacky relationship with mfns and might want toggling.

Incidentally, note that common/numa.c does use PDX.

> > 
> > ... although I do agree that the resulting option shouldn't be user
> > selectable.  It's a property of the architecture, not something a user
> > should be worrying about.
> 
> I disagree. Exotic x86 hardware may or may not want supporting, which
> can only be determined by the build meister, as long as this is indeed
> meant to become a build-time decision (rather than a runtime one; see
> my response to the cover letter of this series).
> 
> Jan
I won't get into x86, because I have never seen such exotic hardware, but
ARM definitely has a lot more heterogeneous system designs where it might
be needed on a system-by-system basis.

Alejandro