Re: [PATCH] Revert arm64: drop ranges in definition of ARCH_FORCE_MAX_ORDER

[Mike Rapoport]

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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx>
> > > > Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> > > > ---
> > > >  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@xxxxxxx>
> > >
> > > For the record, the original discussion:
> > >
> > > Link: https://lore.kernel.org/r/CAFxkdAr5C7ggZ+WdvDbsfmwuXujT_z_x3qcUnhnCn-WrAurvgA@xxxxxxxxxxxxxx
> >
> > 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.
> >

-- 
Sincerely yours,
Mike.