Il 26/06/2013 10:41, Srivatsa S. Bhat ha scritto: > 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. Makes sense, and I see now that it's patch 9 in this series. In general, I would rather add an extra get_online_cpus_atomic pair where it it actually needed (i.e. closer to where cpu_online is actually used), and leave get_cpu/put_cpu as is in the caller... which is exactly what happens in this case, since "where it is actually needed" is "in smp_call_function_single()". > So, please ignore this patch, and sorry for the noise! No problem, thanks for the heads-up. Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-arch" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
