Re: [RFC PATCH powerpc] Set cpu sibling mask before online cpu

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

On 06/11/2013 12:30 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2013-05-16 at 18:20 +0800, Li Zhong wrote:
>> It seems following race is possible:
>>
> 
>  .../...
> 
>>  	vdso_getcpu_init();
>>  #endif
>> -	notify_cpu_starting(cpu);
>> -	set_cpu_online(cpu, true);
>>  	/* Update sibling maps */
>>  	base = cpu_first_thread_sibling(cpu);
>>  	for (i = 0; i < threads_per_core; i++) {
>> -		if (cpu_is_offline(base + i))
>> +		if (cpu_is_offline(base + i) && (cpu != base + i))
>>  			continue;
>>  		cpumask_set_cpu(cpu, cpu_sibling_mask(base + i));
>>  		cpumask_set_cpu(base + i, cpu_sibling_mask(cpu));
>> @@ -667,6 +665,10 @@ __cpuinit void start_secondary(void *unused)
>>  	}
>>  	of_node_put(l2_cache);
>>  
>> +	smp_wmb();
>> +	notify_cpu_starting(cpu);
>> +	set_cpu_online(cpu, true);
>> +
> 
> So we could have an online CPU with an empty sibling mask. Now we can
> have a sibling that isn't online ... Is that ok ?

I think it is OK. We do the same thing on x86 as well - we set up the
sibling links before calling notify_cpu_starting() and setting the cpu
in the cpu_online_mask. In fact, there is even a comment explicitly
noting that order:

arch/x86/kernel/smpboot.c:
 220         /*
 221          * This must be done before setting cpu_online_mask
 222          * or calling notify_cpu_starting.
 223          */
 224         set_cpu_sibling_map(raw_smp_processor_id());
 225         wmb();
 226 
 227         notify_cpu_starting(cpuid);
 228 
 229         /*
 230          * Allow the master to continue.
 231          */
 232         cpumask_set_cpu(cpuid, cpu_callin_mask);


So I agree with Li Zhong's solution.

[Arch-specific CPU hotplug code consolidation efforts such as [1] would
have weeded out such nasty bugs.. I guess we should revive that patchset
sometime soon.]

Regards,
Srivatsa S. Bhat

[1]. https://lwn.net/Articles/500185/