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(-)
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
* Siddh Raman Pant <[email protected]> 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
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>
>
> 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
* Siddh Raman Pant <[email protected]> wrote:
> 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>
> >
> > 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
On Wed, 03 Aug 2022 14:18:18 +0530 Ingo Molnar <[email protected]> wrote:
>
> * Siddh Raman Pant <[email protected]> wrote:
>
> > 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>
> > >
> > > 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
* Siddh Raman Pant <[email protected]> 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
On Wed, 03 Aug 2022 14:38:19 +0530 Ingo Molnar <[email protected]> wrote:
>
> * Siddh Raman Pant <[email protected]> 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
* Siddh Raman Pant <[email protected]> wrote:
> On Wed, 03 Aug 2022 14:38:19 +0530 Ingo Molnar <[email protected]> wrote:
> >
> > * Siddh Raman Pant <[email protected]> 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
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) {
>
>
> > [...]
>
> Yeah - and I agree with your fix - will apply it.
>
> Thanks for your patience :-)
>
> Ingo
>
No worries, and thanks! :)
Siddh
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:
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 <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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);