Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1582666 > unrolled thread

[PATCH 0/5] KVM: rename and extend vcpu->requests API

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] KVM: rename and extend vcpu->requests API Radim Krčmář <rkrcmar@redhat.com> - 2017-02-16 17:10 +0100
    [PATCH 5/5] KVM: add kvm_request_pending Radim Krčmář <rkrcmar@redhat.com> - 2017-02-16 17:10 +0100
      Re: [PATCH 5/5] KVM: add kvm_request_pending David Hildenbrand <david@redhat.com> - 2017-02-16 21:00 +0100
      Re: [PATCH 5/5] KVM: add kvm_request_pending Andrew Jones <drjones@redhat.com> - 2017-02-17 11:00 +0100
        Re: [PATCH 5/5] KVM: add kvm_request_pending Radim Krčmář <rkrcmar@redhat.com> - 2017-02-17 16:00 +0100
    [PATCH 2/5] KVM: add KVM request variants without barrier Radim Krčmář <rkrcmar@redhat.com> - 2017-02-16 17:10 +0100
      Re: [PATCH 2/5] KVM: add KVM request variants without barrier Paolo Bonzini <pbonzini@redhat.com> - 2017-02-23 12:00 +0100
        Re: [PATCH 2/5] KVM: add KVM request variants without barrier Radim Krčmář <rkrcmar@redhat.com> - 2017-02-23 17:00 +0100
    [PATCH 3/5] KVM: optimize kvm_make_all_cpus_request Radim Krčmář <rkrcmar@redhat.com> - 2017-02-16 17:10 +0100
    Re: [PATCH 1/5] KVM: change API for requests to match bit  operations Cornelia Huck <cornelia.huck@de.ibm.com> - 2017-02-17 10:40 +0100
      Re: [PATCH 1/5] KVM: change API for requests to match bit operations Andrew Jones <drjones@redhat.com> - 2017-02-17 11:00 +0100
        Re: [PATCH 1/5] KVM: change API for requests to match bit  operations Cornelia Huck <cornelia.huck@de.ibm.com> - 2017-02-17 11:00 +0100
      Re: [PATCH 1/5] KVM: change API for requests to match bit operations Radim Krčmář <rkrcmar@redhat.com> - 2017-02-17 16:10 +0100

#1582666 — [PATCH 0/5] KVM: rename and extend vcpu->requests API

KVM requests are not used with one API, but are a mix of open-coded
setting and kicking, which is what this series aims to normalize.

The main problem is described in comment of patch 1:
There are three main kinds of requests
 1) requests from a VCPU to itself,
 2) requests for a remote VCPU,
 3) and requests for a remote VCPU that also expose some data

and we have just one function for setting requests.

The first type of requests leads to just set_bit.  The other two need to
notify the remote VCPU -- often with kvm_vcpu_kick, but if the VCPU is
known to be halted, it can be just swait_activate().  And the third one
needs a memory barrier before setting the bit and after testing it.

The kvm_make_request function we have now is suboptimal in all of these
three cases -- it starts with a barrier, but then doesn't kick, so we
cannot use it for (1) or (2) due to performance issues and we need to
explicitly kick afterwards in (3).

Putting all these into one function would require runtime overhead as it
is not easy to tell if the request is local or remote, but the need for
a barrier can be decided at compile time.

The result of this series is still a mess as it does not include
kvm_vcpu_kick() into kvm_set_request(), but I hope to get an early
feedback about the idea.

(Series based on current kvm/queue.)

Radim Krčmář (5):
  KVM: change API for requests to match bit operations
  KVM: add KVM request variants without barrier
  KVM: optimize kvm_make_all_cpus_request
  KVM: add __kvm_request_needs_mb
  KVM: add kvm_request_pending

 arch/mips/kvm/emulate.c           |   4 +-
 arch/mips/kvm/trap_emul.c         |   4 +-
 arch/powerpc/kvm/book3s_pr.c      |   4 +-
 arch/powerpc/kvm/book3s_pr_papr.c |   2 +-
 arch/powerpc/kvm/booke.c          |  22 +++---
 arch/powerpc/kvm/powerpc.c        |   8 +--
 arch/s390/kvm/kvm-s390.c          |  26 +++----
 arch/s390/kvm/kvm-s390.h          |   4 +-
 arch/s390/kvm/priv.c              |   4 +-
 arch/x86/kvm/hyperv.c             |  14 ++--
 arch/x86/kvm/i8259.c              |   2 +-
 arch/x86/kvm/lapic.c              |  22 +++---
 arch/x86/kvm/mmu.c                |  14 ++--
 arch/x86/kvm/pmu.c                |   6 +-
 arch/x86/kvm/svm.c                |  12 ++--
 arch/x86/kvm/vmx.c                |  32 ++++-----
 arch/x86/kvm/x86.c                | 144 +++++++++++++++++++-------------------
 include/linux/kvm_host.h          |  96 +++++++++++++++++++++----
 virt/kvm/kvm_main.c               |   9 ++-
 19 files changed, 253 insertions(+), 176 deletions(-)

