| Message ID | 20130625203043.16593.1600.stgit@srivatsabhat.in.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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? Paolo
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). What this patchset deals with is synchronizing with offline of *any* CPU. Typically, we use get_online_cpus()/put_online_cpus() for that purpose. But they can't be used in atomic context, because they take mutex locks and hence can sleep. So the code that executes in atomic context and which wants to prevent *any* CPU from going offline, used to disable preemption around its critical section. Disabling preemption prevents stop_machine(), and CPU offline (of *any* CPU) was done via stop_machine(). So disabling preemption disabled any CPU from going offline, as a *side-effect*. And this patchset prepares the ground for getting rid of stop_machine() in the CPU offline path. Which means, disabling preemption only prevents the *local* CPU from going offline. So if code in atomic context wants to prevent any CPU from going offline, we need a new set of APIs, like get/put_online_cpus(), but which can be invoked from atomic context. That's why I named it as get/put_online_cpus_atomic(). One of the key points here is that we want to preserve get/put_cpu() as it is, since its purpose is different - disable preemption and offline of the local CPU. There is no reason to change that API, its useful as it is. Regards, Srivatsa S. Bhat
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. 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? Paolo > What this patchset deals with is synchronizing with offline of *any* > CPU. Typically, we use get_online_cpus()/put_online_cpus() for that > purpose. But they can't be used in atomic context, because they take > mutex locks and hence can sleep. > > So the code that executes in atomic context and which wants to prevent > *any* CPU from going offline, used to disable preemption around its > critical section. Disabling preemption prevents stop_machine(), and > CPU offline (of *any* CPU) was done via stop_machine(). So disabling > preemption disabled any CPU from going offline, as a *side-effect*. > > And this patchset prepares the ground for getting rid of stop_machine() > in the CPU offline path. Which means, disabling preemption only prevents > the *local* CPU from going offline. So if code in atomic context wants > to prevent any CPU from going offline, we need a new set of APIs, like > get/put_online_cpus(), but which can be invoked from atomic context. > That's why I named it as get/put_online_cpus_atomic(). > > One of the key points here is that we want to preserve get/put_cpu() > as it is, since its purpose is different - disable preemption and > offline of the local CPU. There is no reason to change that API, its > useful as it is.
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
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
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 260a919..4e1e966 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -26,6 +26,7 @@ #include <linux/mm.h> #include <linux/highmem.h> #include <linux/sched.h> +#include <linux/cpu.h> #include <linux/moduleparam.h> #include <linux/mod_devicetable.h> #include <linux/ftrace_event.h> @@ -7164,12 +7165,12 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id) if (!vmm_exclusive) kvm_cpu_vmxoff(); - 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(); if (err) goto free_vmcs; if (vm_need_virtualize_apic_accesses(kvm)) { @@ -7706,12 +7707,12 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) vmx->nested.vmcs01_tsc_offset = vmcs_read64(TSC_OFFSET); - cpu = get_cpu(); + cpu = get_online_cpus_atomic(); vmx->loaded_vmcs = vmcs02; vmx_vcpu_put(vcpu); vmx_vcpu_load(vcpu, cpu); vcpu->cpu = cpu; - put_cpu(); + put_online_cpus_atomic(); vmx_segment_cache_clear(vmx); @@ -8023,12 +8024,12 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu) leave_guest_mode(vcpu); prepare_vmcs12(vcpu, vmcs12); - cpu = get_cpu(); + cpu = get_online_cpus_atomic(); vmx->loaded_vmcs = &vmx->vmcs01; vmx_vcpu_put(vcpu); vmx_vcpu_load(vcpu, cpu); vcpu->cpu = cpu; - put_cpu(); + put_online_cpus_atomic(); vmx_segment_cache_clear(vmx);
Once stop_machine() is gone from the CPU offline path, we won't be able to depend on disabling preemption to prevent CPUs from going offline from under us. Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline, while invoking from atomic context. Cc: Gleb Natapov <gleb@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: x86@kernel.org Cc: kvm@vger.kernel.org Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> --- arch/x86/kvm/vmx.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)