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

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

[Guenter Roeck]

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.

Adding an explicit dependency on the existence of NR_CPUS fixes the
problem.

Fixes: 394290cba966 ("mm: turn USE_SPLIT_PTE_PTLOCKS / USE_SPLIT_PTE_PTLOCKS into Kconfig options")
Cc: David Hildenbrand <david@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 mm/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/Kconfig b/mm/Kconfig
index 09aebca1cae3..20fe60389cf5 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -595,7 +595,7 @@ config ARCH_MHP_MEMMAP_ON_MEMORY_ENABLE
 config SPLIT_PTE_PTLOCKS
 	def_bool y
 	depends on MMU
-	depends on NR_CPUS >= 4
+	depends on NR_CPUS && NR_CPUS >= 4
 	depends on !ARM || CPU_CACHE_VIPT
 	depends on !PARISC || PA20
 	depends on !SPARC32
-- 
2.45.2

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

[David Hildenbrand]

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.

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@redhat.com>

-- 
Cheers,

David / dhildenb

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

[Guenter Roeck]

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.
> 
> 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@redhat.com>
> 

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.

Guenter

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

[Guenter Roeck]

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.
>>
>> 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@redhat.com>
>>
> 
> 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.

Guenter

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

[Geert Uytterhoeven]

Hi Günter,

CC kbuild

I have two comments...

On Tue, Sep 24, 2024 at 1:52 AM Guenter Roeck <linux@roeck-us.net> 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@redhat.com>
> >>
> >
> > 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
but m68k)?  So the simple solution would be to add a dependency on
SMP to SPLIT_PTE_PTLOCKS.

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)?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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@roeck-us.net> 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@redhat.com>
>>>>
>>>
>>> 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

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

[Guenter Roeck]

On 9/24/24 00: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@roeck-us.net> 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)?
> 

I don't know. I thought that was intentional, though I don't understand
the logic. I didn't find a documentation that would explain how boolean
dependencies of integer objects are supposed to be handled. My expectation
that it would be similar to C was obviously wrong.

>>>> 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@redhat.com>
>>>>
>>>
>>> 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
> but m68k)?  So the simple solution would be to add a dependency on
> SMP to SPLIT_PTE_PTLOCKS.
> 
Makes sense. I'll send v3.

Guenter

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

[kernel test robot]

Hi Guenter,

kernel test robot noticed the following build errors:

