m68k: Define NR_CPUS

[PATCH v2] m68k: Define NR_CPUS

Posted by Guenter Roeck 1 year, 4 months ago

SPLIT_PTE_PTLOCKS depends on "NR_CPUS >= 4". Unfortunately, that evaluates
to true if there is no NR_CPUS configuration option. This results in
CONFIG_SPLIT_PTE_PTLOCKS=y for mac_defconfig. This in turn causes the m68k
"q800" and "virt" machines to crash in qemu if debugging options are
enabled.

Making CONFIG_SPLIT_PTE_PTLOCKS dependent on the existence of NR_CPUS
does not work since a dependency on the existence of a numeric Kconfig
entry always evaluates to false. Example:

config HAVE_NO_NR_CPUS
       def_bool y
       depends on !NR_CPUS

After adding this to a Kconfig file, "make defconfig" includes:
$ grep NR_CPUS .config
CONFIG_NR_CPUS=64
CONFIG_HAVE_NO_NR_CPUS=y

Define NR_CPUS for m68k instead to solve 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>
Acked-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Instead of trying to make SPLIT_PTE_PTLOCKS depend on the 
    existence of NR_CPUS, define NR_CPUS for m68k.

v1: https://lore.kernel.org/lkml/202409240546.SJwj9tUj-lkp@intel.com/T/#t

 arch/m68k/Kconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/m68k/Kconfig b/arch/m68k/Kconfig
index cc26df907bfe..53e4058d2e3c 100644
--- a/arch/m68k/Kconfig
+++ b/arch/m68k/Kconfig
@@ -76,6 +76,10 @@ config PGTABLE_LEVELS
 	default 2 if SUN3 || COLDFIRE
 	default 3
 
+config NR_CPUS
+	int
+	default "1"
+
 config MMU
 	bool "MMU-based Paged Memory Management Support"
 	default y
-- 
2.45.2

Re: [PATCH v2] m68k: Define NR_CPUS

Posted by Geert Uytterhoeven 1 year, 4 months ago

Hi Günter,

Thanks for your patch!

On Tue, Sep 24, 2024 at 1:56 AM Guenter Roeck <linux@roeck-us.net> wrote:
> SPLIT_PTE_PTLOCKS depends on "NR_CPUS >= 4". Unfortunately, that evaluates
> to true if there is no NR_CPUS configuration option. This results in
> CONFIG_SPLIT_PTE_PTLOCKS=y for mac_defconfig. This in turn causes the m68k
> "q800" and "virt" machines to crash in qemu if debugging options are
> enabled.
>
> Making CONFIG_SPLIT_PTE_PTLOCKS dependent on the existence of NR_CPUS
> does not work since a dependency on the existence of a numeric Kconfig
> entry always evaluates to false. Example:
>
> config HAVE_NO_NR_CPUS
>        def_bool y
>        depends on !NR_CPUS
>
> After adding this to a Kconfig file, "make defconfig" includes:
> $ grep NR_CPUS .config
> CONFIG_NR_CPUS=64
> CONFIG_HAVE_NO_NR_CPUS=y
>
> Define NR_CPUS for m68k instead to solve 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>
> Acked-by: David Hildenbrand <david@redhat.com>

I think it's a bit premature to add David's tag, as v2 is completely
different from v1.

> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Instead of trying to make SPLIT_PTE_PTLOCKS depend on the
>     existence of NR_CPUS, define NR_CPUS for m68k.
>
> v1: https://lore.kernel.org/lkml/202409240546.SJwj9tUj-lkp@intel.com/T/#t

I replied on v1 why I think this is not the right solution.

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 v2] m68k: Define NR_CPUS

Posted by Guenter Roeck 1 year, 4 months ago

On 9/24/24 00:47, Geert Uytterhoeven wrote:
> Hi Günter,
> 
> Thanks for your patch!
> 
> On Tue, Sep 24, 2024 at 1:56 AM Guenter Roeck <linux@roeck-us.net> wrote:
>> SPLIT_PTE_PTLOCKS depends on "NR_CPUS >= 4". Unfortunately, that evaluates
>> to true if there is no NR_CPUS configuration option. This results in
>> CONFIG_SPLIT_PTE_PTLOCKS=y for mac_defconfig. This in turn causes the m68k
>> "q800" and "virt" machines to crash in qemu if debugging options are
>> enabled.
>>
>> Making CONFIG_SPLIT_PTE_PTLOCKS dependent on the existence of NR_CPUS
>> does not work since a dependency on the existence of a numeric Kconfig
>> entry always evaluates to false. Example:
>>
>> config HAVE_NO_NR_CPUS
>>         def_bool y
>>         depends on !NR_CPUS
>>
>> After adding this to a Kconfig file, "make defconfig" includes:
>> $ grep NR_CPUS .config
>> CONFIG_NR_CPUS=64
>> CONFIG_HAVE_NO_NR_CPUS=y
>>
>> Define NR_CPUS for m68k instead to solve 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>
>> Acked-by: David Hildenbrand <david@redhat.com>
> 
> I think it's a bit premature to add David's tag, as v2 is completely
> different from v1.
> 

Ups, sorry, that was an unintentional carryover. Anyway, looks like it
may need v3 anyway.

Guenter

Re: [PATCH v2] m68k: Define NR_CPUS

Posted by David Hildenbrand 1 year, 4 months ago

On 24.09.24 01:56, Guenter Roeck wrote:
> SPLIT_PTE_PTLOCKS depends on "NR_CPUS >= 4". Unfortunately, that evaluates
> to true if there is no NR_CPUS configuration option. This results in
> CONFIG_SPLIT_PTE_PTLOCKS=y for mac_defconfig. This in turn causes the m68k
> "q800" and "virt" machines to crash in qemu if debugging options are
> enabled.
> 
> Making CONFIG_SPLIT_PTE_PTLOCKS dependent on the existence of NR_CPUS
> does not work since a dependency on the existence of a numeric Kconfig
> entry always evaluates to false. Example:
> 
> config HAVE_NO_NR_CPUS
>         def_bool y
>         depends on !NR_CPUS
> 
> After adding this to a Kconfig file, "make defconfig" includes:
> $ grep NR_CPUS .config
> CONFIG_NR_CPUS=64
> CONFIG_HAVE_NO_NR_CPUS=y
> 
> Define NR_CPUS for m68k instead to solve 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>
> Acked-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Instead of trying to make SPLIT_PTE_PTLOCKS depend on the
>      existence of NR_CPUS, define NR_CPUS for m68k.

Okay, looks like we're cleaning up CONFIG_NR_CPUS for good.

I'm back from conference travel tomorrow; I'll then throw in the following
into cross compilers and fixup any other arch that needs attention:


diff --git a/include/linux/threads.h b/include/linux/threads.h
index 1674a471b0b4..e31715e6746b 100644
--- a/include/linux/threads.h
+++ b/include/linux/threads.h
@@ -13,8 +13,7 @@
   * bit of memory.  Use nr_cpu_ids instead of this except for static bitmaps.
   */
  #ifndef CONFIG_NR_CPUS
-/* FIXME: This should be fixed in the arch's Kconfig */
-#define CONFIG_NR_CPUS 1
+#error "CONFIG_NR_CPUS not defined"
  #endif
  
  /* Places which use this should consider cpumask_var_t. */


-- 
Cheers,

David / dhildenb

Re: [PATCH v2] m68k: Define NR_CPUS

Posted by Geert Uytterhoeven 1 year, 4 months ago

Hi David,

On Tue, Sep 24, 2024 at 9:34 AM David Hildenbrand <david@redhat.com> wrote:
> On 24.09.24 01:56, Guenter Roeck wrote:
> > v2: Instead of trying to make SPLIT_PTE_PTLOCKS depend on the
> >      existence of NR_CPUS, define NR_CPUS for m68k.
>
> Okay, looks like we're cleaning up CONFIG_NR_CPUS for good.
>
> I'm back from conference travel tomorrow; I'll then throw in the following
> into cross compilers and fixup any other arch that needs attention:
>
>
> diff --git a/include/linux/threads.h b/include/linux/threads.h
> index 1674a471b0b4..e31715e6746b 100644
> --- a/include/linux/threads.h
> +++ b/include/linux/threads.h
> @@ -13,8 +13,7 @@
>    * bit of memory.  Use nr_cpu_ids instead of this except for static bitmaps.
>    */
>   #ifndef CONFIG_NR_CPUS
> -/* FIXME: This should be fixed in the arch's Kconfig */
> -#define CONFIG_NR_CPUS 1
> +#error "CONFIG_NR_CPUS not defined"
>   #endif
>
>   /* Places which use this should consider cpumask_var_t. */

