From: Alejandro Vallejo <alejandro.vallejo@cloud.com>

Adds a new compile-time flag to allow disabling PDX compression and
compiles out compression-related code/data. It also shorts the pdx<->pfn
conversion macros and creates stubs for masking functions.

While at it, removes the old arch-defined CONFIG_HAS_PDX flag.  Despite the
illusion of choice, it was not optional.

There are ARM and PPC platforms with sparse RAM banks - leave compression
active by default there.  OTOH, there are no known production x86 systems with
sparse RAM banks, so disable compression.  This decision can be revisited if
such a platform comes along.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Shawn Anastasio <sanastasio@raptorengineering.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
CC: Henry Wang <Henry.Wang@arm.com>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>

v5:
 * Fix typos
 * Fix what would be a MISRA violation from mismatch paddr_t/unsigned long
 * Rebase over PPC's use of "select HAS_PDX"
 * Rework Kconfig text for non-developers to follow

It is hard to overstate how much of a performace improvement this is on x86.
The compiletime delta is:

  add/remove: 1/24 grow/shrink: 16/401 up/down: 2317/-32367 (-30050)

with 5 memory reads removed from every fastpath involving pagetable
manipulation - which is many.  Prior more-targeted optimisations have netted
gains of 10% by avoiding this pagetable overhead at specific points in the
context switch path.

Oleksii: I've not touched RISCV yet, because I don't know how the platforms
typically look.  I'm happy to default it active in RISCV too if that's the
right thing to do.

But for everyone else, it's probably very worthwhile looking for alternative
options to support multiple RAM banks...
---
 xen/arch/arm/Kconfig  |  1 -
 xen/arch/ppc/Kconfig  |  1 -
 xen/arch/x86/Kconfig  |  1 -
 xen/arch/x86/domain.c | 19 +++++++++++++------
 xen/common/Kconfig    | 18 +++++++++++++++---
 xen/common/Makefile   |  2 +-
 xen/common/pdx.c      | 16 ++++++++++++----
 xen/include/xen/pdx.h | 38 +++++++++++++++++++++++++++++++++++---
 8 files changed, 76 insertions(+), 20 deletions(-)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 632dd9792e3c..2939db429b78 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -14,7 +14,6 @@ config ARM
 	select HAS_ALTERNATIVE
 	select HAS_DEVICE_TREE
 	select HAS_PASSTHROUGH
-	select HAS_PDX
 	select HAS_PMAP
 	select HAS_UBSAN
 	select IOMMU_FORCE_PT_SHARE
diff --git a/xen/arch/ppc/Kconfig b/xen/arch/ppc/Kconfig
index a6eae597afac..ab116ffb2a70 100644
--- a/xen/arch/ppc/Kconfig
+++ b/xen/arch/ppc/Kconfig
@@ -1,7 +1,6 @@
 config PPC
 	def_bool y
 	select HAS_DEVICE_TREE
-	select HAS_PDX
 
 config PPC64
 	def_bool y
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 92f3a627da3c..30df085d969e 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -24,7 +24,6 @@ config X86
 	select HAS_PASSTHROUGH
 	select HAS_PCI
 	select HAS_PCI_MSI
-	select HAS_PDX
 	select HAS_SCHED_GRANULARITY
 	select HAS_UBSAN
 	select HAS_VPCI if HVM
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 8e0af2278104..59a6a2368876 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -458,7 +458,7 @@ void domain_cpu_policy_changed(struct domain *d)
     }
 }
 
-#ifndef CONFIG_BIGMEM
+#if !defined(CONFIG_BIGMEM) && defined(CONFIG_PDX_COMPRESSION)
 /*
  * The hole may be at or above the 44-bit boundary, so we need to determine
  * the total bit count until reaching 32 significant (not squashed out) bits
@@ -485,13 +485,20 @@ static unsigned int __init noinline _domain_struct_bits(void)
 struct domain *alloc_domain_struct(void)
 {
     struct domain *d;
-#ifdef CONFIG_BIGMEM
-    const unsigned int bits = 0;
-#else
+
     /*
-     * We pack the PDX of the domain structure into a 32-bit field within
-     * the page_info structure. Hence the MEMF_bits() restriction.
+     * Without CONFIG_BIGMEM, we pack the PDX of the domain structure into
+     * a 32-bit field within the page_info structure. Hence the MEMF_bits()
+     * restriction. With PDX compression in place the number of bits must
+     * be calculated at runtime, but it's fixed otherwise.
+     *
+     * On systems with CONFIG_BIGMEM there's no packing, and so there's no
+     * such restriction.
      */
