| Message ID | 20200603233203.1695403-2-keescook@chromium.org |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | Remove uninitialized_var() macro | expand |
Kees Cook <keescook@chromium.org> writes: > -#ifdef NODE_NOT_IN_PAGE_FLAGS > - pfn_align = node_map_pfn_alignment(); > - if (pfn_align && pfn_align < PAGES_PER_SECTION) { > - printk(KERN_WARNING "Node alignment %LuMB < min %LuMB, rejecting NUMA config\n", > - PFN_PHYS(pfn_align) >> 20, > - PFN_PHYS(PAGES_PER_SECTION) >> 20); > - return -EINVAL; > + if (IS_ENABLED(NODE_NOT_IN_PAGE_FLAGS)) { Hrm, clever ... > + unsigned long pfn_align = node_map_pfn_alignment(); > + > + if (pfn_align && pfn_align < PAGES_PER_SECTION) { > + pr_warn("Node alignment %LuMB < min %LuMB, rejecting NUMA config\n", > + PFN_PHYS(pfn_align) >> 20, > + PFN_PHYS(PAGES_PER_SECTION) >> 20); > + return -EINVAL; > + } > } > -#endif > if (!numa_meminfo_cover_memory(mi)) > return -EINVAL; > > diff --git a/include/linux/page-flags-layout.h b/include/linux/page-flags-layout.h > index 71283739ffd2..1a4cdec2bd29 100644 > --- a/include/linux/page-flags-layout.h > +++ b/include/linux/page-flags-layout.h > @@ -100,7 +100,7 @@ > * there. This includes the case where there is no node, so it is implicit. > */ > #if !(NODES_WIDTH > 0 || NODES_SHIFT == 0) > -#define NODE_NOT_IN_PAGE_FLAGS > +#define NODE_NOT_IN_PAGE_FLAGS 1 but if we ever lose the 1 then the above will silently compile the code within the IS_ENABLED() section out. Thanks, tglx
On Thu, Jun 4, 2020 at 9:58 AM Thomas Gleixner <tglx@linutronix.de> wrote: > > but if we ever lose the 1 then the above will silently compile the code > within the IS_ENABLED() section out. Yeah, I believe `IS_ENABLED()` is only meant for Kconfig symbols, not macro defs in general. A better option would be `__is_defined()` which works for defined-to-nothing too. Cheers, Miguel
On Thu, Jun 04, 2020 at 09:58:07AM +0200, Thomas Gleixner wrote: > Kees Cook <keescook@chromium.org> writes: > > -#ifdef NODE_NOT_IN_PAGE_FLAGS > > - pfn_align = node_map_pfn_alignment(); > > - if (pfn_align && pfn_align < PAGES_PER_SECTION) { > > - printk(KERN_WARNING "Node alignment %LuMB < min %LuMB, rejecting NUMA config\n", > > - PFN_PHYS(pfn_align) >> 20, > > - PFN_PHYS(PAGES_PER_SECTION) >> 20); > > - return -EINVAL; > > + if (IS_ENABLED(NODE_NOT_IN_PAGE_FLAGS)) { > > Hrm, clever ... > > > + unsigned long pfn_align = node_map_pfn_alignment(); > > + > > + if (pfn_align && pfn_align < PAGES_PER_SECTION) { > > + pr_warn("Node alignment %LuMB < min %LuMB, rejecting NUMA config\n", > > + PFN_PHYS(pfn_align) >> 20, > > + PFN_PHYS(PAGES_PER_SECTION) >> 20); > > + return -EINVAL; > > + } > > } > > -#endif > > if (!numa_meminfo_cover_memory(mi)) > > return -EINVAL; > > > > diff --git a/include/linux/page-flags-layout.h b/include/linux/page-flags-layout.h > > index 71283739ffd2..1a4cdec2bd29 100644 > > --- a/include/linux/page-flags-layout.h > > +++ b/include/linux/page-flags-layout.h > > @@ -100,7 +100,7 @@ > > * there. This includes the case where there is no node, so it is implicit. > > */ > > #if !(NODES_WIDTH > 0 || NODES_SHIFT == 0) > > -#define NODE_NOT_IN_PAGE_FLAGS > > +#define NODE_NOT_IN_PAGE_FLAGS 1 > > but if we ever lose the 1 then the above will silently compile the code > within the IS_ENABLED() section out. That's true, yes. I considered two other ways to do this: 1) smallest patch, but more #ifdef: diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c index 59ba008504dc..fbf5231a3d35 100644 --- a/arch/x86/mm/numa.c +++ b/arch/x86/mm/numa.c @@ -541,7 +541,9 @@ static void __init numa_clear_kernel_node_hotplug(void) static int __init numa_register_memblks(struct numa_meminfo *mi) { - unsigned long uninitialized_var(pfn_align); +#ifdef NODE_NOT_IN_PAGE_FLAGS + unsigned long pfn_align; +#endif int i, nid; /* Account for nodes with cpus and no memory */ 2) medium size, weird style: diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c index 59ba008504dc..0df7ba9b21b2 100644 --- a/arch/x86/mm/numa.c +++ b/arch/x86/mm/numa.c @@ -541,7 +541,6 @@ static void __init numa_clear_kernel_node_hotplug(void) static int __init numa_register_memblks(struct numa_meminfo *mi) { - unsigned long uninitialized_var(pfn_align); int i, nid; /* Account for nodes with cpus and no memory */ @@ -570,12 +569,15 @@ static int __init numa_register_memblks(struct numa_meminfo *mi) * whether its granularity is fine enough. */ #ifdef NODE_NOT_IN_PAGE_FLAGS - pfn_align = node_map_pfn_alignment(); - if (pfn_align && pfn_align < PAGES_PER_SECTION) { - printk(KERN_WARNING "Node alignment %LuMB < min %LuMB, rejecting NUMA config\n", - PFN_PHYS(pfn_align) >> 20, - PFN_PHYS(PAGES_PER_SECTION) >> 20); - return -EINVAL; + { + unsigned long pfn_align = node_map_pfn_alignment(); + + if (pfn_align && pfn_align < PAGES_PER_SECTION) { + pr_warn("Node alignment %LuMB < min %LuMB, rejecting NUMA config\n", + PFN_PHYS(pfn_align) >> 20, + PFN_PHYS(PAGES_PER_SECTION) >> 20); + return -EINVAL; + } } #endif if (!numa_meminfo_cover_memory(mi)) and 3 is what I sent: biggest, but removes #ifdef Any preference? Thanks!
On Thu, Jun 04, 2020 at 01:41:07PM +0200, Miguel Ojeda wrote: > On Thu, Jun 4, 2020 at 9:58 AM Thomas Gleixner <tglx@linutronix.de> wrote: > > > > but if we ever lose the 1 then the above will silently compile the code > > within the IS_ENABLED() section out. > > Yeah, I believe `IS_ENABLED()` is only meant for Kconfig symbols, not > macro defs in general. A better option would be `__is_defined()` which > works for defined-to-nothing too. Er? That's not what it looked like to me: #define IS_BUILTIN(option) __is_defined(option) #define IS_ENABLED(option) __or(IS_BUILTIN(option), IS_MODULE(option)) But just to be sure, I just tested in with a real build: [ 3.242160] IS_ENABLED(TEST_UNDEF) false [ 3.242691] __is_defined(TEST_UNDEF) false [ 3.243240] IS_ENABLED(TEST_VALUE_EMPTY) false [ 3.243794] __is_defined(TEST_VALUE_EMPTY) false [ 3.244353] IS_ENABLED(TEST_VALUE_1) true [ 3.244848] __is_defined(TEST_VALUE_1) true and nope, it only works with a defined value present. diff --git a/init/main.c b/init/main.c index 03371976d387..378a9e54b6dc 100644 --- a/init/main.c +++ b/init/main.c @@ -1406,6 +1406,34 @@ static int __ref kernel_init(void *unused) */ pti_finalize(); +#undef TEST_UNDEF + if (IS_ENABLED(TEST_UNDEF)) + pr_info("IS_ENABLED(TEST_UNDEF) true\n"); + else + pr_info("IS_ENABLED(TEST_UNDEF) false\n"); + if (__is_defined(TEST_UNDEF)) + pr_info("__is_defined(TEST_UNDEF) true\n"); + else + pr_info("__is_defined(TEST_UNDEF) false\n"); +#define TEST_VALUE_EMPTY + if (IS_ENABLED(TEST_VALUE_EMPTY)) + pr_info("IS_ENABLED(TEST_VALUE_EMPTY) true\n"); + else + pr_info("IS_ENABLED(TEST_VALUE_EMPTY) false\n"); + if (__is_defined(TEST_VALUE_EMPTY)) + pr_info("__is_defined(TEST_VALUE_EMPTY) true\n"); + else + pr_info("__is_defined(TEST_VALUE_EMPTY) false\n"); +#define TEST_VALUE_1 1 + if (IS_ENABLED(TEST_VALUE_1)) + pr_info("IS_ENABLED(TEST_VALUE_1) true\n"); + else + pr_info("IS_ENABLED(TEST_VALUE_1) false\n"); + if (__is_defined(TEST_VALUE_1)) + pr_info("__is_defined(TEST_VALUE_1) true\n"); + else + pr_info("__is_defined(TEST_VALUE_1) false\n"); + system_state = SYSTEM_RUNNING; numa_default_policy(); which means a few other __is_defined() users are not correct too...
On Thu, Jun 4, 2020 at 4:56 PM Kees Cook <keescook@chromium.org> wrote: > > Er? That's not what it looked like to me: > > #define IS_BUILTIN(option) __is_defined(option) > #define IS_ENABLED(option) __or(IS_BUILTIN(option), IS_MODULE(option)) > > But just to be sure, I just tested in with a real build: > > [ 3.242160] IS_ENABLED(TEST_UNDEF) false > [ 3.242691] __is_defined(TEST_UNDEF) false > [ 3.243240] IS_ENABLED(TEST_VALUE_EMPTY) false > [ 3.243794] __is_defined(TEST_VALUE_EMPTY) false > [ 3.244353] IS_ENABLED(TEST_VALUE_1) true > [ 3.244848] __is_defined(TEST_VALUE_1) true > > and nope, it only works with a defined value present. You are right, it follows the Kconfig logic, returning false for defined-but-to-0 too. We should probably add an `IS_DEFINED()` macro kernel-wide for this (and add it to the `coding-guidelines.rst` since `IS_ENABLED()` is mentioned there, with a warning not to mix it with `__is_defined()` which looks it was only intended as an implementation detail for `include/linux/kconfig.h`). CC'ing Masahiro by the way. Cheers, Miguel
Kees Cook <keescook@chromium.org> writes: >> > -#define NODE_NOT_IN_PAGE_FLAGS >> > +#define NODE_NOT_IN_PAGE_FLAGS 1 >> >> but if we ever lose the 1 then the above will silently compile the code >> within the IS_ENABLED() section out. > > That's true, yes. I considered two other ways to do this: > > 1) smallest patch, but more #ifdef: > 2) medium size, weird style: > > and 3 is what I sent: biggest, but removes #ifdef > > Any preference? From a readbility POV I surely prefer #3. i"m just wary. Add a big fat comment to the define might mitigate that, hmm? Thanks, tglx
On Thu, Jun 04, 2020 at 11:39:05PM +0200, Thomas Gleixner wrote: > Kees Cook <keescook@chromium.org> writes: > >> > -#define NODE_NOT_IN_PAGE_FLAGS > >> > +#define NODE_NOT_IN_PAGE_FLAGS 1 > >> > >> but if we ever lose the 1 then the above will silently compile the code > >> within the IS_ENABLED() section out. > > > > That's true, yes. I considered two other ways to do this: > > > > 1) smallest patch, but more #ifdef: > > 2) medium size, weird style: > > > > and 3 is what I sent: biggest, but removes #ifdef > > > > Any preference? > > From a readbility POV I surely prefer #3. i"m just wary. Add a big fat > comment to the define might mitigate that, hmm? Sure! I'll add it.
diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c index 59ba008504dc..38eeb15f3b07 100644 --- a/arch/x86/mm/numa.c +++ b/arch/x86/mm/numa.c @@ -541,7 +541,6 @@ static void __init numa_clear_kernel_node_hotplug(void) static int __init numa_register_memblks(struct numa_meminfo *mi) { - unsigned long uninitialized_var(pfn_align); int i, nid; /* Account for nodes with cpus and no memory */ @@ -569,15 +568,16 @@ static int __init numa_register_memblks(struct numa_meminfo *mi) * If sections array is gonna be used for pfn -> nid mapping, check * whether its granularity is fine enough. */ -#ifdef NODE_NOT_IN_PAGE_FLAGS - pfn_align = node_map_pfn_alignment(); - if (pfn_align && pfn_align < PAGES_PER_SECTION) { - printk(KERN_WARNING "Node alignment %LuMB < min %LuMB, rejecting NUMA config\n", - PFN_PHYS(pfn_align) >> 20, - PFN_PHYS(PAGES_PER_SECTION) >> 20); - return -EINVAL; + if (IS_ENABLED(NODE_NOT_IN_PAGE_FLAGS)) { + unsigned long pfn_align = node_map_pfn_alignment(); + + if (pfn_align && pfn_align < PAGES_PER_SECTION) { + pr_warn("Node alignment %LuMB < min %LuMB, rejecting NUMA config\n", + PFN_PHYS(pfn_align) >> 20, + PFN_PHYS(PAGES_PER_SECTION) >> 20); + return -EINVAL; + } } -#endif if (!numa_meminfo_cover_memory(mi)) return -EINVAL; diff --git a/include/linux/page-flags-layout.h b/include/linux/page-flags-layout.h index 71283739ffd2..1a4cdec2bd29 100644 --- a/include/linux/page-flags-layout.h +++ b/include/linux/page-flags-layout.h @@ -100,7 +100,7 @@ * there. This includes the case where there is no node, so it is implicit. */ #if !(NODES_WIDTH > 0 || NODES_SHIFT == 0) -#define NODE_NOT_IN_PAGE_FLAGS +#define NODE_NOT_IN_PAGE_FLAGS 1 #endif #if defined(CONFIG_NUMA_BALANCING) && LAST_CPUPID_WIDTH == 0
Using uninitialized_var() is dangerous as it papers over real bugs[1] (or can in the future), and suppresses unrelated compiler warnings (e.g. "unused variable"). If the compiler thinks it is uninitialized, either simply initialize the variable or make compiler changes. As a precursor to removing[2] this[3] macro[4], refactor code to avoid its need. The original reason for its use here was to work around the #ifdef being the only place the variable was used. This is better expressed using IS_ENABLED() and a new code block where the variable can be used unconditionally. [1] https://lore.kernel.org/lkml/20200603174714.192027-1-glider@google.com/ [2] https://lore.kernel.org/lkml/CA+55aFw+Vbj0i=1TGqCR5vQkCzWJ0QxK6CernOU6eedsudAixw@mail.gmail.com/ [3] https://lore.kernel.org/lkml/CA+55aFwgbgqhbp1fkxvRKEpzyR5J8n1vKT1VZdz9knmPuXhOeg@mail.gmail.com/ [4] https://lore.kernel.org/lkml/CA+55aFz2500WfbKXAx8s67wrm9=yVJu65TpLgN_ybYNv0VEOKA@mail.gmail.com/ Signed-off-by: Kees Cook <keescook@chromium.org> --- arch/x86/mm/numa.c | 18 +++++++++--------- include/linux/page-flags-layout.h | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-)