diff mbox series

Revert arm64: drop ranges in definition of ARCH_FORCE_MAX_ORDER

Message ID 20230428153646.823736-1-jforbes@fedoraproject.org
State New
Headers
Series Revert arm64: drop ranges in definition of ARCH_FORCE_MAX_ORDER |

Commit Message

Justin M. Forbes April 28, 2023, 3:36 p.m. UTC 
  While the ARCH_FORCE_MAX_ORDER changes clarified the descriptions quite
a bit, the aarch64 specific change moved this config to sit behind
CONFIG_EXPERT. This becomes problematic when distros are setting this to
a non default value already. Pushing it behind EXPERT where it was not
before will silently change the configuration for users building with
oldconfig.  If distros patch out if EXPERT downstream, it still creates
problems for users testing out upstream patches, or trying to bisect to
find the root of problem, as the configuration will change unexpectedly,
possibly leading to different behavior and false results.

Whem I asked about reverting the EXPERT, dependency, I was asked to add
the ranges back.

This essentially reverts commit 34affcd7577a232803f729d1870ba475f294e4ea

Signed-off-by: Justin M. Forbes <jforbes@fedoraproject.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/Kconfig | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Catalin Marinas April 28, 2023, 5:01 p.m. UTC | #1 
+ Mike and Andrew

On Fri, Apr 28, 2023 at 10:36:45AM -0500, Justin M. Forbes wrote:
> While the ARCH_FORCE_MAX_ORDER changes clarified the descriptions quite
> a bit, the aarch64 specific change moved this config to sit behind
> CONFIG_EXPERT. This becomes problematic when distros are setting this to
> a non default value already. Pushing it behind EXPERT where it was not
> before will silently change the configuration for users building with
> oldconfig.  If distros patch out if EXPERT downstream, it still creates
> problems for users testing out upstream patches, or trying to bisect to
> find the root of problem, as the configuration will change unexpectedly,
> possibly leading to different behavior and false results.
> 
> Whem I asked about reverting the EXPERT, dependency, I was asked to add
> the ranges back.
> 
> This essentially reverts commit 34affcd7577a232803f729d1870ba475f294e4ea
> 
> Signed-off-by: Justin M. Forbes <jforbes@fedoraproject.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  arch/arm64/Kconfig | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index b1201d25a8a4..dae18ac01e94 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1516,9 +1516,11 @@ config XEN
>  # 16K |       27          |      14      |       13        |         11         |
>  # 64K |       29          |      16      |       13        |         13         |
>  config ARCH_FORCE_MAX_ORDER
> -	int "Order of maximal physically contiguous allocations" if EXPERT && (ARM64_4K_PAGES || ARM64_16K_PAGES)
> +	int "Order of maximal physically contiguous allocations" if ARM64_4K_PAGES || ARM64_16K_PAGES
>  	default "13" if ARM64_64K_PAGES
> +	range 11 13 if ARM64_16K_PAGES
>  	default "11" if ARM64_16K_PAGES
> +	range 10 15 if ARM64_4K_PAGES
>  	default "10"
>  	help
>  	  The kernel page allocator limits the size of maximal physically

The revert looks fine to me:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

For the record, the original discussion:

Link: https://lore.kernel.org/r/CAFxkdAr5C7ggZ+WdvDbsfmwuXujT_z_x3qcUnhnCn-WrAurvgA@mail.gmail.com
Mike Rapoport April 29, 2023, 7:01 p.m. UTC | #2 
On Fri, Apr 28, 2023 at 06:01:30PM +0100, Catalin Marinas wrote:
> + Mike and Andrew
> 
> On Fri, Apr 28, 2023 at 10:36:45AM -0500, Justin M. Forbes wrote:
> > While the ARCH_FORCE_MAX_ORDER changes clarified the descriptions quite
> > a bit, the aarch64 specific change moved this config to sit behind
> > CONFIG_EXPERT. This becomes problematic when distros are setting this to
> > a non default value already. Pushing it behind EXPERT where it was not
> > before will silently change the configuration for users building with
> > oldconfig.  If distros patch out if EXPERT downstream, it still creates
> > problems for users testing out upstream patches, or trying to bisect to
> > find the root of problem, as the configuration will change unexpectedly,
> > possibly leading to different behavior and false results.
> > 
> > Whem I asked about reverting the EXPERT, dependency, I was asked to add

