mm/slab: Fix undefined init_cache_node_node() for NUMA and !SMP

  sh/migor_defconfig:

    mm/slab.c: In function ‘slab_memory_callback’:
    mm/slab.c:1127:23: error: implicit declaration of function ‘init_cache_node_node’; did you mean ‘drain_cache_node_node’? [-Werror=implicit-function-declaration]
     1127 |                 ret = init_cache_node_node(nid);
	  |                       ^~~~~~~~~~~~~~~~~~~~
	  |                       drain_cache_node_node

The #ifdef condition protecting the definition of init_cache_node_node()
no longer matches the conditions protecting the (multiple) users.

Fix this by syncing the conditions.

Fixes: 76af6a054da40553 ("mm/migrate: add CPU hotplug to demotion #ifdef")
Reported-by: Randy Dunlap <rdunlap@infradead.org>
Link: https://lore.kernel.org/r/b5bdea22-ed2f-3187-6efe-0c72330270a4@infradead.org
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 mm/slab.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Hi Geert!

On Tue, 2023-03-21 at 09:30 +0100, Geert Uytterhoeven wrote:
> sh/migor_defconfig:
> 
>     mm/slab.c: In function ‘slab_memory_callback’:
>     mm/slab.c:1127:23: error: implicit declaration of function ‘init_cache_node_node’; did you mean ‘drain_cache_node_node’? [-Werror=implicit-function-declaration]
>      1127 |                 ret = init_cache_node_node(nid);
> 	  |                       ^~~~~~~~~~~~~~~~~~~~
> 	  |                       drain_cache_node_node
> 
> The #ifdef condition protecting the definition of init_cache_node_node()
> no longer matches the conditions protecting the (multiple) users.
> 
> Fix this by syncing the conditions.
> 
> Fixes: 76af6a054da40553 ("mm/migrate: add CPU hotplug to demotion #ifdef")
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Link: https://lore.kernel.org/r/b5bdea22-ed2f-3187-6efe-0c72330270a4@infradead.org
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  mm/slab.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index ba454246ee13dd4d..de1523a78f2e7367 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -839,7 +839,7 @@ static int init_cache_node(struct kmem_cache *cachep, int node, gfp_t gfp)
>  	return 0;
>  }
>  
> -#if (defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)) || defined(CONFIG_SMP)
> +#if defined(CONFIG_NUMA) || defined(CONFIG_SMP)
>  /*
>   * Allocates and initializes node for a node on each slab cache, used for
>   * either memory or cpu hotplug.  If memory is being hot-added, the kmem_cache_node

FWIW, the other #ifdef starting at drain_cache_node_node() closes with "#endif /* CONFIG_NUMA */",
while this #ifdef just ends with "#endif". Just in case you want to make this consistent.

Reviewed-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

Adrian
  

Hi Adrian,

On Tue, Mar 21, 2023 at 9:47 AM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> On Tue, 2023-03-21 at 09:30 +0100, Geert Uytterhoeven wrote:
> > sh/migor_defconfig:
> >
> >     mm/slab.c: In function ‘slab_memory_callback’:
> >     mm/slab.c:1127:23: error: implicit declaration of function ‘init_cache_node_node’; did you mean ‘drain_cache_node_node’? [-Werror=implicit-function-declaration]
> >      1127 |                 ret = init_cache_node_node(nid);
> >         |                       ^~~~~~~~~~~~~~~~~~~~
> >         |                       drain_cache_node_node
> >
> > The #ifdef condition protecting the definition of init_cache_node_node()
> > no longer matches the conditions protecting the (multiple) users.
> >
> > Fix this by syncing the conditions.
> >
> > Fixes: 76af6a054da40553 ("mm/migrate: add CPU hotplug to demotion #ifdef")
> > Reported-by: Randy Dunlap <rdunlap@infradead.org>
> > Link: https://lore.kernel.org/r/b5bdea22-ed2f-3187-6efe-0c72330270a4@infradead.org
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> >  mm/slab.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/slab.c b/mm/slab.c
> > index ba454246ee13dd4d..de1523a78f2e7367 100644
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -839,7 +839,7 @@ static int init_cache_node(struct kmem_cache *cachep, int node, gfp_t gfp)
> >       return 0;
> >  }
> >
> > -#if (defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)) || defined(CONFIG_SMP)
> > +#if defined(CONFIG_NUMA) || defined(CONFIG_SMP)
> >  /*
> >   * Allocates and initializes node for a node on each slab cache, used for
> >   * either memory or cpu hotplug.  If memory is being hot-added, the kmem_cache_node
>
> FWIW, the other #ifdef starting at drain_cache_node_node() closes with "#endif /* CONFIG_NUMA */",
> while this #ifdef just ends with "#endif". Just in case you want to make this consistent.

