Re: [PATCH] cpumask: avoid WARN in prefill_possible_map()

From: Dmitry Safonov
Date: Wed Dec 14 2016 - 05:33:50 EST


On 12/13/2016 09:32 PM, Thomas Gleixner wrote:
On Mon, 12 Dec 2016, Dmitry Safonov wrote:

Subject : [PATCH] cpumask: avoid WARN in prefill_possible_map()

'cpumask' is hardly the proper prefix for x86/smpboot related issues.

With CONFIG_DEBUG_PER_CPU_MAPS and CONFIG_CPUMASK_OFFSTACK enabled
fixes the following WARN_ON_ONCE() for booting with nr_cpus=1:

This sentence, aside of not qualifying as a sentence, makes no sense.

What has this to do with nr_cpus=1? If I boot with nr_cpus=2 then this
won't fail or what?

[ 0.000000] Linux version 4.9.0 (dsafonov@xxxxxxxxxxxxxxxxxxxxx) (gcc version 4.8.5 20150623 (Red Hat 4.8.5-4) (GCC) ) #36 SMP Mon Dec 12 18:05:46 MSK 2016
[ 0.000000] Command line: BOOT_IMAGE=/vmlinuz-4.9.0 root=/dev/mapper/vz-root ro crashkernel=auto rd.lvm.lv=vz/root rd.lvm.lv=vz/swap console=ttyS0,115200 vsyscall=none nr_cpus=1
[ 0.000000] smpboot: 4 Processors exceeds NR_CPUS limit of 1
[ 0.000000] smpboot: Allowing 1 CPUs, 0 hotplug CPUs
[ 0.000000] ------------[ cut here ]------------
[ 0.000000] WARNING: CPU: 0 PID: 0 at ./include/linux/cpumask.h:121 cpumask_check.part.2+0x1c/0x1e
[ 0.000000] Modules linked in:
[ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.9.0 #36
[ 0.000000] Call Trace:
[ 0.000000] [<ffffffff8136664b>] dump_stack+0x67/0x9c
[ 0.000000] [<ffffffff81063e21>] __warn+0xd1/0xf0
[ 0.000000] [<ffffffff81063f0d>] warn_slowpath_null+0x1d/0x20
[ 0.000000] [<ffffffff8105f4ca>] cpumask_check.part.2+0x1c/0x1e
[ 0.000000] [<ffffffff8104008e>] cpumask_clear_cpu+0x2e/0x40
[ 0.000000] [<ffffffff81f88a62>] prefill_possible_map+0x15c/0x16a
[ 0.000000] [<ffffffff81f802c6>] setup_arch+0xba7/0xc33
[ 0.000000] [<ffffffff81f78c59>] start_kernel+0x63/0x448
[ 0.000000] [<ffffffff81f7858c>] x86_64_start_reservations+0x2a/0x2c
[ 0.000000] [<ffffffff81f78678>] x86_64_start_kernel+0xea/0xed
[ 0.000000] ---[ end trace 5876da8d2ace83fb ]---

And that non-trimmed backtrace is useful because it takes so much room in
the changelog and looks nice? The callchain leading to
prefill_possible_map() is pretty much well known, i.e. the backtrace is
pointless.

nr_cpu_ids is set to possible two lines futher - omit checking in
set_cpu_possible() cycles.

-ENOPARSE.

You completely fail to explain the problem, i.e. how nr_cpu_ids gets
overwritten from it's initial compile time value NR_CPUS.

And of course you fail to explain why the "solution" is correct or
whatever you consider it to be.

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 42f5eb7b4f6c..17167bec7c61 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1459,6 +1459,9 @@ __init void prefill_possible_map(void)
pr_info("Allowing %d CPUs, %d hotplug CPUs\n",
possible, max_t(int, possible - num_processors, 0));

+ /* Avoid WARN() in set_cpu_possible()=>cpumask_check() */
+ nr_cpu_ids = NR_CPUS;
+

If anything then this qualifies as a quick hack.

for (i = 0; i < possible; i++)
set_cpu_possible(i, true);
for (; i < NR_CPUS; i++)

The underlying issue is not restricted to nr_cpus=1 at all. The problem
comes from the early_param setting nr_cpu_ids to the command line
parameter. If that one is smaller than NR_CPUS then the access to the
possible mask with a cpu number > nr_cpu_ids will trigger the warning.

So instead of playing completely non obvious hackery with nr_cpu_ids the
proper solution is to have a function which clears the underlying
__cpu_possible_map, which is sized NR_CPUS because it is compile time
allocated and then only set the possible bits. Does the untested patch
below fix the issue for you?

Hi Thomas,

Well, my solution looks like a quick hack, because I didn't want to
introduce a new function in header which is used in one place.
And you did it...

> +static inline void cpumask_reset_possible_mask(void) { }
...
> +static inline void reset_cpu_possible_mask(void)

Is this intentionally?

Don't mind your version with fixed func-names and sorry for the bad
changelog.


Thanks,

tglx
8<--------------------
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1476,15 +1476,15 @@ early_param("possible_cpus", _setup_poss
possible = i;
}

+ nr_cpu_ids = possible;
+
pr_info("Allowing %d CPUs, %d hotplug CPUs\n",
possible, max_t(int, possible - num_processors, 0));

+ reset_cpu_possible_mask();
+
for (i = 0; i < possible; i++)
set_cpu_possible(i, true);
- for (; i < NR_CPUS; i++)
- set_cpu_possible(i, false);
-
- nr_cpu_ids = possible;
}

#ifdef CONFIG_HOTPLUG_CPU
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -112,6 +112,7 @@ extern struct cpumask __cpu_active_mask;
#define cpu_possible(cpu) ((cpu) == 0)
#define cpu_present(cpu) ((cpu) == 0)
#define cpu_active(cpu) ((cpu) == 0)
+static inline void cpumask_reset_possible_mask(void) { }
#endif

/* verify cpu argument to cpumask_* operators */
@@ -722,6 +723,11 @@ void init_cpu_present(const struct cpuma
void init_cpu_possible(const struct cpumask *src);
void init_cpu_online(const struct cpumask *src);

+static inline void reset_cpu_possible_mask(void)
+{
+ bitmap_zero(cpumask_bits(&__cpu_possible_mask), NR_CPUS);
+}
+
static inline void
set_cpu_possible(unsigned int cpu, bool possible)
{








--
Dmitry