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

Groups > linux.kernel > #1618330 > unrolled thread

[PATCH 0/6] KVM: towards maintainable kvm_make_all_cpus_request()

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] KVM: towards maintainable kvm_make_all_cpus_request() Radim Krčmář <rkrcmar@redhat.com> - 2017-04-06 22:30 +0200
    [PATCH 4/6] KVM: remove #ifndef CONFIG_S390 around kvm_vcpu_wake_up Radim Krčmář <rkrcmar@redhat.com> - 2017-04-06 22:30 +0200
      Re: [PATCH 4/6] KVM: remove #ifndef CONFIG_S390 around  kvm_vcpu_wake_up Christian Borntraeger <borntraeger@de.ibm.com> - 2017-04-07 13:10 +0200
    [PATCH RFC 1/6] KVM: fix guest_mode optimization in kvm_make_all_cpus_request() Radim Krčmář <rkrcmar@redhat.com> - 2017-04-06 22:30 +0200
      Re: [PATCH RFC 1/6] KVM: fix guest_mode optimization in  kvm_make_all_cpus_request() James Hogan <james.hogan@imgtec.com> - 2017-04-06 23:10 +0200
      Re: [PATCH RFC 1/6] KVM: fix guest_mode optimization in  kvm_make_all_cpus_request() Christian Borntraeger <borntraeger@de.ibm.com> - 2017-04-07 12:50 +0200
    [PATCH 3/6] KVM: x86: use kvm_make_request instead of set_bit Radim Krčmář <rkrcmar@redhat.com> - 2017-04-06 22:30 +0200
      Re: [PATCH 3/6] KVM: x86: use kvm_make_request instead of set_bit David Hildenbrand <david@redhat.com> - 2017-04-07 10:20 +0200
    [PATCH 6/6] KVM: perform a wake_up in kvm_make_all_cpus_request Radim Krčmář <rkrcmar@redhat.com> - 2017-04-06 22:30 +0200
      Re: [PATCH 6/6] KVM: perform a wake_up in kvm_make_all_cpus_request Andrew Jones <drjones@redhat.com> - 2017-04-10 13:20 +0200
    [PATCH 2/6] KVM: use kvm_{test,clear}_request instead of {test,clear}_bit Radim Krčmář <rkrcmar@redhat.com> - 2017-04-06 22:30 +0200
      Re: [PATCH 2/6] KVM: use kvm_{test,clear}_request instead of  {test,clear}_bit Christian Borntraeger <borntraeger@de.ibm.com> - 2017-04-07 13:00 +0200
        Re: [PATCH 2/6] KVM: use kvm_{test,clear}_request instead of  {test,clear}_bit Radim Krčmář <rkrcmar@redhat.com> - 2017-04-07 14:30 +0200
          Re: [PATCH 2/6] KVM: use kvm_{test,clear}_request instead of  {test,clear}_bit Radim Krčmář <rkrcmar@redhat.com> - 2017-04-07 16:10 +0200

#1618330 — [PATCH 0/6] KVM: towards maintainable kvm_make_all_cpus_request()

[1/6] makes a significant change for s390 and might be too dangerous
because of that.
I'm ok with returning 0 from s390's kvm_arch_vcpu_should_kick() until we
sort out architecture-specific kicks.

Adding kvm_vcpu_wake_up() in [6/6] is the reason why the other patches
were included.

Compile tested on s390, lightly tested and checked that kvm_*_request()
uses are being optimized on x86.


Radim Krčmář (6):
  KVM: fix guest_mode optimization in kvm_make_all_cpus_request()
  KVM: use kvm_{test,clear}_request instead of {test,clear}_bit
  KVM: x86: use kvm_make_request instead of set_bit
  KVM: remove #ifndef CONFIG_S390 around kvm_vcpu_wake_up
  KVM: mark requests that do not need a wakeup
  KVM: perform a wake_up in kvm_make_all_cpus_request

 arch/arm/include/asm/kvm_host.h   |  2 +-
 arch/arm64/include/asm/kvm_host.h |  2 +-
 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          |  6 ++----
 arch/x86/include/asm/kvm_host.h   |  6 +++---
 arch/x86/kvm/vmx.c                |  2 +-
 arch/x86/kvm/x86.c                | 18 ++++++------------
 include/linux/kvm_host.h          | 22 +++++++++++++++++-----
 virt/kvm/kvm_main.c               |  7 ++++---
 13 files changed, 41 insertions(+), 36 deletions(-)

