Re: [Xen-devel] [PATCH] x86/vpmu: add cpu hot unplug notifier for vpmu

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 05/17/2017 08:54 AM, Jan Beulich wrote:
>>>> On 17.05.17 at 14:40, <luwei.kang@xxxxxxxxx> wrote:
>>>>>> On 16.05.17 at 19:29, <luwei.kang@xxxxxxxxx> wrote:
>>>> Currently, hot unplug a cpu with vpmu enabled may cause system hang
>>>> due to send IPI to a die physical cpu. This patch add a cpu hot unplug
>>>> notifer to save vpmu context before cpu offline.
>>>>
>>>> Consider one scene, hotplug physical cpu N with vpmu is enabled.
>>> I think you mean "scenario" and "hot unplug".
>>>
>>>> The vcpu which running on this physical cpu before will be switch to
>>>> other online cpu. Before load the vpmu context to new physical cpu, a
>>>> IPI will be send to cpu N to save the vpmu context.
>>>> System will hang in function on_select_cpus because of that physical
>>>> cpu is offline and can not do any response.
>>> Doesn't this make clear that you would better also make sure
>>> ->last_pcpu doesn't hold to the then stale CPU anymore? For
>>> example, vpmu_load() compares it with smp_processor_id() (the subsequent 
>>> use 
>> is guarded by a VPMU_CONTEXT_LOADED flag
>>> check), allowing badness if the same or another CPU with the same number 
>> comes up again quickly enough. Similarly
>>> vpmu_arch_destroy() uses it without checking VPMU_CONTEXT_LOADED.
>>     I think it may can't make sure  "->last_pcpu" doesn't hold to the then 
>> stale CPU. The purpose of this notifier is to save the vpmu context before 
>> cpu offline. Avoid save vpmu context by send IPI to that offline cpu. There 
>> is no reason to change the value except it saving (vpmu_save()) in another 
>> physical cpu.
> I'm afraid I don't understand most of your reply.
>
>>     Regarding vpmu_arch_destroy(), it indeed will cause same issue. What 
>> about add " this_cpu(cpu) = NULL" in cpu_callback() to clean the last_vcpu 
>> pointer of this physical cpu. 
> That's being done by vpmu_save_force() already afaict (assuming
> you mean this_cpu(last_vcpu)), albeit for whatever reason open
> coding this_cpu().
>
>>     In addition, add VPMU_CONTEXT_LOADED check before execute 
>> on_selected_cpus(cpumask_of(vcpu_vpmu(v)->last_pcpu), vpmu_save_force, v, 1) 
>> in 
>> vpmu_arch_destroy(). Because of force save operation has been finished in 
>> notifier function.
> I'm not sure whether that would be correct. Boris?


I believe we still have a race with vpmu_load(): it can be past
VPMU_CONTEXT_LOADED test and committed to the remote call when the
remote VCPU becomes offlined.

Taking vpmu_lock in vpmu_load() and cpu_callback() (which IMO should be
called vpmu_cpu_callback() or some such) may be one solution, although
holding a lock across a remote call is not optimal, obviously.

And I think the same argument is applicable to vpmu_arch_destroy().

-boris

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel