Re: [Xen-devel] [PATCH v1 1/2] x86/cpu: maintain a parked CPU bitmap

[Chao Gao <chao.gao@xxxxxxxxx>]

On Thu, Nov 21, 2019 at 11:02:10AM +0100, Jan Beulich wrote:
>On 21.11.2019 10:47, Julien Grall wrote:
>> On 20/11/2019 23:05, Chao Gao wrote:
>>> --- a/xen/arch/arm/smpboot.c
>>> +++ b/xen/arch/arm/smpboot.c
>>> @@ -39,6 +39,7 @@
>>>   cpumask_t cpu_online_map;
>>>   cpumask_t cpu_present_map;
>>>   cpumask_t cpu_possible_map;
>>> +cpumask_var_t cpu_parked_map;
>> 
>> You define cpu_parked_map but AFAIK it will never get allocated. The 
>> risk here is any access to that variable will result to a fault.
>> 
>> Looking at the changes below, it looks like access in common code will 
>> be protected by park_offline_cpus. This is always false on Arm, so the 
>> compiler should remove any access to cpu_parked_map.
>> 
>> With that in mind, I think it would be best to only provide a prototype 
>> for cpu_parked_map and so the linker can warn if someone used it.
>
>+1

Will do. I added this because I am not sure all compilers would omit
such access.

>
>In fact I wonder whether the maintenance of the map should live in
>common code in the first place. While clearing the respective bit
>in cpu_up() looks correct (and could be done without any if()),

But when park_offline_cpus() is false, the map isn't allocated. I don't
think it is safe to access the map in this case.

>I'm not convinced the setting of the bit in cpu_down() is going to
>be correct in all cases.

Do you mean in some cases, cpu_down() is to really offline a CPU even
park_offline_cpus is set? And in this case, setting the bit isn't
correct.

If yes, one thing confuses me is that cpu_down() would call
cpu_notifier_call_chain() several times unconditionally. And registered
callbacks take actions depending on the value of park_offline_cpus.
I expected that callbacks would do more check to avoid parking a CPU
in some cases.

Thanks
Chao

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel