[PATCH v2 28/45] KVM: Use get/put_online_cpus_atomic() to prevent CPU offline

[PATCH v2 28/45] KVM: Use get/put_online_cpus_atomic() to prevent CPU offline

[Srivatsa S. Bhat]

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: kvm@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 virt/kvm/kvm_main.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 302681c..5bbfa30 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -174,7 +174,7 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)
 
 	zalloc_cpumask_var(&cpus, GFP_ATOMIC);
 
-	me = get_cpu();
+	me = get_online_cpus_atomic();
 	kvm_for_each_vcpu(i, vcpu, kvm) {
 		kvm_make_request(req, vcpu);
 		cpu = vcpu->cpu;
@@ -192,7 +192,7 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)
 		smp_call_function_many(cpus, ack_flush, NULL, 1);
 	else
 		called = false;
-	put_cpu();
+	put_online_cpus_atomic();
 	free_cpumask_var(cpus);
 	return called;
 }
@@ -1707,11 +1707,11 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
 		++vcpu->stat.halt_wakeup;
 	}
 
-	me = get_cpu();
+	me = get_online_cpus_atomic();
 	if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
 		if (kvm_arch_vcpu_should_kick(vcpu))
 			smp_send_reschedule(cpu);
-	put_cpu();
+	put_online_cpus_atomic();
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_kick);
 #endif /* !CONFIG_S390 */

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

[Srivatsa S. Bhat]

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(-)

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);
 

[PATCH v2 41/45] powerpc: Use get/put_online_cpus_atomic() to prevent CPU offline

[Srivatsa S. Bhat]

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: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Gleb Natapov <gleb@redhat.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: Zhao Chenhui <chenhui.zhao@freescale.com>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: kvm@vger.kernel.org
Cc: kvm-ppc@vger.kernel.org
Cc: oprofile-list@lists.sf.net
Cc: cbe-oss-dev@lists.ozlabs.org
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 arch/powerpc/kernel/irq.c                  |    7 ++++++-
 arch/powerpc/kernel/machine_kexec_64.c     |    4 ++--
 arch/powerpc/kernel/smp.c                  |    2 ++
 arch/powerpc/kvm/book3s_hv.c               |    5 +++--
 arch/powerpc/mm/mmu_context_nohash.c       |    3 +++
 arch/powerpc/oprofile/cell/spu_profiler.c  |    3 +++
 arch/powerpc/oprofile/cell/spu_task_sync.c |    4 ++++
 arch/powerpc/oprofile/op_model_cell.c      |    6 ++++++
 8 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index ca39bac..41e9961 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -45,6 +45,7 @@
 #include <linux/irq.h>
 #include <linux/seq_file.h>
 #include <linux/cpumask.h>
+#include <linux/cpu.h>
 #include <linux/profile.h>
 #include <linux/bitops.h>
 #include <linux/list.h>
@@ -410,7 +411,10 @@ void migrate_irqs(void)
 	unsigned int irq;
 	static int warned;
 	cpumask_var_t mask;
-	const struct cpumask *map = cpu_online_mask;
+	const struct cpumask *map;
+
+	get_online_cpus_atomic();
+	map = cpu_online_mask;
 
 	alloc_cpumask_var(&mask, GFP_ATOMIC);
 
@@ -436,6 +440,7 @@ void migrate_irqs(void)
 	}
 
 	free_cpumask_var(mask);
+	put_online_cpus_atomic();
 
 	local_irq_enable();
 	mdelay(1);
diff --git a/arch/powerpc/kernel/machine_kexec_64.c b/arch/powerpc/kernel/machine_kexec_64.c
index 611acdf..38f6d75 100644
--- a/arch/powerpc/kernel/machine_kexec_64.c
+++ b/arch/powerpc/kernel/machine_kexec_64.c
@@ -187,7 +187,7 @@ static void kexec_prepare_cpus_wait(int wait_state)
 	int my_cpu, i, notified=-1;
 
 	hw_breakpoint_disable();
-	my_cpu = get_cpu();
+	my_cpu = get_online_cpus_atomic();
 	/* Make sure each CPU has at least made it to the state we need.
 	 *
 	 * FIXME: There is a (slim) chance of a problem if not all of the CPUs
@@ -266,7 +266,7 @@ static void kexec_prepare_cpus(void)
 	 */
 	kexec_prepare_cpus_wait(KEXEC_STATE_REAL_MODE);
 