[auto build test ERROR on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Guenter-Roeck/mm-Make-SPLIT_PTE_PTLOCKS-depend-on-the-existence-of-NR_CPUS/20240923-222628
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20240923142533.1197982-1-linux%40roeck-us.net
patch subject: [PATCH] mm: Make SPLIT_PTE_PTLOCKS depend on the existence of NR_CPUS
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20240924/202409240416.eEfELHN9-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240924/202409240416.eEfELHN9-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409240416.eEfELHN9-lkp@intel.com/

All errors (new ones prefixed by >>):

   arch/s390/mm/gmap.c: In function '__gmap_segment_gaddr':
>> arch/s390/mm/gmap.c:357:16: error: implicit declaration of function 'pmd_pgtable_page'; did you mean 'pmd_pgtable'? [-Wimplicit-function-declaration]
     357 |         page = pmd_pgtable_page((pmd_t *) entry);
         |                ^~~~~~~~~~~~~~~~
         |                pmd_pgtable
>> arch/s390/mm/gmap.c:357:14: error: assignment to 'struct page *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     357 |         page = pmd_pgtable_page((pmd_t *) entry);
         |              ^


vim +357 arch/s390/mm/gmap.c

1e133ab296f3ff Martin Schwidefsky 2016-03-08  343  
1e133ab296f3ff Martin Schwidefsky 2016-03-08  344  /**
1e133ab296f3ff Martin Schwidefsky 2016-03-08  345   * __gmap_segment_gaddr - find virtual address from segment pointer
1e133ab296f3ff Martin Schwidefsky 2016-03-08  346   * @entry: pointer to a segment table entry in the guest address space
1e133ab296f3ff Martin Schwidefsky 2016-03-08  347   *
1e133ab296f3ff Martin Schwidefsky 2016-03-08  348   * Returns the virtual address in the guest address space for the segment
1e133ab296f3ff Martin Schwidefsky 2016-03-08  349   */
1e133ab296f3ff Martin Schwidefsky 2016-03-08  350  static unsigned long __gmap_segment_gaddr(unsigned long *entry)
1e133ab296f3ff Martin Schwidefsky 2016-03-08  351  {
1e133ab296f3ff Martin Schwidefsky 2016-03-08  352  	struct page *page;
7e25de77bc5ea5 Anshuman Khandual  2022-11-25  353  	unsigned long offset;
1e133ab296f3ff Martin Schwidefsky 2016-03-08  354  
1e133ab296f3ff Martin Schwidefsky 2016-03-08  355  	offset = (unsigned long) entry / sizeof(unsigned long);
1e133ab296f3ff Martin Schwidefsky 2016-03-08  356  	offset = (offset & (PTRS_PER_PMD - 1)) * PMD_SIZE;
7e25de77bc5ea5 Anshuman Khandual  2022-11-25 @357  	page = pmd_pgtable_page((pmd_t *) entry);
1e133ab296f3ff Martin Schwidefsky 2016-03-08  358  	return page->index + offset;
1e133ab296f3ff Martin Schwidefsky 2016-03-08  359  }
1e133ab296f3ff Martin Schwidefsky 2016-03-08  360  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Guenter,

kernel test robot noticed the following build errors:

[auto build test ERROR on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Guenter-Roeck/mm-Make-SPLIT_PTE_PTLOCKS-depend-on-the-existence-of-NR_CPUS/20240923-222628
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20240923142533.1197982-1-linux%40roeck-us.net
patch subject: [PATCH] mm: Make SPLIT_PTE_PTLOCKS depend on the existence of NR_CPUS
config: s390-defconfig (https://download.01.org/0day-ci/archive/20240924/202409240546.SJwj9tUj-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 8663a75fa2f31299ab8d1d90288d9df92aadee88)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240924/202409240546.SJwj9tUj-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409240546.SJwj9tUj-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from arch/s390/mm/gmap.c:12:
   In file included from include/linux/pagewalk.h:5:
   In file included from include/linux/mm.h:2198:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
>> arch/s390/mm/gmap.c:357:9: error: call to undeclared function 'pmd_pgtable_page'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     357 |         page = pmd_pgtable_page((pmd_t *) entry);
         |                ^
>> arch/s390/mm/gmap.c:357:7: error: incompatible integer to pointer conversion assigning to 'struct page *' from 'int' [-Wint-conversion]
     357 |         page = pmd_pgtable_page((pmd_t *) entry);
         |              ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   4 warnings and 2 errors generated.


vim +/pmd_pgtable_page +357 arch/s390/mm/gmap.c

1e133ab296f3ff Martin Schwidefsky 2016-03-08  343  
1e133ab296f3ff Martin Schwidefsky 2016-03-08  344  /**
1e133ab296f3ff Martin Schwidefsky 2016-03-08  345   * __gmap_segment_gaddr - find virtual address from segment pointer
1e133ab296f3ff Martin Schwidefsky 2016-03-08  346   * @entry: pointer to a segment table entry in the guest address space
1e133ab296f3ff Martin Schwidefsky 2016-03-08  347   *
1e133ab296f3ff Martin Schwidefsky 2016-03-08  348   * Returns the virtual address in the guest address space for the segment
1e133ab296f3ff Martin Schwidefsky 2016-03-08  349   */
1e133ab296f3ff Martin Schwidefsky 2016-03-08  350  static unsigned long __gmap_segment_gaddr(unsigned long *entry)
1e133ab296f3ff Martin Schwidefsky 2016-03-08  351  {
1e133ab296f3ff Martin Schwidefsky 2016-03-08  352  	struct page *page;
7e25de77bc5ea5 Anshuman Khandual  2022-11-25  353  	unsigned long offset;
1e133ab296f3ff Martin Schwidefsky 2016-03-08  354  
1e133ab296f3ff Martin Schwidefsky 2016-03-08  355  	offset = (unsigned long) entry / sizeof(unsigned long);
1e133ab296f3ff Martin Schwidefsky 2016-03-08  356  	offset = (offset & (PTRS_PER_PMD - 1)) * PMD_SIZE;
7e25de77bc5ea5 Anshuman Khandual  2022-11-25 @357  	page = pmd_pgtable_page((pmd_t *) entry);
1e133ab296f3ff Martin Schwidefsky 2016-03-08  358  	return page->index + offset;
1e133ab296f3ff Martin Schwidefsky 2016-03-08  359  }
1e133ab296f3ff Martin Schwidefsky 2016-03-08  360  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki