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

[Julien Grall <julien@xxxxxxx>]

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.
> > 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?
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?
Cheers,

--
Julien Grall