Message-ID: <CAFbkSA0O-4YgNt-7KPhvx+vhvRNc38PO8E--GVAWKVgHK-_9Mw@mail.gmail.com>
Date:   Mon, 1 May 2023 16:24:38 -0500
From:   Justin Forbes <jforbes@...oraproject.org>
To:     Mike Rapoport <rppt@...nel.org>
Cc:     Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        jmforbes@...uxtx.org, Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] Revert arm64: drop ranges in definition of ARCH_FORCE_MAX_ORDER

On Sat, Apr 29, 2023 at 11:02 PM Mike Rapoport <rppt@...nel.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@...nel.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@...oraproject.org>
> > > > > Cc: Catalin Marinas <catalin.marinas@....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@....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.
>

Powered by blists - more mailing lists