Nit: When

> > the ranges back.
> > 
> > This essentially reverts commit 34affcd7577a232803f729d1870ba475f294e4ea
> > 
> > Signed-off-by: Justin M. Forbes <jforbes@fedoraproject.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > ---
> >  arch/arm64/Kconfig | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index b1201d25a8a4..dae18ac01e94 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -1516,9 +1516,11 @@ config XEN
> >  # 16K |       27          |      14      |       13        |         11         |
> >  # 64K |       29          |      16      |       13        |         13         |
> >  config ARCH_FORCE_MAX_ORDER
> > -	int "Order of maximal physically contiguous allocations" if EXPERT && (ARM64_4K_PAGES || ARM64_16K_PAGES)
> > +	int "Order of maximal physically contiguous allocations" if ARM64_4K_PAGES || ARM64_16K_PAGES
> >  	default "13" if ARM64_64K_PAGES
> > +	range 11 13 if ARM64_16K_PAGES
> >  	default "11" if ARM64_16K_PAGES
> > +	range 10 15 if ARM64_4K_PAGES
> >  	default "10"
> >  	help
> >  	  The kernel page allocator limits the size of maximal physically
> 
> The revert looks fine to me:
> 
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> 
> For the record, the original discussion:
> 
> Link: https://lore.kernel.org/r/CAFxkdAr5C7ggZ+WdvDbsfmwuXujT_z_x3qcUnhnCn-WrAurvgA@mail.gmail.com

I'm not really happy about this revert because MAX_ORDER is not something
that should be changed easily.
But since hiding it behind EXPERT would silently change lots of existing
builds, I won't object.

Still, I never got the answer _why_ Fedora/RHEL configs use non-default
value. Quite possible something else needs to be fixed rather than having
overgrown MAX_ORDER.
Justin M. Forbes April 29, 2023, 10:42 p.m. UTC | #3 
On Sat, Apr 29, 2023 at 2:01 PM Mike Rapoport <rppt@kernel.org> wrote:
>
> On Fri, Apr 28, 2023 at 06:01:30PM +0100, Catalin Marinas wrote:
> > + Mike and Andrew
> >
> > On Fri, Apr 28, 2023 at 10:36:45AM -0500, Justin M. Forbes wrote:
> > > While the ARCH_FORCE_MAX_ORDER changes clarified the descriptions quite
> > > a bit, the aarch64 specific change moved this config to sit behind
> > > CONFIG_EXPERT. This becomes problematic when distros are setting this to
> > > a non default value already. Pushing it behind EXPERT where it was not
> > > before will silently change the configuration for users building with
> > > oldconfig.  If distros patch out if EXPERT downstream, it still creates
> > > problems for users testing out upstream patches, or trying to bisect to
> > > find the root of problem, as the configuration will change unexpectedly,
> > > possibly leading to different behavior and false results.
> > >
> > > Whem I asked about reverting the EXPERT, dependency, I was asked to add
>
> Nit: When
>
> > > the ranges back.
> > >
> > > This essentially reverts commit 34affcd7577a232803f729d1870ba475f294e4ea
> > >
> > > Signed-off-by: Justin M. Forbes <jforbes@fedoraproject.org>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > ---
> > >  arch/arm64/Kconfig | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > > index b1201d25a8a4..dae18ac01e94 100644
> > > --- a/arch/arm64/Kconfig
> > > +++ b/arch/arm64/Kconfig
> > > @@ -1516,9 +1516,11 @@ config XEN
> > >  # 16K |       27          |      14      |       13        |         11         |
> > >  # 64K |       29          |      16      |       13        |         13         |
> > >  config ARCH_FORCE_MAX_ORDER
> > > -   int "Order of maximal physically contiguous allocations" if EXPERT && (ARM64_4K_PAGES || ARM64_16K_PAGES)
> > > +   int "Order of maximal physically contiguous allocations" if ARM64_4K_PAGES || ARM64_16K_PAGES
> > >     default "13" if ARM64_64K_PAGES
> > > +   range 11 13 if ARM64_16K_PAGES
> > >     default "11" if ARM64_16K_PAGES
> > > +   range 10 15 if ARM64_4K_PAGES
> > >     default "10"
> > >     help
> > >       The kernel page allocator limits the size of maximal physically
> >
> > The revert looks fine to me:
> >
> > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> >
> > For the record, the original discussion:
> >
> > Link: https://lore.kernel.org/r/CAFxkdAr5C7ggZ+WdvDbsfmwuXujT_z_x3qcUnhnCn-WrAurvgA@mail.gmail.com
>
> I'm not really happy about this revert because MAX_ORDER is not something
> that should be changed easily.
> But since hiding it behind EXPERT would silently change lots of existing
> builds, I won't object.
>
> Still, I never got the answer _why_ Fedora/RHEL configs use non-default
> value. Quite possible something else needs to be fixed rather than having
> overgrown MAX_ORDER.