+#if defined(CONFIG_BIGMEM) || !defined(CONFIG_PDX_COMPRESSION)
+    const unsigned int bits = IS_ENABLED(CONFIG_BIGMEM) ? 0 :
+                                                          32 + PAGE_SHIFT;
+#else
     static unsigned int __read_mostly bits;
 
     if ( unlikely(!bits) )
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 0d248ab94108..4ba9884f52c9 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -23,6 +23,21 @@ config GRANT_TABLE
 
 	  If unsure, say Y.
 
+config PDX_COMPRESSION
+	bool "PDX (Page inDeX) compression" if EXPERT && !X86
+	default ARM || PPC
+	help
+	  PDX compression is a technique designed to reduce the memory
+	  overhead of physical memory management on platforms with sparse RAM
+	  banks.
+
+	  If your platform does have sparse RAM banks, enabling PDX
+	  compression may reduce the memory overhead of Xen, but does carry a
+	  runtime performance cost.
+
+	  If your platform does not have sparse RAM banks, do not enable PDX
+	  compression.
+
 config ALTERNATIVE_CALL
 	bool
 
@@ -53,9 +68,6 @@ config HAS_IOPORTS
 config HAS_KEXEC
 	bool
 
-config HAS_PDX
-	bool
-
 config HAS_PMAP
 	bool
 
diff --git a/xen/common/Makefile b/xen/common/Makefile
index e7e96b1087ae..69d6aa626c7f 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -30,7 +30,7 @@ obj-y += multicall.o
 obj-y += notifier.o
 obj-$(CONFIG_NUMA) += numa.o
 obj-y += page_alloc.o
-obj-$(CONFIG_HAS_PDX) += pdx.o
+obj-y += pdx.o
 obj-$(CONFIG_PERF_COUNTERS) += perfc.o
 obj-bin-$(CONFIG_HAS_PMAP) += pmap.init.o
 obj-y += preempt.o
diff --git a/xen/common/pdx.c b/xen/common/pdx.c
index d3d38965bde9..d3d63b075032 100644
--- a/xen/common/pdx.c
+++ b/xen/common/pdx.c
@@ -31,11 +31,16 @@ unsigned long __read_mostly pdx_group_valid[BITS_TO_LONGS(
 
 bool __mfn_valid(unsigned long mfn)
 {
-    if ( unlikely(evaluate_nospec(mfn >= max_page)) )
+    bool invalid = mfn >= max_page;
+
+#ifdef CONFIG_PDX_COMPRESSION
+    invalid |= mfn & pfn_hole_mask;
+#endif
+
+    if ( unlikely(evaluate_nospec(invalid)) )
         return false;
-    return likely(!(mfn & pfn_hole_mask)) &&
-           likely(test_bit(pfn_to_pdx(mfn) / PDX_GROUP_COUNT,
-                           pdx_group_valid));
+
+    return test_bit(pfn_to_pdx(mfn) / PDX_GROUP_COUNT, pdx_group_valid);
 }
 
 void set_pdx_range(unsigned long smfn, unsigned long emfn)
@@ -49,6 +54,8 @@ void set_pdx_range(unsigned long smfn, unsigned long emfn)
         __set_bit(idx, pdx_group_valid);
 }
 
+#ifdef CONFIG_PDX_COMPRESSION
+
 /*
  * Diagram to make sense of the following variables. The masks and shifts
  * are done on mfn values in order to convert to/from pdx:
@@ -176,6 +183,7 @@ void __init pfn_pdx_hole_setup(unsigned long mask)
     ma_top_mask         = pfn_top_mask << PAGE_SHIFT;
 }
 
+#endif /* CONFIG_PDX_COMPRESSION */
 
 /*
  * Local variables:
diff --git a/xen/include/xen/pdx.h b/xen/include/xen/pdx.h
index f3fbc4273aa4..bd535009ea8f 100644
--- a/xen/include/xen/pdx.h
+++ b/xen/include/xen/pdx.h
@@ -67,8 +67,6 @@
  * region involved.
  */
 
-#ifdef CONFIG_HAS_PDX
-
 extern unsigned long max_pdx;
 
 #define PDX_GROUP_COUNT ((1 << PDX_GROUP_SHIFT) / \
@@ -100,6 +98,8 @@ bool __mfn_valid(unsigned long mfn);
 #define mfn_to_pdx(mfn) pfn_to_pdx(mfn_x(mfn))
 #define pdx_to_mfn(pdx) _mfn(pdx_to_pfn(pdx))
 
+#ifdef CONFIG_PDX_COMPRESSION
+
 extern unsigned long pfn_pdx_bottom_mask, ma_va_bottom_mask;
 extern unsigned int pfn_pdx_hole_shift;
 extern unsigned long pfn_hole_mask;
@@ -206,7 +206,39 @@ static inline paddr_t directmapoff_to_maddr(unsigned long offset)
  */
 void pfn_pdx_hole_setup(unsigned long mask);
 
-#endif /* HAS_PDX */
+#else /* !CONFIG_PDX_COMPRESSION */
+
+/* Without PDX compression we can skip some computations */
+
+/* pdx<->pfn == identity */
+#define pdx_to_pfn(x) (x)
+#define pfn_to_pdx(x) (x)
+
+/* directmap is indexed by by maddr */
+#define maddr_to_directmapoff(x) (x)
+#define directmapoff_to_maddr(x) (x)
+
+static inline bool pdx_is_region_compressible(paddr_t base,
+                                              unsigned long npages)
+{
+    return true;
+}
+
+static inline uint64_t pdx_init_mask(uint64_t base_addr)
+{
+    return 0;
+}
+
+static inline uint64_t pdx_region_mask(uint64_t base, uint64_t len)
+{
+    return 0;
+}
+
+static inline void pfn_pdx_hole_setup(unsigned long mask)
+{
+}
+
+#endif /* CONFIG_PDX_COMPRESSION */
 #endif /* __XEN_PDX_H__ */
 
 /*

base-commit: 295514ff7550626de4fb5e43b51deb25d9331cd5
prerequisite-patch-id: a3880342434550d7612ebc2e760dcd098dabd1d9
-- 
2.30.2

> 
> Oleksii: I've not touched RISCV yet, because I don't know how the
> platforms
> typically look.  I'm happy to default it active in RISCV too if
> that's the
> right thing to do.
We are still waiting for a platform with hypervisor extension support
but I am using PDX in my Xen's repo.

~ Oleksii

On 10/10/2023 3:58 pm, Oleksii wrote:
>> Oleksii: I've not touched RISCV yet, because I don't know how the
>> platforms
>> typically look.  I'm happy to default it active in RISCV too if
>> that's the
>> right thing to do.
> We are still waiting for a platform with hypervisor extension support
> but I am using PDX in my Xen's repo.

Yes, but are you using that because Xen wouldn't build without it, or
because it's necessary for RISC-V platforms?

This patch fixes the problem where PDX had the illusion of being
necessary, but was actually mandatory.

~Andrew

On Tue, 2023-10-10 at 16:52 +0800, Andrew Cooper wrote:
> On 10/10/2023 3:58 pm, Oleksii wrote:
> > > Oleksii: I've not touched RISCV yet, because I don't know how the
> > > platforms
> > > typically look.  I'm happy to default it active in RISCV too if
> > > that's the
> > > right thing to do.
> > We are still waiting for a platform with hypervisor extension
> > support
> > but I am using PDX in my Xen's repo.
> 
> Yes, but are you using that because Xen wouldn't build without it, or
> because it's necessary for RISC-V platforms?
You are right I am using it because before this patch Xen can't be
built without PDX.
This is not necessary for RISC-V. At least platform I know they don't
use sparse RAM banks.

> 
> This patch fixes the problem where PDX had the illusion of being
> necessary, but was actually mandatory.
> 
> ~Andrew

~ Oleksii

On 11/10/2023 10:31 pm, Oleksii wrote:
> On Tue, 2023-10-10 at 16:52 +0800, Andrew Cooper wrote:
>> On 10/10/2023 3:58 pm, Oleksii wrote:
>>>> Oleksii: I've not touched RISCV yet, because I don't know how the
>>>> platforms
>>>> typically look.  I'm happy to default it active in RISCV too if
>>>> that's the
>>>> right thing to do.
>>> We are still waiting for a platform with hypervisor extension
>>> support
>>> but I am using PDX in my Xen's repo.
>> Yes, but are you using that because Xen wouldn't build without it, or
>> because it's necessary for RISC-V platforms?
> You are right I am using it because before this patch Xen can't be
> built without PDX.
> This is not necessary for RISC-V. At least platform I know they don't
> use sparse RAM banks.

Ok thanks.  I'll leave the code as is, but tweak the commit message to
include RISC-V alongside x86.

And as already noted, we can always revisit the decision in the future
if things change.

~Andrew

Hi,

> On Oct 12, 2023, at 01:25, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> On 11/10/2023 10:31 pm, Oleksii wrote:
>> On Tue, 2023-10-10 at 16:52 +0800, Andrew Cooper wrote:
>>> On 10/10/2023 3:58 pm, Oleksii wrote:
>>>>> Oleksii: I've not touched RISCV yet, because I don't know how the
>>>>> platforms
>>>>> typically look.  I'm happy to default it active in RISCV too if
>>>>> that's the
>>>>> right thing to do.
>>>> We are still waiting for a platform with hypervisor extension
>>>> support
>>>> but I am using PDX in my Xen's repo.
>>> Yes, but are you using that because Xen wouldn't build without it, or
>>> because it's necessary for RISC-V platforms?
>> You are right I am using it because before this patch Xen can't be
>> built without PDX.
>> This is not necessary for RISC-V. At least platform I know they don't
>> use sparse RAM banks.
> 
> Ok thanks.  I'll leave the code as is, but tweak the commit message to
> include RISC-V alongside x86.
> 
> And as already noted, we can always revisit the decision in the future
> if things change.

Looks like the discussion has been settled so:

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry


> 
> ~Andrew

On Fri, Oct 06, 2023 at 03:44:05PM +0100, Andrew Cooper wrote:
> From: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> 
> Adds a new compile-time flag to allow disabling PDX compression and
> compiles out compression-related code/data. It also shorts the pdx<->pfn
> conversion macros and creates stubs for masking functions.
> 
> While at it, removes the old arch-defined CONFIG_HAS_PDX flag.  Despite the
> illusion of choice, it was not optional.
> 
> There are ARM and PPC platforms with sparse RAM banks - leave compression
> active by default there.  OTOH, there are no known production x86 systems with
> sparse RAM banks, so disable compression.  This decision can be revisited if
> such a platform comes along.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> Reviewed-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

On 06/10/2023 15:44, Andrew Cooper wrote:
> From: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> 
> Adds a new compile-time flag to allow disabling PDX compression and
> compiles out compression-related code/data. It also shorts the pdx<->pfn
> conversion macros and creates stubs for masking functions.
> 
> While at it, removes the old arch-defined CONFIG_HAS_PDX flag.  Despite the
> illusion of choice, it was not optional.
> 
> There are ARM and PPC platforms with sparse RAM banks - leave compression
> active by default there.  OTOH, there are no known production x86 systems with
> sparse RAM banks, so disable compression.  This decision can be revisited if
> such a platform comes along.

(Process remarks rather than the code itself)

Jan is away this week so I want to make sure this doesn't go in without 
him having a say.

While I don't particularly care about the approach taken for x86, Jan 
voiced concerned with this approach and so far I didn't see any 
conclusion. If there is any, then please point me to them.

For the record, the objections from Jan are in [1]. If we want to ignore 
them, then I think we need a vote. Possibly only from the x86 folks (?).

> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

[1] b0296908-5081-5d34-8487-b8293eee97ca@suse.com

-- 
Julien Grall

On 06/10/2023 4:09 pm, Julien Grall wrote:
> 
> 
> On 06/10/2023 15:44, Andrew Cooper wrote:
>> From: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>>
>> Adds a new compile-time flag to allow disabling PDX compression and
>> compiles out compression-related code/data. It also shorts the pdx<->pfn
>> conversion macros and creates stubs for masking functions.
>>
>> While at it, removes the old arch-defined CONFIG_HAS_PDX flag. 
>> Despite the
>> illusion of choice, it was not optional.
>>
>> There are ARM and PPC platforms with sparse RAM banks - leave compression
>> active by default there.  OTOH, there are no known production x86
>> systems with
>> sparse RAM banks, so disable compression.  This decision can be
>> revisited if
>> such a platform comes along.
> 
> (Process remarks rather than the code itself)
> 
> Jan is away this week so I want to make sure this doesn't go in without
> him having a say.
> 
> While I don't particularly care about the approach taken for x86, Jan
> voiced concerned with this approach and so far I didn't see any
> conclusion. If there is any, then please point me to them.
> 
> For the record, the objections from Jan are in [1]. If we want to ignore
> them, then I think we need a vote. Possibly only from the x86 folks (?).


What do you think the 2 x86 maintainer tags on this patch in this exact
form, following far too much wasted time already, represents.  The vote
has already concluded.

~Andrew

On 06.10.2023 18:01, Andrew Cooper wrote:
> On 06/10/2023 4:09 pm, Julien Grall wrote:
>>
>>
>> On 06/10/2023 15:44, Andrew Cooper wrote:
>>> From: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>>>
>>> Adds a new compile-time flag to allow disabling PDX compression and
>>> compiles out compression-related code/data. It also shorts the pdx<->pfn
>>> conversion macros and creates stubs for masking functions.
>>>
>>> While at it, removes the old arch-defined CONFIG_HAS_PDX flag. 
>>> Despite the
>>> illusion of choice, it was not optional.
>>>
>>> There are ARM and PPC platforms with sparse RAM banks - leave compression
>>> active by default there.  OTOH, there are no known production x86
>>> systems with
>>> sparse RAM banks, so disable compression.  This decision can be
>>> revisited if
>>> such a platform comes along.
>>
>> (Process remarks rather than the code itself)
>>
>> Jan is away this week so I want to make sure this doesn't go in without
>> him having a say.
>>
>> While I don't particularly care about the approach taken for x86, Jan
>> voiced concerned with this approach and so far I didn't see any
>> conclusion. If there is any, then please point me to them.
>>
>> For the record, the objections from Jan are in [1]. If we want to ignore
>> them, then I think we need a vote. Possibly only from the x86 folks (?).
> 
> What do you think the 2 x86 maintainer tags on this patch in this exact
> form, following far too much wasted time already, represents.  The vote
> has already concluded.

In a reply separate from his R-b he also said "I would be fine in leaving
the option to be selected if ...", so I don't think you can count tags as
votes. As much as you apparently have a hard time seeing why I want the
option to remain available (despite knowing why I introduced PDX back at
the time), I'm having a hard time seeing why you want it unilaterally off
(and I'm afraid I haven't seen any reasoning beyond you simply not liking
that code, and you also not having liked my earlier attempts to overcome
the undue overhead).

Jan

On 06/10/2023 17:01, Andrew Cooper wrote:
> On 06/10/2023 4:09 pm, Julien Grall wrote:
>>
>>
>> On 06/10/2023 15:44, Andrew Cooper wrote:
>>> From: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>>>
>>> Adds a new compile-time flag to allow disabling PDX compression and
>>> compiles out compression-related code/data. It also shorts the pdx<->pfn
>>> conversion macros and creates stubs for masking functions.
>>>
>>> While at it, removes the old arch-defined CONFIG_HAS_PDX flag.
>>> Despite the
>>> illusion of choice, it was not optional.
>>>
>>> There are ARM and PPC platforms with sparse RAM banks - leave compression
>>> active by default there.  OTOH, there are no known production x86
>>> systems with
>>> sparse RAM banks, so disable compression.  This decision can be
>>> revisited if
>>> such a platform comes along.
>>
>> (Process remarks rather than the code itself)
>>
>> Jan is away this week so I want to make sure this doesn't go in without
>> him having a say.
>>
>> While I don't particularly care about the approach taken for x86, Jan
>> voiced concerned with this approach and so far I didn't see any
>> conclusion. If there is any, then please point me to them.
>>
>> For the record, the objections from Jan are in [1]. If we want to ignore
>> them, then I think we need a vote. Possibly only from the x86 folks (?).
> 
> 
> What do you think the 2 x86 maintainer tags on this patch in this exact
> form, 

Have you actually looked at the timeline before writing this e-mail?

15:44 -> You sent the series
16:09 -> I have asked for a vote/second review
16:18 -> Roger provided a second reviewed-by

 > The vote has already concluded.

Indeed. But this was only after my e-mail was sent. I would have replied 
differently if Roger had replied before hand.

I am merely trying to make sure we are following the process and don't 
get things committed without unnaddressed objections (you likely 
remember the series I am talking about...).

Next time, I suggest to check the timeline before implying that I am 
putting roadblocks.

Cheers,

-- 
Julien Grall

On Fri, Oct 06, 2023 at 04:09:19PM +0100, Julien Grall wrote:
> 
> 
> On 06/10/2023 15:44, Andrew Cooper wrote:
> > From: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> > 
> > Adds a new compile-time flag to allow disabling PDX compression and
> > compiles out compression-related code/data. It also shorts the pdx<->pfn
> > conversion macros and creates stubs for masking functions.
> > 
> > While at it, removes the old arch-defined CONFIG_HAS_PDX flag.  Despite the
> > illusion of choice, it was not optional.
> > 
> > There are ARM and PPC platforms with sparse RAM banks - leave compression
> > active by default there.  OTOH, there are no known production x86 systems with
> > sparse RAM banks, so disable compression.  This decision can be revisited if
> > such a platform comes along.
> 
> (Process remarks rather than the code itself)
> 
> Jan is away this week so I want to make sure this doesn't go in without him
> having a say.
> 
> While I don't particularly care about the approach taken for x86, Jan voiced
> concerned with this approach and so far I didn't see any conclusion. If
> there is any, then please point me to them.
> 
> For the record, the objections from Jan are in [1]. If we want to ignore
> them, then I think we need a vote. Possibly only from the x86 folks (?).

I would be fine in leaving the option to be selected if we knew that
such x86 systems might be feasible, but so far we have seen 0 x86
systems with sparse RAM.  That said, I don't have a strong opinion, but
the hiding on x86 seems fine to me.  Interested parties can always
forcefully select the option, and a case can be made to make it
available again on Kconfig.

I'm fine with waiting for Jan, in case he has more to add.

Thanks, Roger.

On 06.10.2023 17:27, Roger Pau Monné wrote:
> On Fri, Oct 06, 2023 at 04:09:19PM +0100, Julien Grall wrote:
>> On 06/10/2023 15:44, Andrew Cooper wrote:
>>> From: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>>>
>>> Adds a new compile-time flag to allow disabling PDX compression and
>>> compiles out compression-related code/data. It also shorts the pdx<->pfn
>>> conversion macros and creates stubs for masking functions.
>>>
>>> While at it, removes the old arch-defined CONFIG_HAS_PDX flag.  Despite the
>>> illusion of choice, it was not optional.
>>>
>>> There are ARM and PPC platforms with sparse RAM banks - leave compression
>>> active by default there.  OTOH, there are no known production x86 systems with
>>> sparse RAM banks, so disable compression.  This decision can be revisited if
>>> such a platform comes along.
>>
>> (Process remarks rather than the code itself)
>>
>> Jan is away this week so I want to make sure this doesn't go in without him
>> having a say.
>>
>> While I don't particularly care about the approach taken for x86, Jan voiced
>> concerned with this approach and so far I didn't see any conclusion. If
>> there is any, then please point me to them.
>>
>> For the record, the objections from Jan are in [1]. If we want to ignore
>> them, then I think we need a vote. Possibly only from the x86 folks (?).
> 
> I would be fine in leaving the option to be selected if we knew that
> such x86 systems might be feasible, but so far we have seen 0 x86
> systems with sparse RAM.  That said, I don't have a strong opinion, but
> the hiding on x86 seems fine to me.  Interested parties can always
> forcefully select the option, and a case can be made to make it
> available again on Kconfig.

I find it odd to demand people to change source code for aspects like
this. The very least I'd expect is that BIGMEM configurations (which
I've never seen any production use of) can actually also engage PDX.

Jan

On Mon, Oct 16, 2023 at 03:19:22PM +0200, Jan Beulich wrote:
> On 06.10.2023 17:27, Roger Pau Monné wrote:
> > On Fri, Oct 06, 2023 at 04:09:19PM +0100, Julien Grall wrote:
> >> On 06/10/2023 15:44, Andrew Cooper wrote:
> >>> From: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> >>>
> >>> Adds a new compile-time flag to allow disabling PDX compression and
> >>> compiles out compression-related code/data. It also shorts the pdx<->pfn
> >>> conversion macros and creates stubs for masking functions.
> >>>
> >>> While at it, removes the old arch-defined CONFIG_HAS_PDX flag.  Despite the
> >>> illusion of choice, it was not optional.
> >>>
> >>> There are ARM and PPC platforms with sparse RAM banks - leave compression
> >>> active by default there.  OTOH, there are no known production x86 systems with
> >>> sparse RAM banks, so disable compression.  This decision can be revisited if
> >>> such a platform comes along.
> >>
> >> (Process remarks rather than the code itself)
> >>
> >> Jan is away this week so I want to make sure this doesn't go in without him
> >> having a say.
> >>
> >> While I don't particularly care about the approach taken for x86, Jan voiced
> >> concerned with this approach and so far I didn't see any conclusion. If
> >> there is any, then please point me to them.
> >>
> >> For the record, the objections from Jan are in [1]. If we want to ignore
> >> them, then I think we need a vote. Possibly only from the x86 folks (?).
> > 
> > I would be fine in leaving the option to be selected if we knew that
> > such x86 systems might be feasible, but so far we have seen 0 x86
> > systems with sparse RAM.  That said, I don't have a strong opinion, but
> > the hiding on x86 seems fine to me.  Interested parties can always
> > forcefully select the option, and a case can be made to make it
> > available again on Kconfig.
> 
> I find it odd to demand people to change source code for aspects like
> this. The very least I'd expect is that BIGMEM configurations (which
> I've never seen any production use of) can actually also engage PDX.

So we expect BIGMEM to have sparse RAM regions?  I would have expected
systems with >16TB of RAM to still be contiguous.

Thanks, Roger.

On 16.10.2023 15:40, Roger Pau Monné wrote:
> On Mon, Oct 16, 2023 at 03:19:22PM +0200, Jan Beulich wrote:
>> On 06.10.2023 17:27, Roger Pau Monné wrote:
>>> On Fri, Oct 06, 2023 at 04:09:19PM +0100, Julien Grall wrote:
>>>> On 06/10/2023 15:44, Andrew Cooper wrote:
>>>>> From: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>>>>>
>>>>> Adds a new compile-time flag to allow disabling PDX compression and
>>>>> compiles out compression-related code/data. It also shorts the pdx<->pfn
>>>>> conversion macros and creates stubs for masking functions.
>>>>>
>>>>> While at it, removes the old arch-defined CONFIG_HAS_PDX flag.  Despite the
>>>>> illusion of choice, it was not optional.
>>>>>
>>>>> There are ARM and PPC platforms with sparse RAM banks - leave compression
>>>>> active by default there.  OTOH, there are no known production x86 systems with
>>>>> sparse RAM banks, so disable compression.  This decision can be revisited if
>>>>> such a platform comes along.
>>>>
>>>> (Process remarks rather than the code itself)
>>>>
>>>> Jan is away this week so I want to make sure this doesn't go in without him
>>>> having a say.
>>>>
>>>> While I don't particularly care about the approach taken for x86, Jan voiced
>>>> concerned with this approach and so far I didn't see any conclusion. If
>>>> there is any, then please point me to them.
>>>>
>>>> For the record, the objections from Jan are in [1]. If we want to ignore
>>>> them, then I think we need a vote. Possibly only from the x86 folks (?).
>>>
>>> I would be fine in leaving the option to be selected if we knew that
>>> such x86 systems might be feasible, but so far we have seen 0 x86
>>> systems with sparse RAM.  That said, I don't have a strong opinion, but
>>> the hiding on x86 seems fine to me.  Interested parties can always
>>> forcefully select the option, and a case can be made to make it
>>> available again on Kconfig.
>>
>> I find it odd to demand people to change source code for aspects like
>> this. The very least I'd expect is that BIGMEM configurations (which
>> I've never seen any production use of) can actually also engage PDX.
> 
> So we expect BIGMEM to have sparse RAM regions?  I would have expected
> systems with >16TB of RAM to still be contiguous.

Well, the system kind I did the original work for were sparse for the
purpose of allowing huge hotplug areas which would then be contiguous
with the non-hotplugged memory on the same nodes. That said, me
mentioning BIGMEM was merely yet another step in trying to find some
compromise with Andrew. As pointed out before I'd really expect that
finding compromises doesn't really mean only one side moves, yet here
and elsewhere I can't help getting the impression that this is what's
expected (of me).

Jan