x86/numa: Use cpumask_available instead of hardcoded NULL check

PATCH Applied Siddh Raman Pant 3 participants 10 messages Jul 31, 2022 – Aug 3, 2022

Subsystems (2)

x86 x86 MM

lore

Share & tools

lore

Siddh Raman Pant

author

2022-07-31 16:10 UTC #0 lore raw

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

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 <[email protected]>

---
 arch/x86/mm/numa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/mm/numa.c +2 -2

index e8b061557887..2aadb2019b4f 100644
@@ -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);

Signature · 7 lines

2.35.1


_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Ingo Molnar

2022-08-02 11:07 UTC #1 lore raw

Quoted 7 lines · Siddh Raman Pant

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?

Quoted 2 lines

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
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Siddh Raman Pant

author

2022-08-02 16:29 UTC #2 lore raw

Quoted 13 lines · On Tue, 02 Aug 2022 16:37:44 +0530 Ingo Molnar <[email protected]> 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>

Quoted 1 line

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.

Quoted 4 lines

Quoted 2 lines
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
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Ingo Molnar

2022-08-03 08:48 UTC #3 lore raw

Quoted 16 lines · Siddh Raman Pant

Quoted 13 lines · On Tue, 02 Aug 2022 16:37:44 +0530 Ingo Molnar <[email protected]> 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>
Quoted 1 line
What am I missing?

Quoted 8 lines

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
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Siddh Raman Pant

author

2022-08-03 08:58 UTC #4 lore raw

Quoted 35 lines · On Wed, 03 Aug 2022 14:18:18 +0530 Ingo Molnar <[email protected]> wrote:

Quoted 16 lines · Siddh Raman Pant
Quoted 13 lines · On Tue, 02 Aug 2022 16:37:44 +0530 Ingo Molnar <[email protected]> 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>
Quoted 1 line
What am I missing?
Quoted 8 lines
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
?

Quoted 9 lines

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
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Ingo Molnar

2022-08-03 09:08 UTC #5 lore raw

Quoted 10 lines · Siddh Raman Pant

Quoted 10 lines
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:

Quoted 8 lines

Quoted 6 lines
   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.

Quoted 2 lines

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 ...

Quoted 1 line

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

Yes, and it didn't trigger the warning.

Thanks,

Ingo
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Siddh Raman Pant

author

2022-08-03 09:22 UTC #6 lore raw

Quoted 4 lines · On Wed, 03 Aug 2022 14:38:19 +0530 Ingo Molnar <[email protected]> wrote:

Quoted 1 line · Siddh Raman Pant
There is no reason why allmodconfig would trigger the warning, [...]

Quoted 8 lines

Well, unless I'm misreading your changelog, all the warning needs to trigger is CONFIG_CPUMASK_OFFSTACK=y.
Quoted 2 lines
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.

Quoted 8 lines

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

Thanks,
Siddh
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Ingo Molnar

2022-08-03 09:42 UTC #7 lore raw

Quoted 8 lines · Siddh Raman Pant

Quoted 7 lines · On Wed, 03 Aug 2022 14:38:19 +0530 Ingo Molnar <[email protected]> wrote:
Quoted 1 line · Siddh Raman Pant
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.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
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) {

Quoted 32 lines

[...] 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

Quoted 2 lines

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

Siddh Raman Pant

author

2022-08-03 09:46 UTC #8 lore raw

Quoted 19 lines · On Wed, 03 Aug 2022 15:12:11 +0530 Ingo Molnar <[email protected]> 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) {

Quoted 8 lines

Quoted 1 line
[...]
Yeah - and I agree with your fix - will apply it.
Thanks for your patience :-)
Ingo

No worries, and thanks! :)
Siddh
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

tip-bot2 for Siddh Raman Pant

merge bot Applied

2022-08-03 15:42 UTC #9 lore raw

In reply to 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 <[email protected]>
AuthorDate:    Sun, 31 Jul 2022 21:39:13 +05:30
Committer:     Ingo Molnar <[email protected]>
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:

    916 |         if (node_to_cpumask_map[node] == NULL) {
        |                                       ^~

node_to_cpumask_map is of type cpumask_var_t[].

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 <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 arch/x86/mm/numa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/mm/numa.c +2 -2

index e8b0615..2aadb20 100644
@@ -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);