I get that, but I also looked at the rest of the patch set. Nowhere
else was "if EXPERT" added.  Why wasn't it added to other
architectures? Not that I am complaining, but aarch64 in particular is
the one arch where, as a distro, we are trying to accommodate both
Raspberry Pi, and server class machines.
It is the practicality of building a single kernel image that works
along a large number of machines. The defaults are fine for smaller
boards, and honestly the majority of aarch64 hardware in circulation.
They are not acceptable for server class machines running those types
of workloads. For a long time, it was just Fedora running with 4K
pages, and carrying a patch to allow  MAX_ORDER to hit 13.  With RHEL
9, the 4K page size became the default there as well, and they did the
same. Eventually using a MAX_ORDER of 13 no longer required a patch of
any kind. In an ideal world, we would be building a 4k kernel, a 64k
kernel, and even a 16k kernel for users running Apple hardware, but
logistically that is a whole lot more than just a kernel rebuild. It
has QA cycles and support requirements that go along with it, etc.

Justin

> --
> Sincerely yours,
> Mike.
>
Mike Rapoport April 30, 2023, 3:54 a.m. UTC | #4 
On Sat, Apr 29, 2023 at 05:42:11PM -0500, Justin Forbes wrote:
> On Sat, Apr 29, 2023 at 2:01 PM Mike Rapoport <rppt@kernel.org> wrote:
> >
> > On Fri, Apr 28, 2023 at 06:01:30PM +0100, Catalin Marinas wrote:
> > > + Mike and Andrew
> > >
> > > On Fri, Apr 28, 2023 at 10:36:45AM -0500, Justin M. Forbes wrote:
> > > > While the ARCH_FORCE_MAX_ORDER changes clarified the descriptions quite
> > > > a bit, the aarch64 specific change moved this config to sit behind
> > > > CONFIG_EXPERT. This becomes problematic when distros are setting this to
> > > > a non default value already. Pushing it behind EXPERT where it was not
> > > > before will silently change the configuration for users building with
> > > > oldconfig.  If distros patch out if EXPERT downstream, it still creates
> > > > problems for users testing out upstream patches, or trying to bisect to
> > > > find the root of problem, as the configuration will change unexpectedly,
> > > > possibly leading to different behavior and false results.
> > > >
> > > > Whem I asked about reverting the EXPERT, dependency, I was asked to add
> >
> > Nit: When
> >
> > > > the ranges back.
> > > >
> > > > This essentially reverts commit 34affcd7577a232803f729d1870ba475f294e4ea
> > > >
> > > > Signed-off-by: Justin M. Forbes <jforbes@fedoraproject.org>
> > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > ---
> > > >  arch/arm64/Kconfig | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > > > index b1201d25a8a4..dae18ac01e94 100644
> > > > --- a/arch/arm64/Kconfig
> > > > +++ b/arch/arm64/Kconfig
> > > > @@ -1516,9 +1516,11 @@ config XEN
> > > >  # 16K |       27          |      14      |       13        |         11         |
> > > >  # 64K |       29          |      16      |       13        |         13         |
> > > >  config ARCH_FORCE_MAX_ORDER
> > > > -   int "Order of maximal physically contiguous allocations" if EXPERT && (ARM64_4K_PAGES || ARM64_16K_PAGES)
> > > > +   int "Order of maximal physically contiguous allocations" if ARM64_4K_PAGES || ARM64_16K_PAGES
> > > >     default "13" if ARM64_64K_PAGES
> > > > +   range 11 13 if ARM64_16K_PAGES
> > > >     default "11" if ARM64_16K_PAGES
> > > > +   range 10 15 if ARM64_4K_PAGES
> > > >     default "10"
> > > >     help
> > > >       The kernel page allocator limits the size of maximal physically
> > >
> > > The revert looks fine to me:
> > >
> > > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > >
> > > For the record, the original discussion:
> > >
> > > Link: https://lore.kernel.org/r/CAFxkdAr5C7ggZ+WdvDbsfmwuXujT_z_x3qcUnhnCn-WrAurvgA@mail.gmail.com
> >
> > I'm not really happy about this revert because MAX_ORDER is not something
> > that should be changed easily.
> > But since hiding it behind EXPERT would silently change lots of existing
> > builds, I won't object.
> >
> > Still, I never got the answer _why_ Fedora/RHEL configs use non-default
> > value. Quite possible something else needs to be fixed rather than having
> > overgrown MAX_ORDER.
> 
> I get that, but I also looked at the rest of the patch set. Nowhere
> else was "if EXPERT" added.  Why wasn't it added to other
> architectures? Not that I am complaining, but aarch64 in particular is
> the one arch where, as a distro, we are trying to accommodate both
> Raspberry Pi, and server class machines.

