Re: [PATCH] mm: Make SPLIT_PTE_PTLOCKS depend on the existence of NR_CPUS

[David Hildenbrand]

On 24.09.24 09:45, Geert Uytterhoeven wrote:
> Hi Günter,
>
> CC kbuild
>
> I have two comments...
>
> On Tue, Sep 24, 2024 at 1:52 AM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> > On 9/23/24 15:08, Guenter Roeck wrote:
> > > On 9/23/24 08:23, David Hildenbrand wrote:
> > > > On 23.09.24 16:25, Guenter Roeck wrote:
> > > > > SPLIT_PTE_PTLOCKS already depends on "NR_CPUS >= 4", but that evaluates
> > > > > to true if there is no NR_CPUS configuration option (such as for m68k).
> > > > > This results in CONFIG_SPLIT_PTE_PTLOCKS=y for mac_defconfig.
> > > > > This in turn causes the m68k "q800" machine to crash in qemu.
>
> Should this be fixed in Kconfig (too)?
>
> > > > Oh, that's why my compile tests still worked ... I even removed the additional NR_CPUS check, assuming it's not required ...
> > > >
> > > > Thanks for debugging and fixing!
> > > >
> > > > Acked-by: David Hildenbrand <david@xxxxxxxxxx>
> > >
> > > Apparently it wasn't that simple :-(. 0-day reports a build failure
> > > with s390 builds.
> > >
> > > arch/s390/mm/gmap.c:357:16: error: implicit declaration of function 'pmd_pgtable_page'.
> > >
> > > Turns out that
> > >       depends on NR_CPUS && NR_CPUS >= 4
> > >
> > > doesn't work and disables SPLIT_PTE_PTLOCKS even if NR_CPUS _is_ defined.
> > > I have no idea how to declare the dependency correctly.
> > > Sorry, I did not expect that.
> >
> > The only solution I found was to define NR_CPUS for m68k. That seems to be
> > the only architecture not defining it, so hopefully that is an acceptable
> > solution. I'll send v2 of the patch shortly.
>
> My first thought was to agree, as m68k is indeed the only architecture
> that does not define NR_CPUS. Upon closer look, most architectures
> have NR_CPUS depend on SMP, hence I assume the issue could happen for
> those too (although I didn't manage to create such a config on anything

I recall that I played the same thing, convincing me that having no
CONFIG_NR_CPUS on !SMP would actually do the right thing. Apparently it doesn't
for m68k at least.

> but m68k)?  So the simple solution would be to add a dependency on
> SMP to SPLIT_PTE_PTLOCKS.

That will probably work for now. CONFIG_NR_CPUS should be cleaned up at some point
to sort out the FIXME I commented in v2. Having kconfig set CONFIG_NR_CPUS=1 without
SMP would be easiest, but it's probably not that easy.

> BTW, the list of excluded architectures looks fragile to me:
>
>      config SPLIT_PTE_PTLOCKS
>              def_bool y
>              depends on MMU
>              depends on NR_CPUS >= 4
>              depends on !ARM || CPU_CACHE_VIPT
>              depends on !PARISC || PA20
>              depends on !SPARC32
>
> If this can't be handled in a generic way, perhaps this should be
> changed from opt-out to opt-in (i.e. select gate symbol in arch-specific
> Kconfig)?

Yes, as stated in my commit:

    More cleanups would be reasonable (like the arch-specific "depends on" for
    CONFIG_SPLIT_PTE_PTLOCKS), but we'll leave that for another day.

--
Cheers,

David / dhildenb