This is gonna trigger on almost all architectures if CONFIG_SMP=n.

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 v2] m68k: Define NR_CPUS

Posted by Guenter Roeck 1 year, 4 months ago

On 9/24/24 00:48, Geert Uytterhoeven wrote:
> Hi David,
> 
> On Tue, Sep 24, 2024 at 9:34 AM David Hildenbrand <david@redhat.com> wrote:
>> On 24.09.24 01:56, Guenter Roeck wrote:
>>> v2: Instead of trying to make SPLIT_PTE_PTLOCKS depend on the
>>>       existence of NR_CPUS, define NR_CPUS for m68k.
>>
>> Okay, looks like we're cleaning up CONFIG_NR_CPUS for good.
>>
>> I'm back from conference travel tomorrow; I'll then throw in the following
>> into cross compilers and fixup any other arch that needs attention:
>>
>>
>> diff --git a/include/linux/threads.h b/include/linux/threads.h
>> index 1674a471b0b4..e31715e6746b 100644
>> --- a/include/linux/threads.h
>> +++ b/include/linux/threads.h
>> @@ -13,8 +13,7 @@
>>     * bit of memory.  Use nr_cpu_ids instead of this except for static bitmaps.
>>     */
>>    #ifndef CONFIG_NR_CPUS
>> -/* FIXME: This should be fixed in the arch's Kconfig */
>> -#define CONFIG_NR_CPUS 1
>> +#error "CONFIG_NR_CPUS not defined"
>>    #endif
>>
>>    /* Places which use this should consider cpumask_var_t. */
> 
> This is gonna trigger on almost all architectures if CONFIG_SMP=n.
> 

Guess that means that my patch won't work either. Any better ideas ?

Guenter

Re: [PATCH v2] m68k: Define NR_CPUS

Posted by David Hildenbrand 1 year, 4 months ago

On 24.09.24 16:04, Guenter Roeck wrote:
> On 9/24/24 00:48, Geert Uytterhoeven wrote:
>> Hi David,
>>
>> On Tue, Sep 24, 2024 at 9:34 AM David Hildenbrand <david@redhat.com> wrote:
>>> On 24.09.24 01:56, Guenter Roeck wrote:
>>>> v2: Instead of trying to make SPLIT_PTE_PTLOCKS depend on the
>>>>        existence of NR_CPUS, define NR_CPUS for m68k.
>>>
>>> Okay, looks like we're cleaning up CONFIG_NR_CPUS for good.
>>>
>>> I'm back from conference travel tomorrow; I'll then throw in the following
>>> into cross compilers and fixup any other arch that needs attention:
>>>
>>>
>>> diff --git a/include/linux/threads.h b/include/linux/threads.h
>>> index 1674a471b0b4..e31715e6746b 100644
>>> --- a/include/linux/threads.h
>>> +++ b/include/linux/threads.h
>>> @@ -13,8 +13,7 @@
>>>      * bit of memory.  Use nr_cpu_ids instead of this except for static bitmaps.
>>>      */
>>>     #ifndef CONFIG_NR_CPUS
>>> -/* FIXME: This should be fixed in the arch's Kconfig */
>>> -#define CONFIG_NR_CPUS 1
>>> +#error "CONFIG_NR_CPUS not defined"
>>>     #endif
>>>
>>>     /* Places which use this should consider cpumask_var_t. */
>>
>> This is gonna trigger on almost all architectures if CONFIG_SMP=n.
>>
> 
> Guess that means that my patch won't work either. Any better ideas ?

As discussed,

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


Might work for the time being.


-- 
Cheers,

David / dhildenb

Re: [PATCH v2] m68k: Define NR_CPUS

Posted by Maciej W. Rozycki 1 year, 4 months ago

On Tue, 24 Sep 2024, David Hildenbrand wrote:

> diff --git a/mm/Kconfig b/mm/Kconfig
> index 09aebca1cae3..4c9f5ea13271 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -595,6 +595,7 @@ config ARCH_MHP_MEMMAP_ON_MEMORY_ENABLE
>  config SPLIT_PTE_PTLOCKS
>         def_bool y
>         depends on MMU
> +       depends on SMP
>         depends on NR_CPUS >= 4

 I think it might be more intuitive if written as:

	depends on SMP && NR_CPUS >= 4