The patch was about dropping the ranges, not about adding EXPERT. So on
arm64 it was added because Catalin requested it, other arch maintainers
didn't.

> It is the practicality of building a single kernel image that works
> along a large number of machines. The defaults are fine for smaller
> boards, and honestly the majority of aarch64 hardware in circulation.
> They are not acceptable for server class machines running those types
> of workloads.

Why the default MAX_ORDER was not acceptable on arm64 server machines but
it is fine on, say, x86 and s390? 
I'm not asking how you made it possible in Fedora and RHEL, I'm asking why
did you switch from the default order at all.

> Justin
> 
> > --
> > Sincerely yours,
> > Mike.
> >
Justin M. Forbes May 1, 2023, 9:24 p.m. UTC | #5 
On Sat, Apr 29, 2023 at 11:02 PM Mike Rapoport <rppt@kernel.org> wrote:
>
> On Sat, Apr 29, 2023 at 05:42:11PM -0500, Justin Forbes wrote:
> > On Sat, Apr 29, 2023 at 2:01 PM Mike Rapoport <rppt@kernel.org> wrote:
> > >
> > > On Fri, Apr 28, 2023 at 06:01:30PM +0100, Catalin Marinas wrote:
> > > > + Mike and Andrew
> > > >
> > > > On Fri, Apr 28, 2023 at 10:36:45AM -0500, Justin M. Forbes wrote:
> > > > > While the ARCH_FORCE_MAX_ORDER changes clarified the descriptions quite
> > > > > a bit, the aarch64 specific change moved this config to sit behind
> > > > > CONFIG_EXPERT. This becomes problematic when distros are setting this to
> > > > > a non default value already. Pushing it behind EXPERT where it was not
> > > > > before will silently change the configuration for users building with
> > > > > oldconfig.  If distros patch out if EXPERT downstream, it still creates
> > > > > problems for users testing out upstream patches, or trying to bisect to
> > > > > find the root of problem, as the configuration will change unexpectedly,
> > > > > possibly leading to different behavior and false results.
> > > > >
> > > > > Whem I asked about reverting the EXPERT, dependency, I was asked to add
> > >
> > > Nit: When
> > >
> > > > > the ranges back.
> > > > >
> > > > > This essentially reverts commit 34affcd7577a232803f729d1870ba475f294e4ea
> > > > >
> > > > > Signed-off-by: Justin M. Forbes <jforbes@fedoraproject.org>
> > > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > > ---
> > > > >  arch/arm64/Kconfig | 4 +++-
> > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > > > > index b1201d25a8a4..dae18ac01e94 100644
> > > > > --- a/arch/arm64/Kconfig
> > > > > +++ b/arch/arm64/Kconfig
> > > > > @@ -1516,9 +1516,11 @@ config XEN
> > > > >  # 16K |       27          |      14      |       13        |         11         |
> > > > >  # 64K |       29          |      16      |       13        |         13         |
> > > > >  config ARCH_FORCE_MAX_ORDER
> > > > > -   int "Order of maximal physically contiguous allocations" if EXPERT && (ARM64_4K_PAGES || ARM64_16K_PAGES)
> > > > > +   int "Order of maximal physically contiguous allocations" if ARM64_4K_PAGES || ARM64_16K_PAGES
> > > > >     default "13" if ARM64_64K_PAGES
> > > > > +   range 11 13 if ARM64_16K_PAGES
> > > > >     default "11" if ARM64_16K_PAGES
> > > > > +   range 10 15 if ARM64_4K_PAGES
> > > > >     default "10"
> > > > >     help
> > > > >       The kernel page allocator limits the size of maximal physically
> > > >
> > > > The revert looks fine to me:
> > > >
> > > > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > > >
> > > > For the record, the original discussion:
> > > >
> > > > Link: https://lore.kernel.org/r/CAFxkdAr5C7ggZ+WdvDbsfmwuXujT_z_x3qcUnhnCn-WrAurvgA@mail.gmail.com
> > >
> > > I'm not really happy about this revert because MAX_ORDER is not something
> > > that should be changed easily.
> > > But since hiding it behind EXPERT would silently change lots of existing
> > > builds, I won't object.
> > >
> > > Still, I never got the answer _why_ Fedora/RHEL configs use non-default
> > > value. Quite possible something else needs to be fixed rather than having
> > > overgrown MAX_ORDER.
> >
> > I get that, but I also looked at the rest of the patch set. Nowhere
> > else was "if EXPERT" added.  Why wasn't it added to other
> > architectures? Not that I am complaining, but aarch64 in particular is
> > the one arch where, as a distro, we are trying to accommodate both
> > Raspberry Pi, and server class machines.
>
> The patch was about dropping the ranges, not about adding EXPERT. So on
> arm64 it was added because Catalin requested it, other arch maintainers
> didn't.
>
> > It is the practicality of building a single kernel image that works
> > along a large number of machines. The defaults are fine for smaller
> > boards, and honestly the majority of aarch64 hardware in circulation.
> > They are not acceptable for server class machines running those types
> > of workloads.
>
> Why the default MAX_ORDER was not acceptable on arm64 server machines but
> it is fine on, say, x86 and s390?
> I'm not asking how you made it possible in Fedora and RHEL, I'm asking why
> did you switch from the default order at all.

Because the MAX_ORDER on aarch64 with 4K pages is more tuned to the
needs of the average edge client, not so much those of a server class
machine.  And I get it, I would say well over 90% of the Fedora users
running aarch64 are indeed running on a rPi or similar with a small
memory footprint, and workloads which match that.  But we do support
and run a 4K page size aarch64 kernel on proper server class hardware,
running typical server workloads, and RHEL has a lot more users in the
server class than edge clients.   RHEL could probably default to 64K
pages, and most users would be happy with that. Fedora certainly could
not.  At some point, we may consider adding another build so that we
offer both 4K and 64K pages, but for now, this is where we are, and
where we have been for years.

Justin

> > Justin
> >
> > > --
> > > Sincerely yours,
> > > Mike.
> > >
>
> --
> Sincerely yours,
> Mike.
>
Catalin Marinas May 2, 2023, 2:07 p.m. UTC | #6 
On Mon, May 01, 2023 at 04:24:38PM -0500, Justin Forbes wrote:
> On Sat, Apr 29, 2023 at 11:02 PM Mike Rapoport <rppt@kernel.org> wrote:
> > Why the default MAX_ORDER was not acceptable on arm64 server machines but
> > it is fine on, say, x86 and s390?
> > I'm not asking how you made it possible in Fedora and RHEL, I'm asking why
> > did you switch from the default order at all.
> 
> Because the MAX_ORDER on aarch64 with 4K pages is more tuned to the
> needs of the average edge client, not so much those of a server class
> machine.  And I get it, I would say well over 90% of the Fedora users
> running aarch64 are indeed running on a rPi or similar with a small
> memory footprint, and workloads which match that.  But we do support
> and run a 4K page size aarch64 kernel on proper server class hardware,
> running typical server workloads, and RHEL has a lot more users in the
> server class than edge clients.   RHEL could probably default to 64K
> pages, and most users would be happy with that. Fedora certainly could
> not. 

