linux-kernel - Re: [PATCH] Revert arm64: drop ranges in definition of ARCH_FORCE_MAX

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFbkSA3SzjWZ_Q8XC6-_Kzc+jmUN6sG7vzbSD5X1bRvPUaJg3Q@mail.gmail.com>
Date:   Sat, 29 Apr 2023 17:42:11 -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 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.
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.
>