I guess that's fine, as init_cache_node_node() is a small function.
#endif comments are typically used when the start and end markers
do not fit on your (80x25 ;-) screen.

> Reviewed-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

Thanks!

Gr{oetje,eeting}s,

                        Geert
  

On 3/21/23 09:50, Geert Uytterhoeven wrote:
> Hi Adrian,
> 
> On Tue, Mar 21, 2023 at 9:47 AM John Paul Adrian Glaubitz
> <glaubitz@physik.fu-berlin.de> wrote:
>> On Tue, 2023-03-21 at 09:30 +0100, Geert Uytterhoeven wrote:
>> > sh/migor_defconfig:
>> >
>> >     mm/slab.c: In function ‘slab_memory_callback’:
>> >     mm/slab.c:1127:23: error: implicit declaration of function ‘init_cache_node_node’; did you mean ‘drain_cache_node_node’? [-Werror=implicit-function-declaration]
>> >      1127 |                 ret = init_cache_node_node(nid);
>> >         |                       ^~~~~~~~~~~~~~~~~~~~
>> >         |                       drain_cache_node_node
>> >
>> > The #ifdef condition protecting the definition of init_cache_node_node()
>> > no longer matches the conditions protecting the (multiple) users.
>> >
>> > Fix this by syncing the conditions.
>> >
>> > Fixes: 76af6a054da40553 ("mm/migrate: add CPU hotplug to demotion #ifdef")
>> > Reported-by: Randy Dunlap <rdunlap@infradead.org>
>> > Link: https://lore.kernel.org/r/b5bdea22-ed2f-3187-6efe-0c72330270a4@infradead.org
>> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> > ---
>> >  mm/slab.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/mm/slab.c b/mm/slab.c
>> > index ba454246ee13dd4d..de1523a78f2e7367 100644
>> > --- a/mm/slab.c
>> > +++ b/mm/slab.c
>> > @@ -839,7 +839,7 @@ static int init_cache_node(struct kmem_cache *cachep, int node, gfp_t gfp)
>> >       return 0;
>> >  }
>> >
>> > -#if (defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)) || defined(CONFIG_SMP)
>> > +#if defined(CONFIG_NUMA) || defined(CONFIG_SMP)
>> >  /*
>> >   * Allocates and initializes node for a node on each slab cache, used for
>> >   * either memory or cpu hotplug.  If memory is being hot-added, the kmem_cache_node
>>
>> FWIW, the other #ifdef starting at drain_cache_node_node() closes with "#endif /* CONFIG_NUMA */",
>> while this #ifdef just ends with "#endif". Just in case you want to make this consistent.
> 
> I guess that's fine, as init_cache_node_node() is a small function.
> #endif comments are typically used when the start and end markers
> do not fit on your (80x25 ;-) screen.

Agreed with this reasoning.

>> Reviewed-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> 
> Thanks!

Applied to slab/for-6.3-rc4/fixes, thanks!

> Gr{oetje,eeting}s,
> 
>                         Geert
>
  

On 3/21/23 01:30, Geert Uytterhoeven wrote:
> sh/migor_defconfig:
> 
>     mm/slab.c: In function ‘slab_memory_callback’:
>     mm/slab.c:1127:23: error: implicit declaration of function ‘init_cache_node_node’; did you mean ‘drain_cache_node_node’? [-Werror=implicit-function-declaration]
>      1127 |                 ret = init_cache_node_node(nid);
> 	  |                       ^~~~~~~~~~~~~~~~~~~~
> 	  |                       drain_cache_node_node
> 
> The #ifdef condition protecting the definition of init_cache_node_node()
> no longer matches the conditions protecting the (multiple) users.
> 
> Fix this by syncing the conditions.
> 
> Fixes: 76af6a054da40553 ("mm/migrate: add CPU hotplug to demotion #ifdef")
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Link: https://lore.kernel.org/r/b5bdea22-ed2f-3187-6efe-0c72330270a4@infradead.org
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Acked-by: Randy Dunlap <rdunlap@infradead.org>
Thanks.