I was talking to Marc Zyngier earlier and he reckons the need for a
higher MAX_ORDER is the GIC driver ITS allocation for Thunder-X. I'm
happy to make ARCH_MAX_ORDER higher in defconfig (12, 13?) if
CONFIG_ARCH_THUNDER. Mobile vendors won't enable this platform.

Regarding EXPERT, we could drop it and do like the other architectures
but we'll have randconfig occasionally hitting weird values that won't
build (like -1). Not sure EXPERT helps here.
Marc Zyngier May 2, 2023, 2:21 p.m. UTC | #7 
On Tue, 02 May 2023 15:07:41 +0100,
Catalin Marinas <catalin.marinas@arm.com> wrote:
> 
> On Mon, May 01, 2023 at 04:24:38PM -0500, Justin Forbes wrote:
> > On Sat, Apr 29, 2023 at 11:02 PM Mike Rapoport <rppt@kernel.org> wrote:
> > > Why the default MAX_ORDER was not acceptable on arm64 server machines but
> > > it is fine on, say, x86 and s390?
> > > I'm not asking how you made it possible in Fedora and RHEL, I'm asking why
> > > did you switch from the default order at all.
> > 
> > Because the MAX_ORDER on aarch64 with 4K pages is more tuned to the
> > needs of the average edge client, not so much those of a server class
> > machine.  And I get it, I would say well over 90% of the Fedora users
> > running aarch64 are indeed running on a rPi or similar with a small
> > memory footprint, and workloads which match that.  But we do support
> > and run a 4K page size aarch64 kernel on proper server class hardware,
> > running typical server workloads, and RHEL has a lot more users in the
> > server class than edge clients.   RHEL could probably default to 64K
> > pages, and most users would be happy with that. Fedora certainly could
> > not. 
> 
> I was talking to Marc Zyngier earlier and he reckons the need for a
> higher MAX_ORDER is the GIC driver ITS allocation for Thunder-X. I'm
> happy to make ARCH_MAX_ORDER higher in defconfig (12, 13?) if
> CONFIG_ARCH_THUNDER. Mobile vendors won't enable this platform.

In any case, I'd like to know exactly *what* requires it. The only
platform I know would benefit from this is the old TX1, but this
machine is more a boat anchor than a real server.

	M.
Mike Rapoport May 2, 2023, 4:12 p.m. UTC | #8 
On Tue, May 02, 2023 at 03:21:17PM +0100, Marc Zyngier wrote:
> On Tue, 02 May 2023 15:07:41 +0100,
> Catalin Marinas <catalin.marinas@arm.com> wrote:
> > 
> > On Mon, May 01, 2023 at 04:24:38PM -0500, Justin Forbes wrote:
> > > On Sat, Apr 29, 2023 at 11:02 PM Mike Rapoport <rppt@kernel.org> wrote:
> > > > Why the default MAX_ORDER was not acceptable on arm64 server machines but
> > > > it is fine on, say, x86 and s390?
> > > > I'm not asking how you made it possible in Fedora and RHEL, I'm asking why
> > > > did you switch from the default order at all.
> > > 
> > > Because the MAX_ORDER on aarch64 with 4K pages is more tuned to the
> > > needs of the average edge client, not so much those of a server class
> > > machine.  And I get it, I would say well over 90% of the Fedora users
> > > running aarch64 are indeed running on a rPi or similar with a small
> > > memory footprint, and workloads which match that.  But we do support
> > > and run a 4K page size aarch64 kernel on proper server class hardware,
> > > running typical server workloads, and RHEL has a lot more users in the
> > > server class than edge clients.   RHEL could probably default to 64K
> > > pages, and most users would be happy with that. Fedora certainly could
> > > not. 

