* [PATCH] cpumask: avoid WARN in prefill_possible_map() @ 2016-12-12 16:11 Dmitry Safonov 2016-12-13 18:32 ` Thomas Gleixner 0 siblings, 1 reply; 5+ messages in thread From: Dmitry Safonov @ 2016-12-12 16:11 UTC (permalink / raw) To: linux-kernel Cc: 0x7f454c46, Dmitry Safonov, Thomas Gleixner, Ingo Molnar, Rusty Russell, H. Peter Anvin, Jan Beulich, x86 With CONFIG_DEBUG_PER_CPU_MAPS and CONFIG_CPUMASK_OFFSTACK enabled fixes the following WARN_ON_ONCE() for booting with nr_cpus=1: [ 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 ]--- nr_cpu_ids is set to possible two lines futher - omit checking in set_cpu_possible() cycles. Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: Rusty Russell <rusty@rustcorp.com.au> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Jan Beulich <JBeulich@novell.com> Cc: x86@kernel.org Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com> --- arch/x86/kernel/smpboot.c | 3 +++ 1 file changed, 3 insertions(+) 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; + for (i = 0; i < possible; i++) set_cpu_possible(i, true); for (; i < NR_CPUS; i++) -- 2.10.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cpumask: avoid WARN in prefill_possible_map() 2016-12-12 16:11 [PATCH] cpumask: avoid WARN in prefill_possible_map() Dmitry Safonov @ 2016-12-13 18:32 ` Thomas Gleixner 2016-12-13 18:53 ` Dmitry Safonov 2016-12-15 11:52 ` [tip:x86/urgent] x86/smpboot: Prevent false positive out of bounds cpumask access warning tip-bot for Thomas Gleixner 0 siblings, 2 replies; 5+ messages in thread From: Thomas Gleixner @ 2016-12-13 18:32 UTC (permalink / raw) To: Dmitry Safonov Cc: linux-kernel, 0x7f454c46, Ingo Molnar, Rusty Russell, H. Peter Anvin, Jan Beulich, x86 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? 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) { ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cpumask: avoid WARN in prefill_possible_map() 2016-12-13 18:32 ` Thomas Gleixner @ 2016-12-13 18:53 ` Dmitry Safonov 2016-12-13 20:42 ` Thomas Gleixner 2016-12-15 11:52 ` [tip:x86/urgent] x86/smpboot: Prevent false positive out of bounds cpumask access warning tip-bot for Thomas Gleixner 1 sibling, 1 reply; 5+ messages in thread From: Dmitry Safonov @ 2016-12-13 18:53 UTC (permalink / raw) To: Thomas Gleixner Cc: linux-kernel, 0x7f454c46, Ingo Molnar, Rusty Russell, H. Peter Anvin, Jan Beulich, x86 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cpumask: avoid WARN in prefill_possible_map() 2016-12-13 18:53 ` Dmitry Safonov @ 2016-12-13 20:42 ` Thomas Gleixner 0 siblings, 0 replies; 5+ messages in thread From: Thomas Gleixner @ 2016-12-13 20:42 UTC (permalink / raw) To: Dmitry Safonov Cc: linux-kernel, 0x7f454c46, Ingo Molnar, Rusty Russell, H. Peter Anvin, Jan Beulich, x86 On Tue, 13 Dec 2016, Dmitry Safonov wrote: > > +static inline void cpumask_reset_possible_mask(void) { } > ... > > +static inline void reset_cpu_possible_mask(void) > > Is this intentionally? No, that was a left over and a missing refresh bevor sending it out quickly. Thanks, tglx ^ permalink raw reply [flat|nested] 5+ messages in thread
* [tip:x86/urgent] x86/smpboot: Prevent false positive out of bounds cpumask access warning 2016-12-13 18:32 ` Thomas Gleixner 2016-12-13 18:53 ` Dmitry Safonov @ 2016-12-15 11:52 ` tip-bot for Thomas Gleixner 1 sibling, 0 replies; 5+ messages in thread From: tip-bot for Thomas Gleixner @ 2016-12-15 11:52 UTC (permalink / raw) To: linux-tip-commits Cc: dsafonov, linux-kernel, mingo, hpa, tglx, JBeulich, rusty Commit-ID: 427d77a32365d5f942d335248305a5c237baf63a Gitweb: http://git.kernel.org/tip/427d77a32365d5f942d335248305a5c237baf63a Author: Thomas Gleixner <tglx@linutronix.de> AuthorDate: Tue, 13 Dec 2016 19:32:28 +0100 Committer: Thomas Gleixner <tglx@linutronix.de> CommitDate: Thu, 15 Dec 2016 11:32:31 +0100 x86/smpboot: Prevent false positive out of bounds cpumask access warning prefill_possible_map() reinitializes the cpu_possible_map by setting the possible cpu bits and clearing all other bits up to NR_CPUS. This is technically always correct because cpu_possible_map is statically allocated and sized NR_CPUS. With CPUMASK_OFFSTACK and DEBUG_PER_CPU_MAPS enabled the bounds check of cpu masks happens on nr_cpu_ids. nr_cpu_ids is initialized to NR_CPUS and only limited after the set/clear bit loops have been executed. But if the system was booted with "nr_cpus=N" on the command line, where N is < NR_CPUS then nr_cpu_ids is limited in the parameter parsing function before prefill_possible_map() is invoked. As a consequence the cpumask bounds check triggers when clearing the bits past nr_cpu_ids. Add a helper which allows to reset cpu_possible_map w/o the bounds check and then set only the possible bits which are well inside bounds. Reported-by: Dmitry Safonov <dsafonov@virtuozzo.com> Cc: Rusty Russell <rusty@rustcorp.com.au> Cc: 0x7f454c46@gmail.com Cc: Jan Beulich <JBeulich@novell.com> Link: http://lkml.kernel.org/r/alpine.DEB.2.20.1612131836050.3415@nanos Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- arch/x86/kernel/smpboot.c | 8 ++++---- include/linux/cpumask.h | 5 +++++ 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c index e09aa58..46732dc 100644 --- a/arch/x86/kernel/smpboot.c +++ b/arch/x86/kernel/smpboot.c @@ -1463,15 +1463,15 @@ __init void prefill_possible_map(void) 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 diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h index da7fbf1..c717f5e 100644 --- a/include/linux/cpumask.h +++ b/include/linux/cpumask.h @@ -722,6 +722,11 @@ void init_cpu_present(const struct cpumask *src); 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) { ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-12-15 11:53 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-12-12 16:11 [PATCH] cpumask: avoid WARN in prefill_possible_map() Dmitry Safonov 2016-12-13 18:32 ` Thomas Gleixner 2016-12-13 18:53 ` Dmitry Safonov 2016-12-13 20:42 ` Thomas Gleixner 2016-12-15 11:52 ` [tip:x86/urgent] x86/smpboot: Prevent false positive out of bounds cpumask access warning tip-bot for Thomas Gleixner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
Powered by JetHome