(with a note in the change description to the effect that NR_CPUS will 
have been unset and the condition won't work as expected unless SMP).

 FWIW,

  Maciej

Re: [PATCH v2] m68k: Define NR_CPUS

Posted by David Hildenbrand 1 year, 4 months ago

On 24.09.24 09:48, Geert Uytterhoeven wrote:
> Hi David,
> 
> On Tue, Sep 24, 2024 at 9:34 AM David Hildenbrand <david@redhat.com> wrote:
>> On 24.09.24 01:56, Guenter Roeck wrote:
>>> v2: Instead of trying to make SPLIT_PTE_PTLOCKS depend on the
>>>       existence of NR_CPUS, define NR_CPUS for m68k.
>>
>> Okay, looks like we're cleaning up CONFIG_NR_CPUS for good.
>>
>> I'm back from conference travel tomorrow; I'll then throw in the following
>> into cross compilers and fixup any other arch that needs attention:
>>
>>
>> diff --git a/include/linux/threads.h b/include/linux/threads.h
>> index 1674a471b0b4..e31715e6746b 100644
>> --- a/include/linux/threads.h
>> +++ b/include/linux/threads.h
>> @@ -13,8 +13,7 @@
>>     * bit of memory.  Use nr_cpu_ids instead of this except for static bitmaps.
>>     */
>>    #ifndef CONFIG_NR_CPUS
>> -/* FIXME: This should be fixed in the arch's Kconfig */
>> -#define CONFIG_NR_CPUS 1
>> +#error "CONFIG_NR_CPUS not defined"
>>    #endif
>>
>>    /* Places which use this should consider cpumask_var_t. */
> 
> This is gonna trigger on almost all architectures if CONFIG_SMP=n.

Right, probably it's not as easy as:

diff --git a/mm/Kconfig b/mm/Kconfig
index 09aebca1cae3..b9566312fc9c 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -581,6 +581,13 @@ endif # MEMORY_HOTPLUG
  config ARCH_MHP_MEMMAP_ON_MEMORY_ENABLE
         bool
  
+if !SMP
+
+config NR_CPUS
+       default 1
+
+endif # !SMP
+
  # Heavily threaded applications may benefit from splitting the mm-wide
  # page_table_lock, so that faults on different parts of the user address
  # space can be handled with less contention: split it at this NR_CPUS.


-- 
Cheers,

David / dhildenb

Re: [PATCH v2] m68k: Define NR_CPUS

Posted by David Hildenbrand 1 year, 4 months ago

On 24.09.24 10:07, David Hildenbrand wrote:
> On 24.09.24 09:48, Geert Uytterhoeven wrote:
>> Hi David,
>>
>> On Tue, Sep 24, 2024 at 9:34 AM David Hildenbrand <david@redhat.com> wrote:
>>> On 24.09.24 01:56, Guenter Roeck wrote:
>>>> v2: Instead of trying to make SPLIT_PTE_PTLOCKS depend on the
>>>>        existence of NR_CPUS, define NR_CPUS for m68k.
>>>
>>> Okay, looks like we're cleaning up CONFIG_NR_CPUS for good.
>>>
>>> I'm back from conference travel tomorrow; I'll then throw in the following
>>> into cross compilers and fixup any other arch that needs attention:
>>>
>>>
>>> diff --git a/include/linux/threads.h b/include/linux/threads.h
>>> index 1674a471b0b4..e31715e6746b 100644
>>> --- a/include/linux/threads.h
>>> +++ b/include/linux/threads.h
>>> @@ -13,8 +13,7 @@
>>>      * bit of memory.  Use nr_cpu_ids instead of this except for static bitmaps.
>>>      */
>>>     #ifndef CONFIG_NR_CPUS
>>> -/* FIXME: This should be fixed in the arch's Kconfig */
>>> -#define CONFIG_NR_CPUS 1
>>> +#error "CONFIG_NR_CPUS not defined"
>>>     #endif
>>>
>>>     /* Places which use this should consider cpumask_var_t. */
>>
>> This is gonna trigger on almost all architectures if CONFIG_SMP=n.
> 
> Right, probably it's not as easy as:

Yeah, as assumed, Kconfig behaves weird if a symbol is not around. Let 
me think about this the upcoming weeks. Making split locks depend on SMP 
should make it work for now.

-- 
Cheers,

David / dhildenb