Message ID | 20230717160318.2113-1-alejandro.vallejo@cloud.com (mailing list archive) |
---|---|
Headers | show |
Series | Make PDX compression optional | expand |
On 17.07.2023 18:03, 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. I disagree with this choice of default for x86. To avoid surprising downstreams, this should at best be a two-step process: Keep the default as Y right now, and switch to N a couple of releases later. But that's only the simple / mechanical side. Considering my earlier effort to reduce / remove the involved overhead dynamically at runtime (which you may or may not be aware of; see [2]), I view a compile time choice as less desirable. At the very least I would expect some justification towards this build time choice being acceptable / reasonable despite the earlier effort towards greater flexibility. Only such would be likely to have me merely defer to other x86 maintainers, rather than outright objecting. Jan [2] https://lists.xenproject.org/archives/html/xen-devel/2018-09/msg01616.html
On Tue, Jul 18, 2023 at 11:33:03AM +0200, Jan Beulich wrote: > On 17.07.2023 18:03, 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. > > I disagree with this choice of default for x86. To avoid surprising > downstreams, this should at best be a two-step process: Keep the > default as Y right now, and switch to N a couple of releases later. I'm not particularly worried about the default, as that's easy to change locally for our customers. That said, my .02 on the matter is that it would be easier to accept (leaving it as Y) if there was some documented case of the feature being triggered at present on x86. I'd rather turn it off unless we have strong evidence that it's actually used. Forcing Xen users to pay the cost of a feature they don't benefit from just feels wrong. > > But that's only the simple / mechanical side. Considering my earlier > effort to reduce / remove the involved overhead dynamically at > runtime (which you may or may not be aware of; see [2]), I wasn't. I'll have a look at the end of the day[3]. Regardless, most of this series is about containment of compression into known helpers and hopefully that should be undeniably positive irrespective of how the overhead reduction is achieved. > I view a > compile time choice as less desirable. At the very least I would > expect some justification towards this build time choice being > acceptable / reasonable despite the earlier effort towards greater > flexibility. Only such would be likely to have me merely defer to > other x86 maintainers, rather than outright objecting. > > Jan > > [2] https://lists.xenproject.org/archives/html/xen-devel/2018-09/msg01616.html I believe the burden of proof is reversed. Features bring complexity, and complexity increases the chances of introducing bugs. It's the presence of a feature that ought to be backed by a proof-of-requirement, not its absence. As far as data goes, we're aware of several ARM systems with compressible memory banks, but no publicly available x86 ones. I'm purposefully leaving RISC-V and PPC out of this equation, as they may or may not require this, it's something the maintainers of those ports will have to assess in due time. Alejandro [3] Note that I send this email before reading carefully your 2018 series.
On 18.07.2023 14:58, Alejandro Vallejo wrote: > I believe the burden of proof is reversed. Features bring complexity, and > complexity increases the chances of introducing bugs. It's the presence of > a feature that ought to be backed by a proof-of-requirement, not its > absence. The feature was introduced to support hardware a partner of ours was working on at the time. Xen wouldn't have worked very well there without these additions. It is beyond my control or knowledge whether any such system has ever made it into the public. So at the time of its introduction, the need for this code was well justified imo. Jan
On Tue, Jul 18, 2023 at 03:06:26PM +0200, Jan Beulich wrote: > On 18.07.2023 14:58, Alejandro Vallejo wrote: > > I believe the burden of proof is reversed. Features bring complexity, and > > complexity increases the chances of introducing bugs. It's the presence of > > a feature that ought to be backed by a proof-of-requirement, not its > > absence. > > The feature was introduced to support hardware a partner of ours was > working on at the time. Xen wouldn't have worked very well there > without these additions. It is beyond my control or knowledge whether > any such system has ever made it into the public. So at the time of > its introduction, the need for this code was well justified imo. > > Jan Oh, of course. I don't question the legitimacy of its introduction at all, nor do I question the matter of its optional presence. I do question the default considering the public data we have available. Alejandro
Hi Alejandro, Great work! On 17/07/2023 17:03, 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: Just as a datapoint. I applied this to a tree with Live-Update support. From the basic test I did, this is reducing the downtime by 10% :). Cheers,
On 20/07/2023 11:00 pm, Julien Grall wrote: > Hi Alejandro, > > Great work! > > On 17/07/2023 17:03, 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: > Just as a datapoint. I applied this to a tree with Live-Update > support. From the basic test I did, this is reducing the downtime by > 10% :). I'm not surprised in the slightest. We've had many cases that prove that compression (of 0 bits, on all x86 systems) is a disaster perf wise, and its used in pretty much every fastpath in Xen. Look no further than c/s 564d261687c and the 10% improvements in general PV runtime too, and that was optimising away one single instance in one single fastpath. It's also why I'm not entertaining the concept of leaving it active or selectable on x86. ~Andrew