Re: [PATCH v3 0/4] Make PDX compression optional

[Julien Grall <julien@xxxxxxx>]

Hi,

On 16/08/2023 14:14, Jan Beulich wrote:
> On 16.08.2023 15:06, Julien Grall wrote:
> > Hi,
> >
> > On 16/08/2023 12:27, Jan Beulich wrote:
> > > On 16.08.2023 13:12, Julien Grall wrote:
> > > > Hi Jan,
> > > >
> > > > On 16/08/2023 10:43, Jan Beulich wrote:
> > > > > On 16.08.2023 11:36, Alejandro Vallejo wrote:
> > > > > > On Tue, Aug 08, 2023 at 02:02:16PM +0100, 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.
> > > > > > >
> > > > > > > Series summary:
> > > > > > >
> > > > > > > Patch 1 Moves hard-coded compression-related logic to helper functions
> > > > > > > Patch 2 Refactors all instances of regions being validated for pdx
> > > > > > >            compression conformance so it's done through a helper
> > > > > > > Patch 3 Non-functional reorder in order to simplify the patch 8 diff
> > > > > > > Patch 4 Adds new Kconfig option to compile out PDX compression and removes
> > > > > > >            the old CONFIG_HAS_PDX, as it was non removable
> > > > > > >
> > > > > > > Already committed:
> > > > > > >
> > > > > > > v1/patch 1 documents the current general understanding of the pdx concept and
> > > > > > >               pdx compression in particular
> > > > > > > v1/patch 3 Marks the pdx compression globals as ro_after_init
> > > > > > > v2/patch 1 Documents the differences between arm32 and arm64 directmaps
> > > > > > >
> > > > > > > Alejandro Vallejo (4):
> > > > > > >      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_PDX_COMPRESSION as a common Kconfig option
> > > > > > @Jan: Just making sure, are you generally ok with this series as-is?
> > > > > Well, okay would be too strong; I still don't see why my runtime patching
> > > > > series isn't re-considered.
> > > > Do you have a pointer to the series? I would be interested to have a look.
> > > Sure, I can dig it out a 2nd time:
> > > https://lists.xenproject.org/archives/html/xen-devel/2018-09/msg01616.html
> > Thanks! AFAIU, the optimization would only happen after the alternative
> > has been patched. This is happening after initializing the heap. With
> > Alejandro series, you will have some performance gain for the boot which
> > I care about.
> Fair aspect.
>
> > > > That said... the problem with alt-patching is this is architectural
> > > > specific. Right now, this seems to be a bit unnecessary given that we
> > > > believe that virtually no-one will have a platform (I know we talked
> > > > about a potential one...) where PDX is compressing.
> > > But it defaults to enabled on other than x86 anyway. So it seems like
> > > it's generally wanted everywhere except on x86, and on x86 it can
> > > (could) be patched out.
> > IIUC, you are saying that we would want both Alejandro series (to allow
> > an admin to disable PDX at boot) and your series (to fully optimize the
> > PDX at runtime). Is that correct?
> Not really, no. I don't view a build-time decision as necessary; I favor
> runtime decisions whenever possible.
At least on Arm, we want to cater two different set of users. One set 
will want to tailor Xen to there platform and runtime decision may not 
be desirable. The other set (e.g. distros) will want to run Xen 
everywhere so runtime is preferable.
So I think we should be able to offer both build and runtime option when 
it makes sense. The PDX is one example where both could be interesting, 
yet at the moment there seem to be an appetite for build time only 
configuration.
> > If so, it is unclear to me why you want your series to be re-considered
> > now. Can't this be done as a follow-up if there is a desire to further
> > optimize?
> In principle it could be, yes, but I'm afraid I know that no follow-up
> is going to happen (and me trying to revive the earlier work would be
> yet another case of pointlessly invested time).
Right which is why I wrote "desire". But my point was that Alejandro 
work is not a one-way door. It would still possible to have runtime 
patching on top of it.
Cheers,

--
Julien Grall