-	put_cpu();
+	put_online_cpus_atomic();
 }
 
 #else /* ! SMP */
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index ee7ac5e..2123bec 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -277,9 +277,11 @@ void smp_send_debugger_break(void)
 	if (unlikely(!smp_ops))
 		return;
 
+	get_online_cpus_atomic();
 	for_each_online_cpu(cpu)
 		if (cpu != me)
 			do_message_pass(cpu, PPC_MSG_DEBUGGER_BREAK);
+	put_online_cpus_atomic();
 }
 #endif
 
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 2efa9dd..9d8a973 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -28,6 +28,7 @@
 #include <linux/fs.h>
 #include <linux/anon_inodes.h>
 #include <linux/cpumask.h>
+#include <linux/cpu.h>
 #include <linux/spinlock.h>
 #include <linux/page-flags.h>
 #include <linux/srcu.h>
@@ -78,7 +79,7 @@ void kvmppc_fast_vcpu_kick(struct kvm_vcpu *vcpu)
 		++vcpu->stat.halt_wakeup;
 	}
 
-	me = get_cpu();
+	me = get_online_cpus_atomic();
 
 	/* CPU points to the first thread of the core */
 	if (cpu != me && cpu >= 0 && cpu < nr_cpu_ids) {
@@ -88,7 +89,7 @@ void kvmppc_fast_vcpu_kick(struct kvm_vcpu *vcpu)
 		else if (cpu_online(cpu))
 			smp_send_reschedule(cpu);
 	}
-	put_cpu();
+	put_online_cpus_atomic();
 }
 
 /*
diff --git a/arch/powerpc/mm/mmu_context_nohash.c b/arch/powerpc/mm/mmu_context_nohash.c
index e779642..c7bdcb4 100644
--- a/arch/powerpc/mm/mmu_context_nohash.c
+++ b/arch/powerpc/mm/mmu_context_nohash.c
@@ -194,6 +194,8 @@ void switch_mmu_context(struct mm_struct *prev, struct mm_struct *next)
 	unsigned int i, id, cpu = smp_processor_id();
 	unsigned long *map;
 
+	get_online_cpus_atomic();
+
 	/* No lockless fast path .. yet */
 	raw_spin_lock(&context_lock);
 
@@ -280,6 +282,7 @@ void switch_mmu_context(struct mm_struct *prev, struct mm_struct *next)
 	pr_hardcont(" -> %d\n", id);
 	set_context(id, next->pgd);
 	raw_spin_unlock(&context_lock);
+	put_online_cpus_atomic();
 }
 
 /*
diff --git a/arch/powerpc/oprofile/cell/spu_profiler.c b/arch/powerpc/oprofile/cell/spu_profiler.c
index b129d00..ab6e6c1 100644
--- a/arch/powerpc/oprofile/cell/spu_profiler.c
+++ b/arch/powerpc/oprofile/cell/spu_profiler.c
@@ -14,6 +14,7 @@
 
 #include <linux/hrtimer.h>
 #include <linux/smp.h>
+#include <linux/cpu.h>
 #include <linux/slab.h>
 #include <asm/cell-pmu.h>
 #include <asm/time.h>
@@ -142,6 +143,7 @@ static enum hrtimer_restart profile_spus(struct hrtimer *timer)
 	if (!spu_prof_running)
 		goto stop;
 
+	get_online_cpus_atomic();
 	for_each_online_cpu(cpu) {
 		if (cbe_get_hw_thread_id(cpu))
 			continue;
@@ -177,6 +179,7 @@ static enum hrtimer_restart profile_spus(struct hrtimer *timer)
 				       oprof_spu_smpl_arry_lck_flags);
 
 	}
+	put_online_cpus_atomic();
 	smp_wmb();	/* insure spu event buffer updates are written */
 			/* don't want events intermingled... */
 
diff --git a/arch/powerpc/oprofile/cell/spu_task_sync.c b/arch/powerpc/oprofile/cell/spu_task_sync.c
index 28f1af2..8464ef6 100644
--- a/arch/powerpc/oprofile/cell/spu_task_sync.c
+++ b/arch/powerpc/oprofile/cell/spu_task_sync.c
@@ -28,6 +28,7 @@
 #include <linux/oprofile.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/cpu.h>
 #include "pr_util.h"
 
 #define RELEASE_ALL 9999
