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

[Jan Beulich <jbeulich@xxxxxxxx>]

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. But as said I also don't mind this
series going in.

> 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).

Jan