Date:   Tue, 13 Dec 2016 21:53:06 +0300
From:   Dmitry Safonov <dsafonov@...tuozzo.com>
To:     Thomas Gleixner <tglx@...utronix.de>
CC:     <linux-kernel@...r.kernel.org>, <0x7f454c46@...il.com>,
        Ingo Molnar <mingo@...hat.com>,
        Rusty Russell <rusty@...tcorp.com.au>,
        "H. Peter Anvin" <hpa@...or.com>,
        Jan Beulich <JBeulich@...ell.com>, <x86@...nel.org>
Subject: Re: [PATCH] cpumask: avoid WARN in prefill_possible_map()

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@...alhost.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

Powered by blists - more mailing lists