The memory size of the machine or how heavy the workloads it runs have
nothing to do with MAX_ORDER. Again, x86 and s390 are perfectly fine with
MAX_ORDER == 10 ...

> > I was talking to Marc Zyngier earlier and he reckons the need for a
> > higher MAX_ORDER is the GIC driver ITS allocation for Thunder-X.

... but this indeed could be the reason to increase MAX_ORDER.

> > I'm happy to make ARCH_MAX_ORDER higher in defconfig (12, 13?) if
> > CONFIG_ARCH_THUNDER. Mobile vendors won't enable this platform.
> 
> In any case, I'd like to know exactly *what* requires it. The only
> platform I know would benefit from this is the old TX1, but this
> machine is more a boat anchor than a real server.

Yeah, if we'd knew what exactly requires such huge contiguous allocation,
we probably could fix that and leave Kconfig alone.
Mike Rapoport May 2, 2023, 4:15 p.m. UTC | #9 
On Tue, May 02, 2023 at 03:07:41PM +0100, Catalin Marinas wrote:
> On Mon, May 01, 2023 at 04:24:38PM -0500, Justin Forbes wrote:
> 
> Regarding EXPERT, we could drop it and do like the other architectures
> but we'll have randconfig occasionally hitting weird values that won't
> build (like -1). Not sure EXPERT helps here.

AFAIU, randconfig does not randomize int values, it's probably random
people that do ;-)
 
> -- 
> Catalin
Catalin Marinas May 2, 2023, 5:40 p.m. UTC | #10 
On Tue, May 02, 2023 at 07:15:20PM +0300, Mike Rapoport wrote:
> On Tue, May 02, 2023 at 03:07:41PM +0100, Catalin Marinas wrote:
> > On Mon, May 01, 2023 at 04:24:38PM -0500, Justin Forbes wrote:
> > 
> > Regarding EXPERT, we could drop it and do like the other architectures
> > but we'll have randconfig occasionally hitting weird values that won't
> > build (like -1). Not sure EXPERT helps here.
> 
> AFAIU, randconfig does not randomize int values, it's probably random
> people that do ;-)

https://lore.kernel.org/r/202303232149.Chh6KhiI-lkp@intel.com

with the randconfig here:

https://download.01.org/0day-ci/archive/20230323/202303232149.Chh6KhiI-lkp@intel.com/config

That said, it would fail on other architectures as well, maybe they are
just not wired up in the build machines.
Catalin Marinas May 3, 2023, 10:20 a.m. UTC | #11 
On Tue, May 02, 2023 at 06:40:14PM +0100, Catalin Marinas wrote:
> On Tue, May 02, 2023 at 07:15:20PM +0300, Mike Rapoport wrote:
> > On Tue, May 02, 2023 at 03:07:41PM +0100, Catalin Marinas wrote:
> > > On Mon, May 01, 2023 at 04:24:38PM -0500, Justin Forbes wrote:
> > > 
> > > Regarding EXPERT, we could drop it and do like the other architectures
> > > but we'll have randconfig occasionally hitting weird values that won't
> > > build (like -1). Not sure EXPERT helps here.
> > 
> > AFAIU, randconfig does not randomize int values, it's probably random
> > people that do ;-)
> 
> https://lore.kernel.org/r/202303232149.Chh6KhiI-lkp@intel.com
> 
> with the randconfig here:
> 
> https://download.01.org/0day-ci/archive/20230323/202303232149.Chh6KhiI-lkp@intel.com/config

You may be right, I can't get my randconfig to set ARCH_FORCE_MAX_ORDER
to anything other than the default. Maybe the kernel test robot has its
own config randomisation (cc'ing lkp@intel.com).

If we don't care about about this randconfig, I'm fine do drop EXPERT
from current mainline, together with the 4K/16K pages condition. The
condition only made sense if we kept the ranges in since these were
configurable (no range for 64K).

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index b1201d25a8a4..1867aba83ba3 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1516,7 +1516,7 @@ config XEN
 # 16K |       27          |      14      |       13        |         11         |
 # 64K |       29          |      16      |       13        |         13         |
 config ARCH_FORCE_MAX_ORDER
-	int "Order of maximal physically contiguous allocations" if EXPERT && (ARM64_4K_PAGES || ARM64_16K_PAGES)
+	int "Order of maximal physically contiguous allocations"
 	default "13" if ARM64_64K_PAGES
 	default "11" if ARM64_16K_PAGES
 	default "10"
Philip Li May 3, 2023, 12:08 p.m. UTC | #12 
On Wed, May 03, 2023 at 11:20:40AM +0100, Catalin Marinas wrote:
> On Tue, May 02, 2023 at 06:40:14PM +0100, Catalin Marinas wrote:
> > On Tue, May 02, 2023 at 07:15:20PM +0300, Mike Rapoport wrote:
> > > On Tue, May 02, 2023 at 03:07:41PM +0100, Catalin Marinas wrote:
> > > > On Mon, May 01, 2023 at 04:24:38PM -0500, Justin Forbes wrote:
> > > > 
> > > > Regarding EXPERT, we could drop it and do like the other architectures
> > > > but we'll have randconfig occasionally hitting weird values that won't
> > > > build (like -1). Not sure EXPERT helps here.
> > > 
> > > AFAIU, randconfig does not randomize int values, it's probably random
> > > people that do ;-)
> > 
> > https://lore.kernel.org/r/202303232149.Chh6KhiI-lkp@intel.com
> > 
> > with the randconfig here:
> > 
> > https://download.01.org/0day-ci/archive/20230323/202303232149.Chh6KhiI-lkp@intel.com/config
> 
> You may be right, I can't get my randconfig to set ARCH_FORCE_MAX_ORDER
> to anything other than the default. Maybe the kernel test robot has its
> own config randomisation (cc'ing lkp@intel.com).

Really appologize here, around Mar 23 time period, there's a bug in kernel test
robot code which wrongly set the value of ARCH_FORCE_MAX_ORDER to -1. We fixed
it after noticing this, and replied to the false positive reports we sent out.
But we missed to reply this report to point out it was invalid report.

Sorry about this again, pls ignore this report.

> 
> If we don't care about about this randconfig, I'm fine do drop EXPERT
> from current mainline, together with the 4K/16K pages condition. The
> condition only made sense if we kept the ranges in since these were
> configurable (no range for 64K).
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index b1201d25a8a4..1867aba83ba3 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1516,7 +1516,7 @@ config XEN
>  # 16K |       27          |      14      |       13        |         11         |
>  # 64K |       29          |      16      |       13        |         13         |
>  config ARCH_FORCE_MAX_ORDER
> -	int "Order of maximal physically contiguous allocations" if EXPERT && (ARM64_4K_PAGES || ARM64_16K_PAGES)
> +	int "Order of maximal physically contiguous allocations"
>  	default "13" if ARM64_64K_PAGES
>  	default "11" if ARM64_16K_PAGES
>  	default "10"
> 
> -- 
> Catalin
diff mbox series

Patch

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index b1201d25a8a4..dae18ac01e94 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1516,9 +1516,11 @@  config XEN
 # 16K |       27          |      14      |       13        |         11         |
 # 64K |       29          |      16      |       13        |         13         |
 config ARCH_FORCE_MAX_ORDER
-	int "Order of maximal physically contiguous allocations" if EXPERT && (ARM64_4K_PAGES || ARM64_16K_PAGES)
+	int "Order of maximal physically contiguous allocations" if ARM64_4K_PAGES || ARM64_16K_PAGES
 	default "13" if ARM64_64K_PAGES
+	range 11 13 if ARM64_16K_PAGES
 	default "11" if ARM64_16K_PAGES
+	range 10 15 if ARM64_4K_PAGES
 	default "10"
 	help
 	  The kernel page allocator limits the size of maximal physically