[PATCH] x86/numa: Use cpumask_available instead of hardcoded NULL check

[PATCH] x86/numa: Use cpumask_available instead of hardcoded NULL check

[Siddh Raman Pant]

node_to_cpumask_map is of type cpumask_var_t[].

When CONFIG_CPUMASK_OFFSTACK is set, cpumask_var_t is typedef'd to a
pointer for dynamic allocation, else to an array of one element. The
"wicked game" can be checked on line 700 of include/linux/cpumask.h.

The lines changed in this commit were probably written by the original
authors with CONFIG_CPUMASK_OFFSTACK=y (i.e. dynamic allocation) in mind,
checking if the cpumask was available via a direct NULL check.

When CONFIG_CPUMASK_OFFSTACK is not set, GCC gives the below given warning
while compiling the kernel.

Fix that by using cpumask_available(), which does the NULL check when
CONFIG_CPUMASK_OFFSTACK is set, otherwise returns true. Use it wherever
such checks are made.

Conditional definitions of cpumask_available() can be found along with
the definition of cpumask_var_t. Check the cpumask.h reference mentioned
above.

GCC warning log:
===========================================================================

arch/x86/mm/numa.c: In function ‘cpumask_of_node’:
arch/x86/mm/numa.c:916:39: warning: the comparison will always evaluate as ‘false’ for the address of ‘node_to_cpumask_map’ will never be NULL [-Waddress]
  916 |         if (node_to_cpumask_map[node] == NULL) {
      |                                       ^~
In file included from ./include/linux/linkage.h:7,
                 from ./include/linux/kernel.h:17,
                 from ./arch/x86/include/asm/percpu.h:27,
                 from ./arch/x86/include/asm/current.h:6,
                 from ./include/linux/mutex.h:14,
                 from ./include/linux/kernfs.h:11,
                 from ./include/linux/sysfs.h:16,
                 from ./include/linux/kobject.h:20,
                 from ./include/linux/of.h:17,
                 from ./include/linux/irqdomain.h:35,
                 from ./include/linux/acpi.h:13,
                 from arch/x86/mm/numa.c:3:
arch/x86/mm/numa.c:67:15: note: ‘node_to_cpumask_map’ declared here
   67 | EXPORT_SYMBOL(node_to_cpumask_map);
      |               ^~~~~~~~~~~~~~~~~~~
./include/linux/export.h:87:28: note: in definition of macro ‘___EXPORT_SYMBOL’
   87 |         extern typeof(sym) sym;                                                 \
      |                            ^~~
./include/linux/export.h:147:41: note: in expansion of macro ‘__EXPORT_SYMBOL’
  147 | #define _EXPORT_SYMBOL(sym, sec)        __EXPORT_SYMBOL(sym, sec, "")
      |                                         ^~~~~~~~~~~~~~~
./include/linux/export.h:150:41: note: in expansion of macro ‘_EXPORT_SYMBOL’
  150 | #define EXPORT_SYMBOL(sym)              _EXPORT_SYMBOL(sym, "")
      |                                         ^~~~~~~~~~~~~~
arch/x86/mm/numa.c:67:1: note: in expansion of macro ‘EXPORT_SYMBOL’
   67 | EXPORT_SYMBOL(node_to_cpumask_map);
      | ^~~~~~~~~~~~~

===========================================================================

Fixes: c032ef60d1aa ("cpumask: convert node_to_cpumask_map[] to cpumask_var_t")
Fixes: de2d9445f162 ("x86: Unify node_to_cpumask_map handling between 32 and 64bit")

Signed-off-by: Siddh Raman Pant <code@siddh.me>
---
 arch/x86/mm/numa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
index e8b061557887..2aadb2019b4f 100644
--- a/arch/x86/mm/numa.c
+++ b/arch/x86/mm/numa.c
@@ -867,7 +867,7 @@ void debug_cpumask_set_cpu(int cpu, int node, bool enable)
 		return;
 	}
 	mask = node_to_cpumask_map[node];