@@ -448,11 +449,14 @@ static int number_of_online_nodes(void)
 {
         u32 cpu; u32 tmp;
         int nodes = 0;
+
+	get_online_cpus_atomic();
         for_each_online_cpu(cpu) {
                 tmp = cbe_cpu_to_node(cpu) + 1;
                 if (tmp > nodes)
                         nodes++;
         }
+	put_online_cpus_atomic();
         return nodes;
 }
 
diff --git a/arch/powerpc/oprofile/op_model_cell.c b/arch/powerpc/oprofile/op_model_cell.c
index b9589c1..c9bb028 100644
--- a/arch/powerpc/oprofile/op_model_cell.c
+++ b/arch/powerpc/oprofile/op_model_cell.c
@@ -22,6 +22,7 @@
 #include <linux/oprofile.h>
 #include <linux/percpu.h>
 #include <linux/smp.h>
+#include <linux/cpu.h>
 #include <linux/spinlock.h>
 #include <linux/timer.h>
 #include <asm/cell-pmu.h>
@@ -463,6 +464,7 @@ static void cell_virtual_cntr(unsigned long data)
 	 * not both playing with the counters on the same node.
 	 */
 
+	get_online_cpus_atomic();
 	spin_lock_irqsave(&cntr_lock, flags);
 
 	prev_hdw_thread = hdw_thread;
@@ -550,6 +552,7 @@ static void cell_virtual_cntr(unsigned long data)
 	}
 
 	spin_unlock_irqrestore(&cntr_lock, flags);
+	put_online_cpus_atomic();
 
 	mod_timer(&timer_virt_cntr, jiffies + HZ / 10);
 }
@@ -608,6 +611,8 @@ static void spu_evnt_swap(unsigned long data)
 	/* Make sure spu event interrupt handler and spu event swap
 	 * don't access the counters simultaneously.
 	 */
+
+	get_online_cpus_atomic();
 	spin_lock_irqsave(&cntr_lock, flags);
 
 	cur_spu_evnt_phys_spu_indx = spu_evnt_phys_spu_indx;
@@ -673,6 +678,7 @@ static void spu_evnt_swap(unsigned long data)
 	}
 
 	spin_unlock_irqrestore(&cntr_lock, flags);
+	put_online_cpus_atomic();
 
 	/* swap approximately every 0.1 seconds */
 	mod_timer(&timer_spu_event_swap, jiffies + HZ / 25);

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

[Paolo Bonzini]

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

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

[Srivatsa S. Bhat]

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

Re: [PATCH v2 28/45] KVM: Use get/put_online_cpus_atomic() to prevent CPU offline

[Paolo Bonzini]

Il 25/06/2013 22:30, Srivatsa S. Bhat ha scritto:
> 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: kvm@vger.kernel.org
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
> 
>  virt/kvm/kvm_main.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 302681c..5bbfa30 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -174,7 +174,7 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)
>  
>  	zalloc_cpumask_var(&cpus, GFP_ATOMIC);
>  
> -	me = get_cpu();
> +	me = get_online_cpus_atomic();
>  	kvm_for_each_vcpu(i, vcpu, kvm) {
>  		kvm_make_request(req, vcpu);
>  		cpu = vcpu->cpu;
> @@ -192,7 +192,7 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)
>  		smp_call_function_many(cpus, ack_flush, NULL, 1);
>  	else
>  		called = false;
> -	put_cpu();
> +	put_online_cpus_atomic();
>  	free_cpumask_var(cpus);
>  	return called;
>  }
> @@ -1707,11 +1707,11 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
>  		++vcpu->stat.halt_wakeup;
>  	}
>  
> -	me = get_cpu();
> +	me = get_online_cpus_atomic();
>  	if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
>  		if (kvm_arch_vcpu_should_kick(vcpu))
>  			smp_send_reschedule(cpu);
> -	put_cpu();
> +	put_online_cpus_atomic();
>  }
>  EXPORT_SYMBOL_GPL(kvm_vcpu_kick);
>  #endif /* !CONFIG_S390 */
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

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

[Paolo Bonzini]

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.

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

[Srivatsa S. Bhat]

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

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

[Paolo Bonzini]

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