-- 
2.12.0

[toc] | [next] | [standalone]

#1618331 — [PATCH 4/6] KVM: remove #ifndef CONFIG_S390 around kvm_vcpu_wake_up

The #ifndef was protecting a missing halt_wakeup stat, but that is no
longer necessary.

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

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 2389e9c41cd2..a486c6ad27a6 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2195,7 +2195,6 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_block);
 
-#ifndef CONFIG_S390
 void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
 {
 	struct swait_queue_head *wqp;
@@ -2225,7 +2224,6 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
 	put_cpu();
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_kick);
-#endif /* !CONFIG_S390 */
 
 int kvm_vcpu_yield_to(struct kvm_vcpu *target)
 {
-- 
2.12.0

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

#1618679 — Re: [PATCH 4/6] KVM: remove #ifndef CONFIG_S390 around kvm_vcpu_wake_up

On 04/06/2017 10:20 PM, Radim Krčmář wrote:
> The #ifndef was protecting a missing halt_wakeup stat, but that is no
> longer necessary.


Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  virt/kvm/kvm_main.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 2389e9c41cd2..a486c6ad27a6 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2195,7 +2195,6 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
>  }
>  EXPORT_SYMBOL_GPL(kvm_vcpu_block);
> 
> -#ifndef CONFIG_S390
>  void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
>  {
>  	struct swait_queue_head *wqp;
> @@ -2225,7 +2224,6 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
>  	put_cpu();
>  }
>  EXPORT_SYMBOL_GPL(kvm_vcpu_kick);
> -#endif /* !CONFIG_S390 */
> 
>  int kvm_vcpu_yield_to(struct kvm_vcpu *target)
>  {
> 

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

#1618334 — [PATCH RFC 1/6] KVM: fix guest_mode optimization in kvm_make_all_cpus_request()

We have kvm_arch_vcpu_should_kick() to decide whether the target cpu
needs to be kicked.  The previous condition was wrong, because
architectures that don't use vcpu->mode would not get interrupts and
also suboptimal, because it sent IPI in cases where none was necessary.

The situation is even more convoluted.  MIPS and POWERPC return 1 from
kvm_arch_vcpu_should_kick(), but implement vcpu->mode for some reason,
so now they might kick uselessly.  This is not a huge problem.

s390, on the other hand, never changed vcpu->mode, so it would always be
OUTSIDE_GUEST_MODE before and therefore didn't send IPIs.
I don't see a reason why s390 had kvm_make_all_cpus_request() that did
nothing but set the bit in vcpu->request, so the new behavior seems
better.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/s390/kvm/kvm-s390.c | 4 +---
 virt/kvm/kvm_main.c      | 2 +-
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index fd6cd05bb6a7..45b6d9ca5d24 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2130,9 +2130,7 @@ static void kvm_gmap_notifier(struct gmap *gmap, unsigned long start,
 
 int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
 {
-	/* kvm common code refers to this, but never calls it */
-	BUG();
-	return 0;
+	return 1;
 }
 
 static int kvm_arch_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu,
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index f489167839c4..2389e9c41cd2 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -187,7 +187,7 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
 		smp_mb__after_atomic();
 
 		if (cpus != NULL && cpu != -1 && cpu != me &&
-		      kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
+				kvm_arch_vcpu_should_kick(vcpu))
 			cpumask_set_cpu(cpu, cpus);
 	}
 	if (unlikely(cpus == NULL))
-- 
2.12.0

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

#1618348 — Re: [PATCH RFC 1/6] KVM: fix guest_mode optimization in kvm_make_all_cpus_request()

[Multipart message — attachments visible in raw view] — view raw

On Thu, Apr 06, 2017 at 10:20:51PM +0200, Radim Krčmář wrote:
> We have kvm_arch_vcpu_should_kick() to decide whether the target cpu
> needs to be kicked.  The previous condition was wrong, because
> architectures that don't use vcpu->mode would not get interrupts and
> also suboptimal, because it sent IPI in cases where none was necessary.
> 
> The situation is even more convoluted.  MIPS and POWERPC return 1 from
> kvm_arch_vcpu_should_kick(), but implement vcpu->mode for some reason,
> so now they might kick uselessly.  This is not a huge problem.

Whoops. I hadn't spotted kvm_arch_vcpu_should_kick() when I added
vcpu->mode stuff in 4.11... I'm guessing I need to implement that
similar to ARM / x86... though MIPS doesn't use kvm_vcpu_kick() yet.

> 
> s390, on the other hand, never changed vcpu->mode, so it would always be
> OUTSIDE_GUEST_MODE before and therefore didn't send IPIs.
> I don't see a reason why s390 had kvm_make_all_cpus_request() that did
> nothing but set the bit in vcpu->request, so the new behavior seems
> better.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 4 +---
>  virt/kvm/kvm_main.c      | 2 +-
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index fd6cd05bb6a7..45b6d9ca5d24 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2130,9 +2130,7 @@ static void kvm_gmap_notifier(struct gmap *gmap, unsigned long start,
>  
>  int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
>  {
> -	/* kvm common code refers to this, but never calls it */
> -	BUG();
> -	return 0;
> +	return 1;
>  }
>  
>  static int kvm_arch_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu,
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index f489167839c4..2389e9c41cd2 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -187,7 +187,7 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
>  		smp_mb__after_atomic();
>  
>  		if (cpus != NULL && cpu != -1 && cpu != me &&
> -		      kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
> +				kvm_arch_vcpu_should_kick(vcpu))

This presumably changes the behaviour on x86, from != OUTSIDE_GUEST_MODE
to == IN_GUEST_MODE. so:
- you'll no longer get IPIs if its in READING_SHADOW_PAGE_TABLES (which
  MIPS also now uses when accessing mappings outside of guest mode and
  depends upon to wait until the old mappings are no longer in use).
- you'll no longer get IPIs if its in EXITING_GUEST_MODE (i.e. if you
  get two of these in quick succession only the first will wait for the
  IPI, which might work as long as they're already serialised but it
  still feels wrong).

