Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933585AbcLMSfW (ORCPT ); Tue, 13 Dec 2016 13:35:22 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:42324 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932250AbcLMSfS (ORCPT ); Tue, 13 Dec 2016 13:35:18 -0500 Date: Tue, 13 Dec 2016 19:32:28 +0100 (CET) From: Thomas Gleixner To: Dmitry Safonov cc: linux-kernel@vger.kernel.org, 0x7f454c46@gmail.com, Ingo Molnar , Rusty Russell , "H. Peter Anvin" , Jan Beulich , x86@kernel.org Subject: Re: [PATCH] cpumask: avoid WARN in prefill_possible_map() In-Reply-To: <20161212161118.1095-1-dsafonov@virtuozzo.com> Message-ID: References: <20161212161118.1095-1-dsafonov@virtuozzo.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4946 Lines: 133 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] [] dump_stack+0x67/0x9c > [ 0.000000] [] __warn+0xd1/0xf0 > [ 0.000000] [] warn_slowpath_null+0x1d/0x20 > [ 0.000000] [] cpumask_check.part.2+0x1c/0x1e > [ 0.000000] [] cpumask_clear_cpu+0x2e/0x40 > [ 0.000000] [] prefill_possible_map+0x15c/0x16a > [ 0.000000] [] setup_arch+0xba7/0xc33 > [ 0.000000] [] start_kernel+0x63/0x448 > [ 0.000000] [] x86_64_start_reservations+0x2a/0x2c > [ 0.000000] [] 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? 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) {