Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [PATCH] cpumask: avoid WARN in prefill_possible_map() | From | Dmitry Safonov <> | Date | Tue, 13 Dec 2016 21:53:06 +0300 |
| |
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@localhost.localdomain) (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
| ![\](/images/icornerr.gif) |