But it doesn't seem right to change the kvm_arch_vcpu_should_kick()
implementations to check kvm_vcpu_exiting_guest_mode(vcpu) !=
OUTSIDE_GUEST_MODE to match condition either, since kvm_vcpu_kick()
doesn't seem to need synchronisation, only to know that it won't be
delayed in reaching hypervisor code.

Cheers
James


>  			cpumask_set_cpu(cpu, cpus);
>  	}
>  	if (unlikely(cpus == NULL))
> -- 
> 2.12.0
> 

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

#1618671 — Re: [PATCH RFC 1/6] KVM: fix guest_mode optimization in kvm_make_all_cpus_request()

On 04/06/2017 10:20 PM, Radim Krčmář wrote:
> We have kvm_arch_vcpu_should_kick() to decide whether the target cpu
> needs to be kicked.  The previous condition was wrong, because
> architectures that don't use vcpu->mode would not get interrupts and
> also suboptimal, because it sent IPI in cases where none was necessary.
> 
> The situation is even more convoluted.  MIPS and POWERPC return 1 from
> kvm_arch_vcpu_should_kick(), but implement vcpu->mode for some reason,
> so now they might kick uselessly.  This is not a huge problem.
> 
> s390, on the other hand, never changed vcpu->mode, so it would always be
> OUTSIDE_GUEST_MODE before and therefore didn't send IPIs.
> I don't see a reason why s390 had kvm_make_all_cpus_request() that did
> nothing but set the bit in vcpu->request, so the new behavior seems
> better.

As on s390 nobody ever called kvm_make_all_cpus_request this patch should be fine
for s390. But even if somebody would start calling kvm_make_all_cpus_requests
this should do what we want (as long as we do not need the special "make sure
to be really out of guest" thing).

Not sure about the x86 things that James mentioned. 
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 4 +---
>  virt/kvm/kvm_main.c      | 2 +-
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index fd6cd05bb6a7..45b6d9ca5d24 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2130,9 +2130,7 @@ static void kvm_gmap_notifier(struct gmap *gmap, unsigned long start,
> 
>  int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
>  {
> -	/* kvm common code refers to this, but never calls it */
> -	BUG();
> -	return 0;
> +	return 1;
>  }
> 
>  static int kvm_arch_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu,
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index f489167839c4..2389e9c41cd2 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -187,7 +187,7 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
>  		smp_mb__after_atomic();
> 
>  		if (cpus != NULL && cpu != -1 && cpu != me &&
> -		      kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
> +				kvm_arch_vcpu_should_kick(vcpu))
>  			cpumask_set_cpu(cpu, cpus);
>  	}
>  	if (unlikely(cpus == NULL))
> 

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

#1618335 — [PATCH 3/6] KVM: x86: use kvm_make_request instead of set_bit

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/x86.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 71a019832df9..57e9989232e5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2229,8 +2229,7 @@ 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_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
 
 			ka->boot_vcpu_runs_old_kvmclock = tmp;
 		}