> ---
>  mm/slab.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index ba454246ee13dd4d..de1523a78f2e7367 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -839,7 +839,7 @@ static int init_cache_node(struct kmem_cache *cachep, int node, gfp_t gfp)
>  	return 0;
>  }
>  
> -#if (defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)) || defined(CONFIG_SMP)
> +#if defined(CONFIG_NUMA) || defined(CONFIG_SMP)
>  /*
>   * Allocates and initializes node for a node on each slab cache, used for
>   * either memory or cpu hotplug.  If memory is being hot-added, the kmem_cache_node
  

On Tue, Mar 21, 2023 at 09:30:59AM +0100, Geert Uytterhoeven wrote:
> -#if (defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)) || defined(CONFIG_SMP)
> +#if defined(CONFIG_NUMA) || defined(CONFIG_SMP)

I'm amused by the thought of CONFIG_NUMA without CONFIG_SMP.
Is it possible to have one node with memory and a single CPU, then
another node with memory and no CPU?
  

On 3/21/23 09:40, Matthew Wilcox wrote:
> On Tue, Mar 21, 2023 at 09:30:59AM +0100, Geert Uytterhoeven wrote:
>> -#if (defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)) || defined(CONFIG_SMP)
>> +#if defined(CONFIG_NUMA) || defined(CONFIG_SMP)
> 
> I'm amused by the thought of CONFIG_NUMA without CONFIG_SMP.
> Is it possible to have one node with memory and a single CPU, then
> another node with memory and no CPU?

More likely 1 with CPU+memory, 1 with memory only.

I've been told that that are also I/O-only nodes.
  

On 3/21/23 09:40, Matthew Wilcox wrote:
> On Tue, Mar 21, 2023 at 09:30:59AM +0100, Geert Uytterhoeven wrote:
>> -#if (defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)) || defined(CONFIG_SMP)
>> +#if defined(CONFIG_NUMA) || defined(CONFIG_SMP)
> I'm amused by the thought of CONFIG_NUMA without CONFIG_SMP.
> Is it possible to have one node with memory and a single CPU, then
> another node with memory and no CPU?

It's _possible_ for sure, just unlikely.  The most likely place these
days is probably a teensy tiny VM that just happens to have some
performance-differentiated memory exposed to it for some reason.  Maybe
it's got a slice of slow PMEM or fast High-Bandwidth memory for whatever
reason.
  

On Wed, Mar 22, 2023 at 09:16:55AM -0700, Dave Hansen wrote:
> On 3/21/23 09:40, Matthew Wilcox wrote:
> > On Tue, Mar 21, 2023 at 09:30:59AM +0100, Geert Uytterhoeven wrote:
> >> -#if (defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)) || defined(CONFIG_SMP)
> >> +#if defined(CONFIG_NUMA) || defined(CONFIG_SMP)
> > I'm amused by the thought of CONFIG_NUMA without CONFIG_SMP.
> > Is it possible to have one node with memory and a single CPU, then
> > another node with memory and no CPU?
> 
> It's _possible_ for sure, just unlikely.  The most likely place these
> days is probably a teensy tiny VM that just happens to have some
> performance-differentiated memory exposed to it for some reason.  Maybe
> it's got a slice of slow PMEM or fast High-Bandwidth memory for whatever
> reason.

Right, you can construct such a system, but do we support the CONFIG
options of NUMA enabled and SMP disabled?  It seems so niche that we
shouldn't be spending time testing that combination.
  

On 3/22/23 09:46, Matthew Wilcox wrote:
> On Wed, Mar 22, 2023 at 09:16:55AM -0700, Dave Hansen wrote:
>> On 3/21/23 09:40, Matthew Wilcox wrote:
>>> On Tue, Mar 21, 2023 at 09:30:59AM +0100, Geert Uytterhoeven wrote:
>>>> -#if (defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)) || defined(CONFIG_SMP)
>>>> +#if defined(CONFIG_NUMA) || defined(CONFIG_SMP)
>>> I'm amused by the thought of CONFIG_NUMA without CONFIG_SMP.
>>> Is it possible to have one node with memory and a single CPU, then
>>> another node with memory and no CPU?
>> It's _possible_ for sure, just unlikely.  The most likely place these
>> days is probably a teensy tiny VM that just happens to have some
>> performance-differentiated memory exposed to it for some reason.  Maybe
>> it's got a slice of slow PMEM or fast High-Bandwidth memory for whatever
>> reason.
> Right, you can construct such a system, but do we support the CONFIG
> options of NUMA enabled and SMP disabled?  It seems so niche that we
> shouldn't be spending time testing that combination.

On x86 we don't:

> config NUMA
>         bool "NUMA Memory Allocation and Scheduler Support"
>         depends on SMP
>         depends on X86_64 || (X86_32 && HIGHMEM64G && X86_BIGSMP)

... which I think is fine.  I totally agree that NUMA without SMP is too
niche to care about.  Heck, !SMP is almost too niche to care about these
days.
  

Hi Matthew,

On Wed, Mar 22, 2023 at 5:47 PM Matthew Wilcox <willy@infradead.org> wrote:
> On Wed, Mar 22, 2023 at 09:16:55AM -0700, Dave Hansen wrote:
> > On 3/21/23 09:40, Matthew Wilcox wrote:
> > > On Tue, Mar 21, 2023 at 09:30:59AM +0100, Geert Uytterhoeven wrote:
> > >> -#if (defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)) || defined(CONFIG_SMP)
> > >> +#if defined(CONFIG_NUMA) || defined(CONFIG_SMP)
> > > I'm amused by the thought of CONFIG_NUMA without CONFIG_SMP.
> > > Is it possible to have one node with memory and a single CPU, then
> > > another node with memory and no CPU?
> >
> > It's _possible_ for sure, just unlikely.  The most likely place these
> > days is probably a teensy tiny VM that just happens to have some
> > performance-differentiated memory exposed to it for some reason.  Maybe
> > it's got a slice of slow PMEM or fast High-Bandwidth memory for whatever
> > reason.
>
> Right, you can construct such a system, but do we support the CONFIG
> options of NUMA enabled and SMP disabled?  It seems so niche that we
> shouldn't be spending time testing that combination.

SH has been using this for a long time.

It's supported. Dave just forgot to update the #ifdef around the
definition of init_cache_node_node() when updating an #ifdef around
a code block that contains one of the callers.

P.S. To me, this discussion reminds me of the old discussion about
     discontigmem without NUMA. Yes, not all systems are PCs with
     contiguous memory on a single fast bus ;-)

Gr{oetje,eeting}s,

                        Geert
  

Hi Geert!

On Thu, 2023-03-23 at 09:25 +0100, Geert Uytterhoeven wrote:
> It's supported. Dave just forgot to update the #ifdef around the
> definition of init_cache_node_node() when updating an #ifdef around
> a code block that contains one of the callers.
> 
> P.S. To me, this discussion reminds me of the old discussion about
>      discontigmem without NUMA. Yes, not all systems are PCs with
>      contiguous memory on a single fast bus ;-)

I'm wondering: Could the NUMA code be used to work with the different
memory types found on the Amiga, i.e. chip RAM, fast RAM etc?

Adrian
  

Hi Adrian,

On Thu, Mar 23, 2023 at 9:28 AM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> On Thu, 2023-03-23 at 09:25 +0100, Geert Uytterhoeven wrote:
> > It's supported. Dave just forgot to update the #ifdef around the
> > definition of init_cache_node_node() when updating an #ifdef around
> > a code block that contains one of the callers.
> >
> > P.S. To me, this discussion reminds me of the old discussion about
> >      discontigmem without NUMA. Yes, not all systems are PCs with
> >      contiguous memory on a single fast bus ;-)
>
> I'm wondering: Could the NUMA code be used to work with the different
> memory types found on the Amiga, i.e. chip RAM, fast RAM etc?

I guess so, but only for 32-bit motherboard RAM on A3000/A4000
vs. RAM on an accelerator card vs. Zorro-III RAM on e.g. BigRamPlus.
Chip RAM and Zorro-II RAM do not support RMW-cycles on
Zorro-III capable machines.

Gr{oetje,eeting}s,

                        Geert