-- 
2.11.1

[toc] | [next] | [standalone]

#1582671 — [PATCH 5/5] KVM: add kvm_request_pending

Just to complete the encapsulation.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/mips/kvm/trap_emul.c  | 2 +-
 arch/powerpc/kvm/booke.c   | 2 +-
 arch/powerpc/kvm/powerpc.c | 4 ++--
 arch/s390/kvm/kvm-s390.c   | 2 +-
 arch/x86/kvm/x86.c         | 4 ++--
 include/linux/kvm_host.h   | 5 +++++
 6 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/arch/mips/kvm/trap_emul.c b/arch/mips/kvm/trap_emul.c
index 35068823cde6..59e121343170 100644
--- a/arch/mips/kvm/trap_emul.c
+++ b/arch/mips/kvm/trap_emul.c
@@ -1029,7 +1029,7 @@ static void kvm_trap_emul_check_requests(struct kvm_vcpu *vcpu, int cpu,
 	struct mm_struct *mm;
 	int i;
 
-	if (likely(!vcpu->requests))
+	if (likely(!kvm_request_pending(vcpu)))
 		return;
 
 	if (kvm_request_test_and_clear(KVM_REQ_TLB_FLUSH, vcpu)) {
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index e9098af0ab2a..54d2d1cca514 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -682,7 +682,7 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
 
 	kvmppc_core_check_exceptions(vcpu);
 
-	if (vcpu->requests) {
+	if (kvm_request_pending(vcpu)) {
 		/* Exception delivery raised request; start over */
 		return 1;
 	}
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index f570ca9cd8c6..f92d82382da3 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -52,7 +52,7 @@ EXPORT_SYMBOL_GPL(kvmppc_pr_ops);
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
 {
 	return !!(v->arch.pending_exceptions) ||
-	       v->requests;
+	       kvm_request_pending(v);
 }
 
 int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
@@ -104,7 +104,7 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
 		 */
 		smp_mb();
 
-		if (vcpu->requests) {
+		if (kvm_request_pending(vcpu)) {
 			/* Make sure we process requests preemptable */
 			local_irq_enable();
 			trace_kvm_check_requests(vcpu);
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 0fa77e6fdfaf..f2625da20e38 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2393,7 +2393,7 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu)
 {
 retry:
 	kvm_s390_vcpu_request_handled(vcpu);
-	if (!vcpu->requests)
+	if (!kvm_request_pending(vcpu))
 		return 0;
 	/*
 	 * We use MMU_RELOAD just to re-arm the ipte notifier for the
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6e8c62c29a5a..4f325b4eb2cd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6723,7 +6723,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 
 	bool req_immediate_exit = false;
 
-	if (vcpu->requests) {
+	if (kvm_request_pending(vcpu)) {
 		if (kvm_request_test_and_clear(KVM_REQ_MMU_RELOAD, vcpu))
 			kvm_mmu_unload(vcpu);
 		if (kvm_request_test_and_clear(KVM_REQ_MIGRATE_TIMER, vcpu))
@@ -6887,7 +6887,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			kvm_x86_ops->sync_pir_to_irr(vcpu);
 	}
 
-	if (vcpu->mode == EXITING_GUEST_MODE || vcpu->requests
+	if (vcpu->mode == EXITING_GUEST_MODE || kvm_request_pending(vcpu)
 	    || need_resched() || signal_pending(current)) {
 		vcpu->mode = OUTSIDE_GUEST_MODE;
 		smp_wmb();
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 2cc438685af8..563cf964dc5c 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1101,6 +1101,11 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
  * should contain kvm_vcpu_kick().
  */
 
+static inline bool kvm_request_pending(struct kvm_vcpu *vcpu)
+{
+	return vcpu->requests;
+}
+
 static inline void __kvm_request_set(unsigned req, struct kvm_vcpu *vcpu)
 {
 	set_bit(req, &vcpu->requests);
-- 
2.11.1

[toc] | [prev] | [next] | [standalone]

#1582903 — Re: [PATCH 5/5] KVM: add kvm_request_pending

Am 16.02.2017 um 17:04 schrieb Radim Krčmář:
> Just to complete the encapsulation.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David

[toc] | [prev] | [next] | [standalone]

#1583274 — Re: [PATCH 5/5] KVM: add kvm_request_pending

On Thu, Feb 16, 2017 at 05:04:49PM +0100, Radim Krčmář wrote:
...
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 2cc438685af8..563cf964dc5c 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1101,6 +1101,11 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>   * should contain kvm_vcpu_kick().
>   */
>  
> +static inline bool kvm_request_pending(struct kvm_vcpu *vcpu)
> +{
> +	return vcpu->requests;

How about wrapping this with READ_ONCE for good measure?

> +}
> +
>  static inline void __kvm_request_set(unsigned req, struct kvm_vcpu *vcpu)
>  {
>  	set_bit(req, &vcpu->requests);
> -- 
> 2.11.1
>

Thanks,
drew

[toc] | [prev] | [next] | [standalone]

#1583502 — Re: [PATCH 5/5] KVM: add kvm_request_pending

2017-02-17 10:51+0100, Andrew Jones:
> On Thu, Feb 16, 2017 at 05:04:49PM +0100, Radim Krčmář wrote:
> ...
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 2cc438685af8..563cf964dc5c 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -1101,6 +1101,11 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>>   * should contain kvm_vcpu_kick().
>>   */
>>  
>> +static inline bool kvm_request_pending(struct kvm_vcpu *vcpu)
>> +{
>> +	return vcpu->requests;
> 
> How about wrapping this with READ_ONCE for good measure?

Sounds good; I don't think that callers would want stale values, thanks.

[toc] | [prev] | [next] | [standalone]

#1582674 — [PATCH 2/5] KVM: add KVM request variants without barrier

The leading underscores mean that the call is just a bitop wrapper.

Switch all users of open-coded set/check/test to kvm_request ones.
Automated by coccinelle script:
  @@
  expression VCPU, REQ;
  @@
  -set_bit(REQ, &VCPU->requests)
  +__kvm_request_set(REQ, VCPU)

  @@
  expression VCPU, REQ;
  @@
  -clear_bit(REQ, &VCPU->requests)
  +__kvm_request_clear(REQ, VCPU)

  @@
  expression VCPU, REQ;
  @@
  -test_bit(REQ, &VCPU->requests)
  +__kvm_request_test(REQ, VCPU)

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/mips/kvm/emulate.c           |  2 +-
 arch/powerpc/kvm/book3s_pr.c      |  2 +-
 arch/powerpc/kvm/book3s_pr_papr.c |  2 +-
 arch/powerpc/kvm/booke.c          |  4 ++--
 arch/powerpc/kvm/powerpc.c        |  2 +-
 arch/s390/kvm/kvm-s390.c          |  2 +-
 arch/x86/kvm/vmx.c                |  2 +-
 arch/x86/kvm/x86.c                | 14 +++++++-------
 include/linux/kvm_host.h          | 22 +++++++++++++++++++---
 9 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c
index ee4af898bcf6..552ae2b5e911 100644
--- a/arch/mips/kvm/emulate.c
+++ b/arch/mips/kvm/emulate.c
@@ -865,7 +865,7 @@ enum emulation_result kvm_mips_emul_wait(struct kvm_vcpu *vcpu)
 		 * check if any I/O interrupts are pending.
 		 */
 		if (kvm_request_test_and_clear(KVM_REQ_UNHALT, vcpu)) {
-			clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
+			__kvm_request_clear(KVM_REQ_UNHALT, vcpu);
 			vcpu->run->exit_reason = KVM_EXIT_IRQ_WINDOW_OPEN;
 		}
 	}
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index 7af5154e848b..f5894d27a8a9 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -349,7 +349,7 @@ static void kvmppc_set_msr_pr(struct kvm_vcpu *vcpu, u64 msr)
 	if (msr & MSR_POW) {
 		if (!vcpu->arch.pending_exceptions) {
 			kvm_vcpu_block(vcpu);
-			clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
+			__kvm_request_clear(KVM_REQ_UNHALT, vcpu);
 			vcpu->stat.halt_wakeup++;
 
 			/* Unset POW bit after we woke up */
diff --git a/arch/powerpc/kvm/book3s_pr_papr.c b/arch/powerpc/kvm/book3s_pr_papr.c
index f102616febc7..86847220811a 100644
--- a/arch/powerpc/kvm/book3s_pr_papr.c
+++ b/arch/powerpc/kvm/book3s_pr_papr.c
@@ -344,7 +344,7 @@ int kvmppc_h_pr(struct kvm_vcpu *vcpu, unsigned long cmd)
 	case H_CEDE:
 		kvmppc_set_msr_fast(vcpu, kvmppc_get_msr(vcpu) | MSR_EE);
 		kvm_vcpu_block(vcpu);
-		clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
+		__kvm_request_clear(KVM_REQ_UNHALT, vcpu);
 		vcpu->stat.halt_wakeup++;
 		return EMULATE_DONE;
 	case H_LOGICAL_CI_LOAD:
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 806caaf60e10..e9098af0ab2a 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -579,7 +579,7 @@ static void arm_next_watchdog(struct kvm_vcpu *vcpu)
 	 * userspace, so clear the KVM_REQ_WATCHDOG request.
 	 */
 	if ((vcpu->arch.tsr & (TSR_ENW | TSR_WIS)) != (TSR_ENW | TSR_WIS))
-		clear_bit(KVM_REQ_WATCHDOG, &vcpu->requests);
+		__kvm_request_clear(KVM_REQ_WATCHDOG, vcpu);
 
 	spin_lock_irqsave(&vcpu->arch.wdt_lock, flags);
 	nr_jiffies = watchdog_next_timeout(vcpu);
@@ -690,7 +690,7 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
 	if (vcpu->arch.shared->msr & MSR_WE) {
 		local_irq_enable();
 		kvm_vcpu_block(vcpu);
-		clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
+		__kvm_request_clear(KVM_REQ_UNHALT, vcpu);
 		hard_irq_disable();
 
 		kvmppc_set_exit_type(vcpu, EMULATED_MTMSRWE_EXITS);
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 19e89419a843..f570ca9cd8c6 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -231,7 +231,7 @@ int kvmppc_kvm_pv(struct kvm_vcpu *vcpu)
 	case EV_HCALL_TOKEN(EV_IDLE):
 		r = EV_SUCCESS;
 		kvm_vcpu_block(vcpu);
-		clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
+		__kvm_request_clear(KVM_REQ_UNHALT, vcpu);
 		break;
 	default:
 		r = EV_UNIMPLEMENTED;
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index db3c742a5dc9..0fa77e6fdfaf 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2443,7 +2443,7 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu)
 	}
 
 	/* nothing to do, just clear the request */
-	clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
+	__kvm_request_clear(KVM_REQ_UNHALT, vcpu);
 
 	return 0;
 }
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index b183b4ac3ea5..7ab638f2189b 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6352,7 +6352,7 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
 		if (intr_window_requested && vmx_interrupt_allowed(vcpu))
 			return handle_interrupt_window(&vmx->vcpu);
 
-		if (test_bit(KVM_REQ_EVENT, &vcpu->requests))
+		if (__kvm_request_test(KVM_REQ_EVENT, vcpu))
 			return 1;
 
 		err = emulate_instruction(vcpu, EMULTYPE_NO_REEXECUTE);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index da125323682a..6e8c62c29a5a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1767,7 +1767,7 @@ static void kvm_gen_update_masterclock(struct kvm *kvm)
 
 	/* guest entries allowed */
 	kvm_for_each_vcpu(i, vcpu, kvm)
-		clear_bit(KVM_REQ_MCLOCK_INPROGRESS, &vcpu->requests);
+		__kvm_request_clear(KVM_REQ_MCLOCK_INPROGRESS, vcpu);
 
 	spin_unlock(&ka->pvclock_gtod_sync_lock);
 #endif
@@ -2228,8 +2228,8 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			bool tmp = (msr == MSR_KVM_SYSTEM_TIME);
 
 			if (ka->boot_vcpu_runs_old_kvmclock != tmp)
-				set_bit(KVM_REQ_MASTERCLOCK_UPDATE,
-						&vcpu->requests);
+				__kvm_request_set(KVM_REQ_MASTERCLOCK_UPDATE,
+						  vcpu);
 
 			ka->boot_vcpu_runs_old_kvmclock = tmp;
 		}
@@ -2816,7 +2816,7 @@ static bool need_emulate_wbinvd(struct kvm_vcpu *vcpu)
 
 static inline void kvm_migrate_timers(struct kvm_vcpu *vcpu)
 {
-	set_bit(KVM_REQ_MIGRATE_TIMER, &vcpu->requests);
+	__kvm_request_set(KVM_REQ_MIGRATE_TIMER, vcpu);
 }
 
 void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
@@ -7048,7 +7048,7 @@ static int vcpu_run(struct kvm_vcpu *vcpu)
 		if (r <= 0)
 			break;
 
-		clear_bit(KVM_REQ_PENDING_TIMER, &vcpu->requests);
+		__kvm_request_clear(KVM_REQ_PENDING_TIMER, vcpu);
 		if (kvm_cpu_has_pending_timer(vcpu))
 			kvm_inject_pending_timer_irqs(vcpu);
 
@@ -7176,7 +7176,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_UNINITIALIZED)) {
 		kvm_vcpu_block(vcpu);
 		kvm_apic_accept_events(vcpu);
-		clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
+		__kvm_request_clear(KVM_REQ_UNHALT, vcpu);
 		r = -EAGAIN;
 		goto out;
 	}
@@ -8381,7 +8381,7 @@ static inline bool kvm_vcpu_has_events(struct kvm_vcpu *vcpu)
 	if (atomic_read(&vcpu->arch.nmi_queued))
 		return true;
 
-	if (test_bit(KVM_REQ_SMI, &vcpu->requests))
+	if (__kvm_request_test(KVM_REQ_SMI, vcpu))
 		return true;
 
 	if (kvm_arch_interrupt_allowed(vcpu) &&
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 21f91de3098b..d899473859d3 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1100,6 +1100,12 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
  * TODO: the API is inconsistent -- a request doesn't call kvm_vcpu_kick(), but
  * forces smp_wmb() for all requests.
  */
+
+static inline void __kvm_request_set(unsigned req, struct kvm_vcpu *vcpu)
+{
+	set_bit(req, &vcpu->requests);
+}
+
 static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu)
 {
 	/*
@@ -1108,13 +1114,23 @@ static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu)
 	 * Paired with the smp_mb__after_atomic in kvm_request_test_and_clear.
 	 */
 	smp_wmb();
-	set_bit(req, &vcpu->requests);
+	__kvm_request_set(req, vcpu);
+}
+
+static inline bool __kvm_request_test(unsigned req, struct kvm_vcpu *vcpu)
+{
+	return test_bit(req, &vcpu->requests);
+}
+
+static inline void __kvm_request_clear(unsigned req, struct kvm_vcpu *vcpu)
+{
+	test_bit(req, &vcpu->requests);
 }
 
 static inline bool kvm_request_test_and_clear(unsigned req, struct kvm_vcpu *vcpu)
 {
-	if (test_bit(req, &vcpu->requests)) {
-		clear_bit(req, &vcpu->requests);
+	if (__kvm_request_test(req, vcpu)) {
+		__kvm_request_clear(req, vcpu);
 
 		/*
 		 * Ensure the rest of the request is visible to
-- 
2.11.1

[toc] | [prev] | [next] | [standalone]

#1586818 — Re: [PATCH 2/5] KVM: add KVM request variants without barrier

On 16/02/2017 17:04, Radim Krčmář wrote:
> +
> +static inline void __kvm_request_clear(unsigned req, struct kvm_vcpu *vcpu)
> +{
> +	test_bit(req, &vcpu->requests);
>  }

Are you sure? :)

Paolo

>  static inline bool kvm_request_test_and_clear(unsigned req, struct kvm_vcpu *vcpu)
>  {
> -	if (test_bit(req, &vcpu->requests)) {
> -		clear_bit(req, &vcpu->requests);
> +	if (__kvm_request_test(req, vcpu)) {
> +		__kvm_request_clear(req, vcpu);
>  
>  		/*
>  		 * Ensure the rest of the request is visible to
> -- 

[toc] | [prev] | [next] | [standalone]

#1586976 — Re: [PATCH 2/5] KVM: add KVM request variants without barrier

2017-02-23 11:57+0100, Paolo Bonzini:
> On 16/02/2017 17:04, Radim Krčmář wrote:
>> +
>> +static inline void __kvm_request_clear(unsigned req, struct kvm_vcpu *vcpu)
>> +{
>> +	test_bit(req, &vcpu->requests);
>>  }
> 
> Are you sure? :)

No, as always. :)

[toc] | [prev] | [next] | [standalone]

#1582675 — [PATCH 3/5] KVM: optimize kvm_make_all_cpus_request

Use __kvm_request_set to avoid repeated use of wmb().
kvm_make_all_cpus_request is also a candidate for renaming.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 virt/kvm/kvm_main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 934f135c0d23..2250920ec965 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -177,8 +177,12 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
 	zalloc_cpumask_var(&cpus, GFP_ATOMIC);
 
 	me = get_cpu();
+
+	/* Paired with the smp_mb__after_atomic in kvm_request_test_and_clear. */
+	smp_wmb();
+
 	kvm_for_each_vcpu(i, vcpu, kvm) {
-		kvm_request_set(req, vcpu);
+		__kvm_request_set(req, vcpu);
 		cpu = vcpu->cpu;
 
 		/* Set ->requests bit before we read ->mode. */
-- 
2.11.1

[toc] | [prev] | [next] | [standalone]

#1583240 — Re: [PATCH 1/5] KVM: change API for requests to match bit operations

On Thu, 16 Feb 2017 17:04:45 +0100
Radim Krčmář <rkrcmar@redhat.com> wrote:

> kvm_make_request was a wrapper that added barriers to bit_set and
> kvm_check_request did the same for bit_test and bit_check, but the name
> was not very obvious and we were also lacking operations that cover
> bit_test and bit_clear, which resulted in an inconsistent use.
> 
> The renaming:
>   kvm_request_set            <- kvm_make_request
>   kvm_request_test_and_clear <- kvm_check_request
> 
> Automated with coccinelle script:
>   @@
>   expression VCPU, REQ;
>   @@
>   -kvm_make_request(REQ, VCPU)
>   +kvm_request_set(REQ, VCPU)
> 
>   @@
>   expression VCPU, REQ;
>   @@
>   -kvm_check_request(REQ, VCPU)
>   +kvm_request_test_and_clear(REQ, VCPU)

Forgot your s-o-b?

> ---
>  arch/mips/kvm/emulate.c      |   2 +-
>  arch/mips/kvm/trap_emul.c    |   2 +-
>  arch/powerpc/kvm/book3s_pr.c |   2 +-
>  arch/powerpc/kvm/booke.c     |  16 +++---
>  arch/powerpc/kvm/powerpc.c   |   2 +-
>  arch/s390/kvm/kvm-s390.c     |  22 ++++----
>  arch/s390/kvm/kvm-s390.h     |   4 +-
>  arch/s390/kvm/priv.c         |   4 +-
>  arch/x86/kvm/hyperv.c        |  14 ++---
>  arch/x86/kvm/i8259.c         |   2 +-
>  arch/x86/kvm/lapic.c         |  22 ++++----
>  arch/x86/kvm/mmu.c           |  14 ++---
>  arch/x86/kvm/pmu.c           |   6 +-
>  arch/x86/kvm/svm.c           |  12 ++--
>  arch/x86/kvm/vmx.c           |  30 +++++-----
>  arch/x86/kvm/x86.c           | 128 +++++++++++++++++++++----------------------
>  include/linux/kvm_host.h     |  30 ++++++++--
>  virt/kvm/kvm_main.c          |   4 +-
>  18 files changed, 167 insertions(+), 149 deletions(-)

(...lots of coccinelle changes...)

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 8d69d5150748..21f91de3098b 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1084,24 +1084,42 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>  
>  #endif /* CONFIG_HAVE_KVM_EVENTFD */
>  
> -static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
> +/*
> + * An API for setting KVM requests.
> + * The general API design is inspired by bit_* API.
> + *
> + * A request can be set either to itself or to a remote VCPU.  If the request
> + * is set to a remote VCPU, then the VCPU needs to be notified, which is
> + * usually done with kvm_vcpu_kick().
> + * The request can also mean that some data is ready, so a remote requests
> + * needs a smp_wmb().  i.e. there are three types of requests:
> + *  1) local request
> + *  2) remote request with no data (= kick)
> + *  3) remote request with data (= kick + mb)
> + *
> + * TODO: the API is inconsistent -- a request doesn't call kvm_vcpu_kick(), but
> + * forces smp_wmb() for all requests.
> + */
> +static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu)

Should we make req unsigned long as well, so that it matches the bit
api even more?

>  {
>  	/*
> -	 * Ensure the rest of the request is published to kvm_check_request's
> -	 * caller.  Paired with the smp_mb__after_atomic in kvm_check_request.
> +	 * Ensure the rest of the request is published to
> +	 * kvm_request_test_and_clear's caller.
> +	 * Paired with the smp_mb__after_atomic in kvm_request_test_and_clear.
>  	 */
>  	smp_wmb();
>  	set_bit(req, &vcpu->requests);
>  }
>  
> -static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
> +static inline bool kvm_request_test_and_clear(unsigned req, struct kvm_vcpu *vcpu)
>  {
>  	if (test_bit(req, &vcpu->requests)) {
>  		clear_bit(req, &vcpu->requests);
>  
>  		/*
> -		 * Ensure the rest of the request is visible to kvm_check_request's
> -		 * caller.  Paired with the smp_wmb in kvm_make_request.
> +		 * Ensure the rest of the request is visible to
> +		 * kvm_request_test_and_clear's caller.
> +		 * Paired with the smp_wmb in kvm_request_set.
>  		 */
>  		smp_mb__after_atomic();
>  		return true;

[toc] | [prev] | [next] | [standalone]

#1583267 — Re: [PATCH 1/5] KVM: change API for requests to match bit operations

On Fri, Feb 17, 2017 at 10:30:14AM +0100, Cornelia Huck wrote:
> On Thu, 16 Feb 2017 17:04:45 +0100
> Radim Krčmář <rkrcmar@redhat.com> wrote:
> > +static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu)
> 
> Should we make req unsigned long as well, so that it matches the bit
> api even more?

The bitops API is inconsistent among architectures; some are int, some
are unsigned int, some are unsigned long, and x86 is long. If we want
to be consistent with something, then, IMO, we should be consistent with
asm-generic/bitops, which is int, but actually unsigned makes more sense
to me...

Thanks,
drew

[toc] | [prev] | [next] | [standalone]

#1583279 — Re: [PATCH 1/5] KVM: change API for requests to match bit operations

On Fri, 17 Feb 2017 10:49:35 +0100
Andrew Jones <drjones@redhat.com> wrote:

> On Fri, Feb 17, 2017 at 10:30:14AM +0100, Cornelia Huck wrote:
> > On Thu, 16 Feb 2017 17:04:45 +0100
> > Radim Krčmář <rkrcmar@redhat.com> wrote:
> > > +static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu)
> > 
> > Should we make req unsigned long as well, so that it matches the bit
> > api even more?
> 
> The bitops API is inconsistent among architectures; some are int, some
> are unsigned int, some are unsigned long, and x86 is long. If we want
> to be consistent with something, then, IMO, we should be consistent with
> asm-generic/bitops, which is int, but actually unsigned makes more sense
> to me...

Inconsistent interfaces are great :/

Having (any) unsigned value makes the most sense to me as well.

[toc] | [prev] | [next] | [standalone]

#1583505 — Re: [PATCH 1/5] KVM: change API for requests to match bit operations

2017-02-17 10:30+0100, Cornelia Huck:
> On Thu, 16 Feb 2017 17:04:45 +0100
> Radim Krčmář <rkrcmar@redhat.com> wrote:
> 
>> kvm_make_request was a wrapper that added barriers to bit_set and
>> kvm_check_request did the same for bit_test and bit_check, but the name
>> was not very obvious and we were also lacking operations that cover
>> bit_test and bit_clear, which resulted in an inconsistent use.
>> 
>> The renaming:
>>   kvm_request_set            <- kvm_make_request
>>   kvm_request_test_and_clear <- kvm_check_request
>> 
>> Automated with coccinelle script:
>>   @@
>>   expression VCPU, REQ;
>>   @@
>>   -kvm_make_request(REQ, VCPU)
>>   +kvm_request_set(REQ, VCPU)
>> 
>>   @@
>>   expression VCPU, REQ;
>>   @@
>>   -kvm_check_request(REQ, VCPU)
>>   +kvm_request_test_and_clear(REQ, VCPU)
> 
> Forgot your s-o-b?

Oops, thanks.

>> +static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu)
> 
> Should we make req unsigned long as well, so that it matches the bit
> api even more?

From the discussion that followed, I'll keep unsigned.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web