@@ -2803,11 +2802,6 @@ static bool need_emulate_wbinvd(struct kvm_vcpu *vcpu)
 	return kvm_arch_has_noncoherent_dma(vcpu->kvm);
 }
 
-static inline void kvm_migrate_timers(struct kvm_vcpu *vcpu)
-{
-	set_bit(KVM_REQ_MIGRATE_TIMER, &vcpu->requests);
-}
-
 void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 {
 	/* Address WBINVD may be executed by guest */
@@ -2851,7 +2845,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 		if (!vcpu->kvm->arch.use_master_clock || vcpu->cpu == -1)
 			kvm_make_request(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu);
 		if (vcpu->cpu != cpu)
-			kvm_migrate_timers(vcpu);
+			kvm_make_request(KVM_REQ_MIGRATE_TIMER, vcpu);
 		vcpu->cpu = cpu;
 	}
 
-- 
2.12.0

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

#1618569 — Re: [PATCH 3/6] KVM: x86: use kvm_make_request instead of set_bit

On 06.04.2017 22:20, Radim Krčmář wrote:
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 71a019832df9..57e9989232e5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2229,8 +2229,7 @@ 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_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
>  
>  			ka->boot_vcpu_runs_old_kvmclock = tmp;
>  		}
> @@ -2803,11 +2802,6 @@ static bool need_emulate_wbinvd(struct kvm_vcpu *vcpu)
>  	return kvm_arch_has_noncoherent_dma(vcpu->kvm);
>  }
>  
> -static inline void kvm_migrate_timers(struct kvm_vcpu *vcpu)
> -{
> -	set_bit(KVM_REQ_MIGRATE_TIMER, &vcpu->requests);
> -}
> -
>  void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  {
>  	/* Address WBINVD may be executed by guest */
> @@ -2851,7 +2845,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  		if (!vcpu->kvm->arch.use_master_clock || vcpu->cpu == -1)
>  			kvm_make_request(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu);
>  		if (vcpu->cpu != cpu)
> -			kvm_migrate_timers(vcpu);
> +			kvm_make_request(KVM_REQ_MIGRATE_TIMER, vcpu);
>  		vcpu->cpu = cpu;
>  	}
>  
> 

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

-- 

Thanks,

David

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

#1618337 — [PATCH 6/6] KVM: perform a wake_up in kvm_make_all_cpus_request

We want to have kvm_make_all_cpus_request() to be an optmized version of

  kvm_for_each_vcpu(i, vcpu, kvm) {
    kvm_make_request(vcpu, request);
    kvm_vcpu_kick(vcpu);
  }

and kvm_vcpu_kick() wakes up the target vcpu.  We know which requests do
not need the wake up and use it to optimize the loop.

Thanks to that, this patch doesn't change the behavior of current users
(the all don't need the wake up) and only prepares for future where the
wake up is going to be needed.

I think that most requests do not need the wake up, so we would flip the
bit then.

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

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a486c6ad27a6..1db503bab3dc 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -186,6 +186,9 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
 		/* Set ->requests bit before we read ->mode. */
 		smp_mb__after_atomic();
 
+		if (!(req & KVM_REQUEST_NO_WAKEUP))
+			kvm_vcpu_wake_up(vcpu);
+
 		if (cpus != NULL && cpu != -1 && cpu != me &&
 				kvm_arch_vcpu_should_kick(vcpu))
 			cpumask_set_cpu(cpu, cpus);
-- 
2.12.0

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

#1619823 — Re: [PATCH 6/6] KVM: perform a wake_up in kvm_make_all_cpus_request

On Thu, Apr 06, 2017 at 10:20:56PM +0200, Radim Krčmář wrote:
> We want to have kvm_make_all_cpus_request() to be an optmized version of
> 
>   kvm_for_each_vcpu(i, vcpu, kvm) {
>     kvm_make_request(vcpu, request);
>     kvm_vcpu_kick(vcpu);
>   }
> 
> and kvm_vcpu_kick() wakes up the target vcpu.  We know which requests do
> not need the wake up and use it to optimize the loop.

Any reason we don't want kvm_vcpu_kick() to also get the
if (!(req & KVM_REQUEST_NO_WAKEUP)) optimization condition?  I did some
grepping, and don't see any kicks of the requests that have been marked as
NO_WAKEUP, so nothing should change by adding it now.  But the consistency
would be nice for the doc I'm writing.

Also, the condition in kvm_vcpu_kick() looks like overkill

 cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu)

How could vcpu->cpu ever be any offline/invalid cpu, other than -1?  The
condition in kvm_make_all_cpus_request() makes more sense to me

 cpu != -1 && cpu != me

I guess a lot this stuff is planned for a larger requests rework, when
kicks get integrated with requests?  I'm a bit anxious, though, as it
changes how I document stuff now, and even how I approach the ARM series.
For example, if kvm_make_request() already integrated kvm_vcpu_kick(),
which means also adding the smp_mb__after_atomic(), like
kvm_make_all_cpus_request() has, then I wouldn't need to add the smp_mb()
to kvm_arch_vcpu_should_kick().

Thanks,
drew

> 
> Thanks to that, this patch doesn't change the behavior of current users
> (the all don't need the wake up) and only prepares for future where the
> wake up is going to be needed.
> 
> I think that most requests do not need the wake up, so we would flip the
> bit then.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  virt/kvm/kvm_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index a486c6ad27a6..1db503bab3dc 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -186,6 +186,9 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
>  		/* Set ->requests bit before we read ->mode. */
>  		smp_mb__after_atomic();
>  
> +		if (!(req & KVM_REQUEST_NO_WAKEUP))
> +			kvm_vcpu_wake_up(vcpu);
> +
>  		if (cpus != NULL && cpu != -1 && cpu != me &&
>  				kvm_arch_vcpu_should_kick(vcpu))
>  			cpumask_set_cpu(cpu, cpus);
> -- 
> 2.12.0
> 

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

#1618338 — [PATCH 2/6] KVM: use kvm_{test,clear}_request instead of {test,clear}_bit

Users were expected to use kvm_check_request() for testing and clearing,
but request have expanded their use since then and some users want to
only test or do a faster clear.

Make sure that requests are not directly accessed with bit operations, because
we'll be clearing them later.

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                |  8 ++++----
 include/linux/kvm_host.h          | 14 ++++++++++++--
 9 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c
index d40cfaad4529..aee5ba5840af 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_check_request(KVM_REQ_UNHALT, vcpu)) {
-			clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
+			kvm_clear_request(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 d4dfc0ca2a44..d8ace42f34a6 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_clear_request(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..bcbeeb62dd13 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_clear_request(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 0514cbd4e533..ab968f60d14c 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_clear_request(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_clear_request(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 0e42aa8a279f..63bdf14d4389 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -232,7 +232,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_clear_request(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 45b6d9ca5d24..ead4b476cce9 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2444,7 +2444,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_clear_request(KVM_REQ_UNHALT, vcpu);
 
 	return 0;
 }
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index cfdb0d9389d1..24cb416aa84f 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6317,7 +6317,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_test_request(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 6bc47e2712c8..71a019832df9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1768,7 +1768,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_clear_request(KVM_REQ_MCLOCK_INPROGRESS, vcpu);
 
 	spin_unlock(&ka->pvclock_gtod_sync_lock);
 #endif
@@ -7045,7 +7045,7 @@ static int vcpu_run(struct kvm_vcpu *vcpu)
 		if (r <= 0)
 			break;
 
-		clear_bit(KVM_REQ_PENDING_TIMER, &vcpu->requests);
+		kvm_clear_request(KVM_REQ_PENDING_TIMER, vcpu);
 		if (kvm_cpu_has_pending_timer(vcpu))
 			kvm_inject_pending_timer_irqs(vcpu);
 
@@ -7173,7 +7173,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_clear_request(KVM_REQ_UNHALT, vcpu);
 		r = -EAGAIN;
 		goto out;
 	}
@@ -8383,7 +8383,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_test_request(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 7e74ae4d99bb..51737d6401ae 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1079,10 +1079,20 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
 	set_bit(req, &vcpu->requests);
 }
 
+static inline bool kvm_test_request(int req, struct kvm_vcpu *vcpu)
+{
+	return test_bit(req, &vcpu->requests);
+}
+
+static inline void kvm_clear_request(int req, struct kvm_vcpu *vcpu)
+{
+	clear_bit(req, &vcpu->requests);
+}
+
 static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
 {
-	if (test_bit(req, &vcpu->requests)) {
-		clear_bit(req, &vcpu->requests);
+	if (kvm_test_request(req, vcpu)) {
+		kvm_clear_request(req, vcpu);
 
 		/*
 		 * Ensure the rest of the request is visible to kvm_check_request's
-- 
2.12.0

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

#1618676 — Re: [PATCH 2/6] KVM: use kvm_{test,clear}_request instead of {test,clear}_bit

On 04/06/2017 10:20 PM, Radim Krčmář wrote:
> Users were expected to use kvm_check_request() for testing and clearing,
> but request have expanded their use since then and some users want to
> only test or do a faster clear.
> 
> Make sure that requests are not directly accessed with bit operations, because
> we'll be clearing them later.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>

Patch itself looks sane
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>


one question:
>  static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
>  {
> -	if (test_bit(req, &vcpu->requests)) {
> -		clear_bit(req, &vcpu->requests);
> +	if (kvm_test_request(req, vcpu)) {
> +		kvm_clear_request(req, vcpu);

This looks fine. I am just asking myself why we do not use
test_and_clear_bit? Do we expect gcc to merge all test bits as
a fast path? This does not seem to work as far as I can tell and
almost everybody does a fast path like in



arch/s390/kvm/kvm-s390.c:
        if (!vcpu->requests)
                return 0;

arch/x86/kvm/x86.c:
    if (vcpu->requests) {

> 
>  		/*
>  		 * Ensure the rest of the request is visible to kvm_check_request's
> 

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

#1618728 — Re: [PATCH 2/6] KVM: use kvm_{test,clear}_request instead of {test,clear}_bit

2017-04-07 12:55+0200, Christian Borntraeger:
> On 04/06/2017 10:20 PM, Radim Krčmář wrote:
>>  static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
>>  {
>> -	if (test_bit(req, &vcpu->requests)) {
>> -		clear_bit(req, &vcpu->requests);
>> +	if (kvm_test_request(req, vcpu)) {
>> +		kvm_clear_request(req, vcpu);
> 
> This looks fine. I am just asking myself why we do not use
> test_and_clear_bit? Do we expect gcc to merge all test bits as
> a fast path? This does not seem to work as far as I can tell and
> almost everybody does a fast path like in

test_and_clear_bit() is a slower operation even if the test is false (at
least on x86), because it needs to be fully atomic.

> arch/s390/kvm/kvm-s390.c:
>         if (!vcpu->requests)
>                 return 0;
> 
> arch/x86/kvm/x86.c:
>     if (vcpu->requests) {

We'll mostly have only one request set, so splitting the test_and_clear
improves the performance of many subsequent tests_and_clear()s even if
the compiler doesn't optimize.

GCC couldn't even optimize if we used test_and_clear_bit(), because that
instruction adds barriers, but the forward check for vcpu->requests is
there because we do not trust the optimizer to do it for us and it would
make a big difference.

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

#1618829 — Re: [PATCH 2/6] KVM: use kvm_{test,clear}_request instead of {test,clear}_bit

2017-04-07 14:24+0200, Radim Krčmář:
> 2017-04-07 12:55+0200, Christian Borntraeger:
>> On 04/06/2017 10:20 PM, Radim Krčmář wrote:
>>>  static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
>>>  {
>>> -	if (test_bit(req, &vcpu->requests)) {
>>> -		clear_bit(req, &vcpu->requests);
>>> +	if (kvm_test_request(req, vcpu)) {
>>> +		kvm_clear_request(req, vcpu);
>> 
>> This looks fine. I am just asking myself why we do not use
>> test_and_clear_bit? Do we expect gcc to merge all test bits as
>> a fast path? This does not seem to work as far as I can tell and
>> almost everybody does a fast path like in
> 
> test_and_clear_bit() is a slower operation even if the test is false (at
> least on x86), because it needs to be fully atomic.
> 
>> arch/s390/kvm/kvm-s390.c:
>>         if (!vcpu->requests)
>>                 return 0;
>> 
>> arch/x86/kvm/x86.c:
>>     if (vcpu->requests) {
> 
> We'll mostly have only one request set, so splitting the test_and_clear
> improves the performance of many subsequent tests_and_clear()s even if
> the compiler doesn't optimize.
> 
> GCC couldn't even optimize if we used test_and_clear_bit(), because that
> instruction adds barriers, but the forward check for vcpu->requests is
> there because we do not trust the optimizer to do it for us and it would
> make a big difference.

Ugh, I started thinking that bitops are not atomic because I looked at
wrong boot/bitops.h by mistake.  The compiler cannot merge test_bit()s,
but the speed difference holds.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web