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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.05.17 at 15:58, <boris.ostrovsky@xxxxxxxxxx> wrote:
> 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.

The offlined entity is a pCPU, and such offlining happens in stop-
machine context iirc.

Jan

> 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