| Message ID | 20230717160318.2113-5-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Make PDX compression optional | expand |
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. If at all, it should only go in together with the later re-introduction of a CONFIG_*. Still I question whether that new CONFIG_HAS_PDX_COMPRESSION (which imo ought to be CONFIG_PDX_COMPRESSION) 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. Jan
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_*. Still I question > whether that new CONFIG_HAS_PDX_COMPRESSION (which imo ought to be > CONFIG_PDX_COMPRESSION) 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. ... 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 also don't see why this patch needs to come here in the series. The main refactoring in the following two patches looks to be independent. ~Andrew
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_*. Still I question >> whether that new CONFIG_HAS_PDX_COMPRESSION (which imo ought to be >> CONFIG_PDX_COMPRESSION) 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. > > ... 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
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
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index 439cc94f33..ea1949fbaa 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/x86/Kconfig b/xen/arch/x86/Kconfig index 92f3a627da..30df085d96 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/common/Kconfig b/xen/common/Kconfig index dd8d7c3f1c..40ec63c4b2 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -53,9 +53,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 46049eac35..0020cafb8a 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -29,7 +29,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/include/xen/pdx.h b/xen/include/xen/pdx.h index de5439a5e5..67ae20e89c 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; extern unsigned long pfn_pdx_bottom_mask, ma_va_bottom_mask; extern unsigned int pfn_pdx_hole_shift; @@ -171,7 +169,6 @@ static inline unsigned long pdx_to_pfn(unsigned long pdx) */ void pfn_pdx_hole_setup(unsigned long mask); -#endif /* HAS_PDX */ #endif /* __XEN_PDX_H__ */ /*
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> --- xen/arch/arm/Kconfig | 1 - xen/arch/x86/Kconfig | 1 - xen/common/Kconfig | 3 --- xen/common/Makefile | 2 +- xen/include/xen/pdx.h | 3 --- 5 files changed, 1 insertion(+), 9 deletions(-)