Re: [PATCH] Avoid mask based num_possible_cpus and num_online_cpus

[Venki Pallipadi]

On Sun, Jan 22, 2012 at 9:22 PM, Srivatsa S. Bhat
<srivatsa.bhat@xxxxxxxxxxxxxxxxxx> wrote:
> On 01/21/2012 05:25 AM, Venki Pallipadi wrote:
>
>> On Fri, Jan 20, 2012 at 3:45 PM, KOSAKI Motohiro
>> <kosaki.motohiro@xxxxxxxxx> wrote:
>>>>>> +int nr_online_cpus __read_mostly;
>>>>>> +EXPORT_SYMBOL(nr_online_cpus);
>>>>>> +
>>>>>>  void set_cpu_possible(unsigned int cpu, bool possible)
>>>>>>  {
>>>>>>       if (possible)
>>>>>
>>>>>
>>>>> Did you forget to add:
>>>>>
>>>>>        nr_possible_cpus = cpumask_weight(cpu_possible_mask);
>>>>>
>>>>> inside set_cpu_possible() ?
>>>>
>>>> No. That was intentional as I have that coupled with nr_cpu_ids and
>>>> set once after all the bits are set in setup_nr_cpu_ids() instead of
>>>> doing for each bit set.
>>>
>>> But, Srivatsa's way seems more safer, no? Is there any advantage to make couple
>>> with nr_cpu_ids?
>>
>> I think it is a tradeoff between safer and cleaner :). infact, that's
>> how I had coded the patch first. But, then I changed it to be in sync
>> with nr_cpu_ids as it seemed a bit cleaner (and also to make sure 2048
>> CPU guys won't come after me for doing the mask calculation 2048 times
>> during the boot).
>>
>
>
> I knew you were trying to optimize further when I saw your patch. That's
> precisely the reason I cross-checked the code to ensure that the optimization
> didn't go beyond correctness :)
>
> And this is what I found:
>
> start_kernel()
>  setup_nr_cpu_ids() // This is not the end of setting up cpu_possible_mask
>  rest_init()
>    kernel_init()
>      smp_prepare_cpus();
>
> And on x86, this becomes:
>      native_smp_prepare_cpus();
>        smp_sanity_check(); // cpu_possible_mask & nr_cpu_ids can change here!
>           ^^^^^^^^^
>

Ack. Thanks for pointing this out. I missed this in my testing as this is under
if !defined(CONFIG_X86_BIGSMP) && defined(CONFIG_X86_32)

Proper way to handle this would be to call
setup_nr_cpu_ids();
After set_cpu_possible() loop.

I did a grep for any other place where nr_cpu_ids may be getting
written. And only other place I find
arch/powerpc/platforms/iseries/setup.c: nr_cpu_ids = max(nr_cpu_ids, 64);
but it does not have a accompanying set_cpu_possible(). So, the change
here should not affect this.

-Venki

> And there is another place where things can change:
> prefill_possible_map(). But this is called in setup_arch(), which is called
> before setup_nr_cpu_ids(). So we need not worry about this.
>
> (Btw, I checked only the x86 arch. Not sure how other architectures handle
> things.)
>
> Regards,
> Srivatsa S. Bhat
> IBM Linux Technology Center
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/