Re: [PATCH v2 29/45] kvm/vmx: Use get/put_online_cpus_atomic() to prevent CPU offline

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

On 06/26/2013 01:53 PM, Paolo Bonzini wrote:
> Il 26/06/2013 10:06, Srivatsa S. Bhat ha scritto:
>> On 06/26/2013 01:16 PM, Paolo Bonzini wrote:
>>> Il 25/06/2013 22:30, Srivatsa S. Bhat ha scritto:
>>>> -	cpu = get_cpu();
>>>> +	cpu = get_online_cpus_atomic();
>>>>  	vmx_vcpu_load(&vmx->vcpu, cpu);
>>>>  	vmx->vcpu.cpu = cpu;
>>>>  	err = vmx_vcpu_setup(vmx);
>>>>  	vmx_vcpu_put(&vmx->vcpu);
>>>> -	put_cpu();
>>>> +	put_online_cpus_atomic();
>>>
>>> The new API has a weird name.  Why are you adding new functions instead
>>> of just modifying get/put_cpu?
>>>
>>
>> Because the purpose of those two functions are distinctly different
>> from each other.
>>
>> get/put_cpu() is used to disable preemption on the local CPU. (Which
>> also disables offlining the local CPU during that critical section).
> 
> Ok, then I understood correctly... and I acked the other KVM patch.
>

Thank you!
 
> However, keeping the code on the local CPU is exactly the point of this
> particular use of get_cpu()/put_cpu().  Why does it need to synchronize
> with offlining of other CPUs?
> 

Now that I looked at it again, I think you are right, get/put_cpu() is
good enough here.

But let me explain why I initially thought we needed full synchronization
with CPU offline. In short, I wanted to synchronize the calls to
__loaded_vmcs_clear(). We have the scenario shown below:

CPU offline:
	CPU_DYING:
		hardware_disable();
		->vmclear_local_loaded_vmcss();
		  ->__loaded_vmcs_clear(v);



And vmx_vcpu_load() (among others) can do:
       vmx_vcpu_load();
       -> loaded_vmcs_clear();
          -> __loaded_vmcs_clear();


So I wanted to avoid this race-condition and hence wrapped the code with
get/put_online_cpus_atomic().

But the point I missed earlier is that loaded_vmcs_clear() calls
__loaded_vmcs_clear() using smp_call_function_single(), which itself
synchronizes properly with CPU hotplug. So there is no need to add full
hotplug synchronization in the vmx code, as you noted above.

So, please ignore this patch, and sorry for the noise!

Regards,
Srivatsa S. Bhat