-	if (!mask) {
+	if (!cpumask_available(mask)) {
 		pr_err("node_to_cpumask_map[%i] NULL\n", node);
 		dump_stack();
 		return;
@@ -913,7 +913,7 @@ const struct cpumask *cpumask_of_node(int node)
 		dump_stack();
 		return cpu_none_mask;
 	}
-	if (node_to_cpumask_map[node] == NULL) {
+	if (!cpumask_available(node_to_cpumask_map[node])) {
 		printk(KERN_WARNING
 			"cpumask_of_node(%d): no node_to_cpumask_map!\n",
 			node);
-- 
2.35.1

Re: [PATCH] x86/numa: Use cpumask_available instead of hardcoded NULL check

[Ingo Molnar]

* Siddh Raman Pant <code@siddh.me> wrote:

> GCC warning log:
> ===========================================================================
> 
> arch/x86/mm/numa.c: In function ‘cpumask_of_node’:
> arch/x86/mm/numa.c:916:39: warning: the comparison will always evaluate as ‘false’ for the address of ‘node_to_cpumask_map’ will never be NULL [-Waddress]
>   916 |         if (node_to_cpumask_map[node] == NULL) {
>       |                                       ^~

Your fix makes sense I suppose, but I'm wondering how testing didn't 
trigger this warning.

Off-stack isn't a rare config option:

  kepler:~/tip> make allmodconfig
  #
  # No change to .config
  #
  kepler:~/tip> grep CPUMASK_OFFSTACK .config
  CONFIG_CPUMASK_OFFSTACK=y
  kepler:~/tip> 

What am I missing?

> Fixes: c032ef60d1aa ("cpumask: convert node_to_cpumask_map[] to cpumask_var_t")
> Fixes: de2d9445f162 ("x86: Unify node_to_cpumask_map handling between 32 and 64bit")

These are ancient commits from 2009 & 2011.

Thanks,

	Ingo

Re: [PATCH] x86/numa: Use cpumask_available instead of hardcoded NULL check

[Siddh Raman Pant]

On Tue, 02 Aug 2022 16:37:44 +0530  Ingo Molnar <mingo@kernel.org> wrote:
> Your fix makes sense I suppose, but I'm wondering how testing didn't 
> trigger this warning.
> 
> Off-stack isn't a rare config option:
> 
>   kepler:~/tip> make allmodconfig
>   #
>   # No change to .config
>   #
>   kepler:~/tip> grep CPUMASK_OFFSTACK .config
>   CONFIG_CPUMASK_OFFSTACK=y
>   kepler:~/tip> 
> 
> What am I missing?

Maybe this triggers on certain config options set, or maybe due to new
gcc version? (I'm using gcc-12, I also likely saw while on gcc-11.)
It nevertheless is a helpful warning.

I just now tried `make defconfig` (default configuration based on
'x86_64_defconfig') and compiling with `make -j13 all`, and gcc doesn't
give any warning. (CONFIG_CPUMASK_OFFSTACK isn't even listed in the
.config file produced, grep fails.)

The config on which I can reproduce the warning can be found here:
https://gist.github.com/siddhpant/0197ea2b9873e8719d5d7ef991e2cd89
(It has 8969 lines, thus uploaded as a gist.)

This is a modification of a config found on syzkaller, which I was using
to compile and test some bug. I had noticed the gcc warning earlier while
on similar detours and usually ignored it, but now I finally took a look.

I tested compiling with it 5 times (`make clean` and `make -j13 all`),
and gcc gave the warning in all attempts. I also tried `make -j1 all`,
which also had gcc spitting out the warning, so it cannot be any race.

> > Fixes: c032ef60d1aa ("cpumask: convert node_to_cpumask_map[] to cpumask_var_t")
> > Fixes: de2d9445f162 ("x86: Unify node_to_cpumask_map handling between 32 and 64bit")
> 
> These are ancient commits from 2009 & 2011.

Yes, that's where blaming the file leads me to.

Thanks,
Siddh

Re: [PATCH] x86/numa: Use cpumask_available instead of hardcoded NULL check

[Ingo Molnar]

* Siddh Raman Pant <code@siddh.me> wrote:

> On Tue, 02 Aug 2022 16:37:44 +0530  Ingo Molnar <mingo@kernel.org> wrote:
> > Your fix makes sense I suppose, but I'm wondering how testing didn't 
> > trigger this warning.
> > 
> > Off-stack isn't a rare config option:
> > 
> >   kepler:~/tip> make allmodconfig
> >   #
> >   # No change to .config
> >   #
> >   kepler:~/tip> grep CPUMASK_OFFSTACK .config
> >   CONFIG_CPUMASK_OFFSTACK=y
> >   kepler:~/tip> 
> > 
> > What am I missing?
> 
> Maybe this triggers on certain config options set, or maybe due to new
> gcc version? (I'm using gcc-12, I also likely saw while on gcc-11.)
> It nevertheless is a helpful warning.
> 
> I just now tried `make defconfig` (default configuration based on
> 'x86_64_defconfig') and compiling with `make -j13 all`, and gcc doesn't
> give any warning. (CONFIG_CPUMASK_OFFSTACK isn't even listed in the
> .config file produced, grep fails.)

Does 'allmodconfig' reproduce the warning for you:

  $ make allmodconfig
  $ make arch/x86/mm/numa.o

?

If yes, then this could be due to gcc-12, as it doesn't reproduce with 
gcc-11 for me:

   gcc version 11.2.0 (Ubuntu 11.2.0-19ubuntu1) 

Thanks,

	Ingo

Re: [PATCH] x86/numa: Use cpumask_available instead of hardcoded NULL check

[Siddh Raman Pant]

On Wed, 03 Aug 2022 14:18:18 +0530  Ingo Molnar <mingo@kernel.org> wrote:
> 
> * Siddh Raman Pant <code@siddh.me> wrote:
> 
> > On Tue, 02 Aug 2022 16:37:44 +0530  Ingo Molnar <mingo@kernel.org> wrote:
> > > Your fix makes sense I suppose, but I'm wondering how testing didn't 
> > > trigger this warning.
> > > 
> > > Off-stack isn't a rare config option:
> > > 
> > >   kepler:~/tip> make allmodconfig
> > >   #
> > >   # No change to .config
> > >   #
> > >   kepler:~/tip> grep CPUMASK_OFFSTACK .config
> > >   CONFIG_CPUMASK_OFFSTACK=y
> > >   kepler:~/tip> 
> > > 
> > > What am I missing?
> > 
> > Maybe this triggers on certain config options set, or maybe due to new
> > gcc version? (I'm using gcc-12, I also likely saw while on gcc-11.)
> > It nevertheless is a helpful warning.
> > 
> > I just now tried `make defconfig` (default configuration based on
> > 'x86_64_defconfig') and compiling with `make -j13 all`, and gcc doesn't
> > give any warning. (CONFIG_CPUMASK_OFFSTACK isn't even listed in the
> > .config file produced, grep fails.)
> 
> Does 'allmodconfig' reproduce the warning for you:
> 
>   $ make allmodconfig
>   $ make arch/x86/mm/numa.o
> 
> ?
> 
> If yes, then this could be due to gcc-12, as it doesn't reproduce with 
> gcc-11 for me:
> 
>    gcc version 11.2.0 (Ubuntu 11.2.0-19ubuntu1) 
> 
> Thanks,
> 
>     Ingo
> 

There is no reason why allmodconfig would trigger the warning, as it has
CONFIG_CPUMASK_OFFSTACK=y, but the warning is because of the other case.
Still, I tried that, and as expected there was no warning.

Did you try the config file I had linked to earlier?

Thanks,
Siddh

Re: [PATCH] x86/numa: Use cpumask_available instead of hardcoded NULL check

[Ingo Molnar]

* Siddh Raman Pant <code@siddh.me> wrote:

> > Does 'allmodconfig' reproduce the warning for you:
> > 
> >   $ make allmodconfig
> >   $ make arch/x86/mm/numa.o
> > 
> > ?
> > 
> > If yes, then this could be due to gcc-12, as it doesn't reproduce with 
> > gcc-11 for me:
> > 
> >    gcc version 11.2.0 (Ubuntu 11.2.0-19ubuntu1) 
> > 
> > Thanks,
> > 
> >     Ingo
> > 
> 
> There is no reason why allmodconfig would trigger the warning, [...]

Well, unless I'm misreading your changelog, all the warning needs to 
trigger is CONFIG_CPUMASK_OFFSTACK=y.

> as it has CONFIG_CPUMASK_OFFSTACK=y, but the warning is because of the 
> other case.

What 'other case'? I've re-read the discussion and don't see it, but maybe 
I'm a bit daft this morning ...

> Did you try the config file I had linked to earlier?

Yes, and it didn't trigger the warning.

Thanks,

	Ingo

Re: [PATCH] x86/numa: Use cpumask_available instead of hardcoded NULL check

[Siddh Raman Pant]

On Wed, 03 Aug 2022 14:38:19 +0530  Ingo Molnar <mingo@kernel.org> wrote:
> 
> * Siddh Raman Pant <code@siddh.me> wrote:
> > There is no reason why allmodconfig would trigger the warning, [...]
> 
> Well, unless I'm misreading your changelog, all the warning needs to 
> trigger is CONFIG_CPUMASK_OFFSTACK=y.
> 
> > as it has CONFIG_CPUMASK_OFFSTACK=y, but the warning is because of the 
> > other case.
> 
> What 'other case'? I've re-read the discussion and don't see it, but maybe 
> I'm a bit daft this morning ...
 
No, the warning is happening because CONFIG_CPUMASK_OFFSTACK is not set. This
is because in that case cpumask_var_t type is not a pointer, and thus the var
can never be NULL, which leads gcc to warn us when comparing with NULL.

The chain of events are like:

        #ifdef CONFIG_CPUMASK_OFFSTACK
                typedef struct cpumask *cpumask_var_t;
        #else
                typedef struct cpumask cpumask_var_t[1];
        endif

        cpumask_var_t node_to_cpumask_map[MAX_NUMNODES];
        ...
        if (node_to_cpumask_map[node] == NULL) {


The fix works because:

        #ifdef CONFIG_CPUMASK_OFFSTACK
                static inline bool cpumask_available(cpumask_var_t mask)
                {
                        return mask != NULL;
                }
        #else
                static inline bool cpumask_available(cpumask_var_t mask)
                {
                        return true;
                }
        endif


The conditional definitions, as pointed out earlier, can be seen from line 700
of include/linux/cpumask.h file.

> > Did you try the config file I had linked to earlier?
> 
> Yes, and it didn't trigger the warning.
> 
> Thanks,
> 
>     Ingo
> 

 Thanks,
 Siddh

Re: [PATCH] x86/numa: Use cpumask_available instead of hardcoded NULL check

[Ingo Molnar]

* Siddh Raman Pant <code@siddh.me> wrote:

> On Wed, 03 Aug 2022 14:38:19 +0530  Ingo Molnar <mingo@kernel.org> wrote:
> > 
> > * Siddh Raman Pant <code@siddh.me> wrote:
> > > There is no reason why allmodconfig would trigger the warning, [...]
> > 
> > Well, unless I'm misreading your changelog, all the warning needs to 
> > trigger is CONFIG_CPUMASK_OFFSTACK=y.
> > 
> > > as it has CONFIG_CPUMASK_OFFSTACK=y, but the warning is because of the 
> > > other case.
> > 
> > What 'other case'? I've re-read the discussion and don't see it, but maybe 
> > I'm a bit daft this morning ...
>  
> No, the warning is happening because CONFIG_CPUMASK_OFFSTACK is not set.

Oh. So I was daft, as suspected. ;-)

And I can confirm that while gcc-11 doesn't trigger the warning, gcc-12 
does:

  $ make ARCH=x86_64 CC=gcc-11 arch/x86/mm/numa.o
    CC      arch/x86/mm/numa.o
  $

  $ rm -f arch/x86/mm/numa.o
  $
  $ make ARCH=x86_64 CC=gcc-12 arch/x86/mm/numa.o

    CC      arch/x86/mm/numa.o
  arch/x86/mm/numa.c: In function ‘cpumask_of_node’:
  arch/x86/mm/numa.c:916:39: error: the comparison will always evaluate as ‘false’ for the address of ‘node_to_cpumask_map’ will never be NULL [-Werror=address]
    916 |         if (node_to_cpumask_map[node] == NULL) {


> [...] This is because in that case cpumask_var_t type is not a pointer, 
> and thus the var can never be NULL, which leads gcc to warn us when 
> comparing with NULL.
> 
> The chain of events are like:
> 
>         #ifdef CONFIG_CPUMASK_OFFSTACK
>                 typedef struct cpumask *cpumask_var_t;
>         #else
>                 typedef struct cpumask cpumask_var_t[1];
>         endif
> 
>         cpumask_var_t node_to_cpumask_map[MAX_NUMNODES];
>         ...
>         if (node_to_cpumask_map[node] == NULL) {
> 
> 
> The fix works because:
> 
>         #ifdef CONFIG_CPUMASK_OFFSTACK
>                 static inline bool cpumask_available(cpumask_var_t mask)
>                 {
>                         return mask != NULL;
>                 }
>         #else
>                 static inline bool cpumask_available(cpumask_var_t mask)
>                 {
>                         return true;
>                 }
>         endif
> 
> 
> The conditional definitions, as pointed out earlier, can be seen from line 700
> of include/linux/cpumask.h file.

Yeah - and I agree with your fix - will apply it.

Thanks for your patience :-)

	Ingo

Re: [PATCH] x86/numa: Use cpumask_available instead of hardcoded NULL check

[Siddh Raman Pant]

On Wed, 03 Aug 2022 15:12:11 +0530  Ingo Molnar <mingo@kernel.org> wrote:
> Oh. So I was daft, as suspected. ;-)
> 
> And I can confirm that while gcc-11 doesn't trigger the warning, gcc-12 
> does:
> 
>   $ make ARCH=x86_64 CC=gcc-11 arch/x86/mm/numa.o
>     CC      arch/x86/mm/numa.o
>   $
> 
>   $ rm -f arch/x86/mm/numa.o
>   $
>   $ make ARCH=x86_64 CC=gcc-12 arch/x86/mm/numa.o
> 
>     CC      arch/x86/mm/numa.o
>   arch/x86/mm/numa.c: In function ‘cpumask_of_node’:
>   arch/x86/mm/numa.c:916:39: error: the comparison will always evaluate as ‘false’ for the address of ‘node_to_cpumask_map’ will never be NULL [-Werror=address]
>     916 |         if (node_to_cpumask_map[node] == NULL) {
> 
> 
> > [...]
> 
> Yeah - and I agree with your fix - will apply it.
> 
> Thanks for your patience :-)
> 
>     Ingo
> 

No worries, and thanks! :)
Siddh

[tip: x86/urgent] x86/numa: Use cpumask_available instead of hardcoded NULL check

[tip-bot2 for Siddh Raman Pant]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     625395c4a0f4775e0fe00f616888d2e6c1ba49db
Gitweb:        https://git.kernel.org/tip/625395c4a0f4775e0fe00f616888d2e6c1ba49db
Author:        Siddh Raman Pant <code@siddh.me>
AuthorDate:    Sun, 31 Jul 2022 21:39:13 +05:30
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Wed, 03 Aug 2022 11:44:57 +02:00

x86/numa: Use cpumask_available instead of hardcoded NULL check

GCC-12 started triggering a new warning:

  arch/x86/mm/numa.c: In function ‘cpumask_of_node’:
  arch/x86/mm/numa.c:916:39: warning: the comparison will always evaluate as ‘false’ for the address of ‘node_to_cpumask_map’ will never be NULL [-Waddress]
    916 |         if (node_to_cpumask_map[node] == NULL) {
        |                                       ^~

node_to_cpumask_map is of type cpumask_var_t[].

When CONFIG_CPUMASK_OFFSTACK is set, cpumask_var_t is typedef'd to a
pointer for dynamic allocation, else to an array of one element. The
"wicked game" can be checked on line 700 of include/linux/cpumask.h.

The original code in debug_cpumask_set_cpu() and cpumask_of_node() were
probably written by the original authors with CONFIG_CPUMASK_OFFSTACK=y
(i.e. dynamic allocation) in mind, checking if the cpumask was available
via a direct NULL check.

When CONFIG_CPUMASK_OFFSTACK is not set, GCC gives the above warning
while compiling the kernel.

Fix that by using cpumask_available(), which does the NULL check when
CONFIG_CPUMASK_OFFSTACK is set, otherwise returns true. Use it wherever
such checks are made.

Conditional definitions of cpumask_available() can be found along with
the definition of cpumask_var_t. Check the cpumask.h reference mentioned
above.

Fixes: c032ef60d1aa ("cpumask: convert node_to_cpumask_map[] to cpumask_var_t")
Fixes: de2d9445f162 ("x86: Unify node_to_cpumask_map handling between 32 and 64bit")
Signed-off-by: Siddh Raman Pant <code@siddh.me>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lore.kernel.org/r/20220731160913.632092-1-code@siddh.me
---
 arch/x86/mm/numa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
index e8b0615..2aadb20 100644
--- a/arch/x86/mm/numa.c
+++ b/arch/x86/mm/numa.c
@@ -867,7 +867,7 @@ void debug_cpumask_set_cpu(int cpu, int node, bool enable)
 		return;
 	}
 	mask = node_to_cpumask_map[node];
-	if (!mask) {
+	if (!cpumask_available(mask)) {
 		pr_err("node_to_cpumask_map[%i] NULL\n", node);
 		dump_stack();
 		return;
@@ -913,7 +913,7 @@ const struct cpumask *cpumask_of_node(int node)
 		dump_stack();
 		return cpu_none_mask;
 	}
-	if (node_to_cpumask_map[node] == NULL) {
+	if (!cpumask_available(node_to_cpumask_map[node])) {
 		printk(KERN_WARNING
 			"cpumask_of_node(%d): no node_to_cpumask_map!\n",
 			node);