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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.11.2019 12:43, Chao Gao wrote:
> 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.

Oh, you're right of course. Unless the map was allocated
unconditionally ...

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

The purposes of cpu_down() calls _may_ be different. Plus whether
there's parking wanted/necessary for an architecture should remain
- as much as possible - an architecture thing to deal with. I.e.
despite park_offline_cpus being used in common code, I think we
should strive to avoid adding more there when it can be avoided.

Jan

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