Skip to contentSkip to conversation

KVM: don't sent IPI if the vcpu is not online

PATCH In review Xiao Guangrong 3 participants 11 messages Sep 3, 2010 – Sep 6, 2010 KVM

lore
Share & tools
lore

Xiao Guangrong

author
#0 lore raw

PATCH 1/1 KVM: don't sent IPI if the vcpu is not online

It's no need sent IPI to the vcpu which is schedule out

Signed-off-by Xiao Guangrong <[email protected]>
---
 include/linux/kvm_host.h |    1 +
 virt/kvm/kvm_main.c      |    5 ++++-
 2 files changed, 5 insertions(+), 1 deletions(-)
include/linux/kvm_host.h +1 -0
index f2ecdd5..a2682cf 100644
@@ -86,6 +86,7 @@ struct kvm_vcpu {
 	unsigned long requests;
 	unsigned long guest_debug;
 	int srcu_idx;
+	bool online;
 
 	int fpu_active;
 	int guest_fpu_loaded, guest_xcr0_loaded;
virt/kvm/kvm_main.c +4 -1
index 9a73b98..998cbc8 100644
@@ -148,8 +148,9 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)
 	raw_spin_lock(&kvm->requests_lock);
 	me = smp_processor_id();
 	kvm_for_each_vcpu(i, vcpu, kvm) {
-		if (kvm_make_check_request(req, vcpu))
+		if (kvm_make_check_request(req, vcpu) || !vcpu->online)
 			continue;
+
 		cpu = vcpu->cpu;
 		if (cpus != NULL && cpu != -1 && cpu != me)
 			cpumask_set_cpu(cpu, cpus);

@@ -2245,6 +2246,7 @@ static void kvm_sched_in(struct preempt_notifier *pn, int cpu)
 {
 	struct kvm_vcpu *vcpu = preempt_notifier_to_vcpu(pn);
 
+	vcpu->online = true;
 	kvm_arch_vcpu_load(vcpu, cpu);
 }
 

@@ -2253,6 +2255,7 @@ static void kvm_sched_out(struct preempt_notifier *pn,
 {
 	struct kvm_vcpu *vcpu = preempt_notifier_to_vcpu(pn);
 
+	vcpu->online = false;
 	kvm_arch_vcpu_put(vcpu);
 }
Signature · 1 line
1.7.0.4
#1 lore raw

include/linux/kvm_host.h struct kvm_vcpu { · line 89

Quoted 8 lines · Xiao Guangrong

It's no need sent IPI to the vcpu which is schedule out

include/linux/kvm_host.h +1 -0
@@ -86,6 +86,7 @@ struct kvm_vcpu {
  	unsigned long requests;
  	unsigned long guest_debug;
  	int srcu_idx;
+	bool online;

Why not check for guest_mode instead?

Quoted 8 lines
include/linux/kvm_host.h +0 -0
  	int fpu_active;
  	int guest_fpu_loaded, guest_xcr0_loaded;
virt/kvm/kvm_main.c +0 -0
index 9a73b98..998cbc8 100644

virt/kvm/kvm_main.c static bool make_all_cpus_request(struct kvm *kvm, unsigned int req) · line 156

Quoted 10 lines
virt/kvm/kvm_main.c +2 -1
@@ -148,8 +148,9 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)
  	raw_spin_lock(&kvm->requests_lock);
  	me = smp_processor_id();
  	kvm_for_each_vcpu(i, vcpu, kvm) {
-		if (kvm_make_check_request(req, vcpu))
+		if (kvm_make_check_request(req, vcpu) || !vcpu->online)
  			continue;
+
  		cpu = vcpu->cpu;
  		if (cpus != NULL&&  cpu != -1&&  cpu != me)
  			cpumask_set_cpu(cpu, cpus);

btw, I think a more important optimization would be to drop kvm->requests_lock. But I have no idea how to do it.

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
author
#2 lore raw
Quoted 4 lines · Avi Kivity
Quoted 3 lines · Xiao Guangrong

It's no need sent IPI to the vcpu which is schedule out

Quoted 8 lines

include/linux/kvm_host.h struct kvm_vcpu { · line 89

Quoted 5 lines
include/linux/kvm_host.h +1 -0
@@ -86,6 +86,7 @@ struct kvm_vcpu {
      unsigned long requests;
      unsigned long guest_debug;
      int srcu_idx;
+    bool online;

Why not check for guest_mode instead?

Oh, i forget it...but 'vcpu->guest_mode' is only used in x86 platform, and make_all_cpus_request() is a common function.

So, maybe it's better use 'vcpu->online' here, and move 'guest_mode' into 'vcpu->arch' ?

#3 lore raw
Quoted 12 lines · Xiao Guangrong
Quoted 11 lines · Avi Kivity

include/linux/kvm_host.h struct kvm_vcpu { · line 89

Quoted 8 lines · Xiao Guangrong

It's no need sent IPI to the vcpu which is schedule out

include/linux/kvm_host.h +1 -0
@@ -86,6 +86,7 @@ struct kvm_vcpu {
       unsigned long requests;
       unsigned long guest_debug;
       int srcu_idx;
+    bool online;

Why not check for guest_mode instead?

Quoted 2 lines

Oh, i forget it...but 'vcpu->guest_mode' is only used in x86 platform, and make_all_cpus_request() is a common function.

We can have a function kvm_vcpu_guest_mode() that is defined differently for x86 and the other.

Quoted 2 lines

So, maybe it's better use 'vcpu->online' here, and move 'guest_mode' into 'vcpu->arch' ?

I think guest_mode makes sense for the other archs for reducing IPIs, so let's leave it common and recommend that they implement it. Alex, if you're ever bored.

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
#4 lore raw
Quoted 16 lines · Avi Kivity
Quoted 12 lines · Xiao Guangrong
Quoted 11 lines · Avi Kivity

include/linux/kvm_host.h struct kvm_vcpu { · line 89

Quoted 8 lines · Xiao Guangrong

It's no need sent IPI to the vcpu which is schedule out

include/linux/kvm_host.h +1 -0
@@ -86,6 +86,7 @@ struct kvm_vcpu {
      unsigned long requests;
      unsigned long guest_debug;
      int srcu_idx;
+    bool online;

Why not check for guest_mode instead?

Quoted 2 lines

Oh, i forget it...but 'vcpu->guest_mode' is only used in x86 platform, and make_all_cpus_request() is a common function.

Quoted 6 lines

We can have a function kvm_vcpu_guest_mode() that is defined differently for x86 and the other.

Quoted 2 lines

So, maybe it's better use 'vcpu->online' here, and move 'guest_mode' into 'vcpu->arch' ?

I think guest_mode makes sense for the other archs for reducing IPIs, so let's leave it common and recommend that they implement it. Alex, if you're ever bored.

What does the bit do? Do we have documentation on it ;)? No seriously, what's the intent of the field?

Alex

#5 lore raw
Quoted 4 lines · Alexander Graf
Quoted 2 lines

I think guest_mode makes sense for the other archs for reducing IPIs, so let's leave it common and recommend that they implement it. Alex, if you're ever bored.

What does the bit do? Do we have documentation on it ;)? No seriously, what's the intent of the field?

It indicates that the vcpu is currently executing guest code. Which in turn is important if you need to force it out of guest mode in order to inject an interrupt or flush the tlb.

The procedure is:

remote:
- queue a request in vcpu->requests
- IPI vcpu->cpu (new: only if in guest_mode)

vcpu:
- set guest_mode
- dequeue and execute requests in vcpu->requests
- enter guest
- clear guest_mode

Signature · 1 line
error compiling committee.c: too many arguments to function
#6 lore raw
Quoted 7 lines · Avi Kivity
Quoted 4 lines · Alexander Graf
Quoted 2 lines

I think guest_mode makes sense for the other archs for reducing IPIs, so let's leave it common and recommend that they implement it. Alex, if you're ever bored.

What does the bit do? Do we have documentation on it ;)? No seriously, what's the intent of the field?

It indicates that the vcpu is currently executing guest code. Which in turn is important if you need to force it out of guest mode in order to inject an interrupt or flush the tlb.

Well, a vcpu is either offline in halt state or in guest mode, no? So we can conclude that guest_mode == !offline && !halt_state.

When in halt state, we are active on the wakeup waitqueue:

static void kvmppc_decrementer_func(unsigned long data)

{
        struct kvm_vcpu *vcpu = (struct kvm_vcpu *)data;

        kvmppc_core_queue_dec(vcpu);

        if (waitqueue_active(&vcpu->wq)) {
                wake_up_interruptible(&vcpu->wq);
                vcpu->stat.halt_wakeup++;
        }
}

Shouldn't that be enough information already?

Alex

#7 lore raw
Quoted 8 lines · Alexander Graf
Quoted 5 lines · Avi Kivity
Quoted 3 lines · Alexander Graf
Quoted 1 line

I think guest_mode makes sense for the other archs for reducing IPIs, so let's leave it common and recommend that they implement it. Alex, if you're ever bored.

What does the bit do? Do we have documentation on it ;)? No seriously, what's the intent of the field?

It indicates that the vcpu is currently executing guest code. Which in turn is important if you need to force it out of guest mode in order to inject an interrupt or flush the tlb.

Well, a vcpu is either offline in halt state or in guest mode, no? So we can conclude that guest_mode == !offline&& !halt_state.

It can also be running host kernel or user code.

Quoted 14 lines

When in halt state, we are active on the wakeup waitqueue:

static void kvmppc_decrementer_func(unsigned long data)

{
         struct kvm_vcpu *vcpu = (struct kvm_vcpu *)data;

         kvmppc_core_queue_dec(vcpu);

         if (waitqueue_active(&vcpu->wq)) {
                 wake_up_interruptible(&vcpu->wq);
                 vcpu->stat.halt_wakeup++;
         }
}
Quoted 1 line

Shouldn't that be enough information already?

It's sufficient for correctness. It's not optimal since you miss the cases where you're not running guest code.

Not sure how important it is. Xiao, any numbers?

Signature · 1 line
error compiling committee.c: too many arguments to function
#8 lore raw
Quoted 11 lines · Avi Kivity
Quoted 8 lines · Alexander Graf
Quoted 5 lines · Avi Kivity
Quoted 3 lines · Alexander Graf
Quoted 1 line

I think guest_mode makes sense for the other archs for reducing IPIs, so let's leave it common and recommend that they implement it. Alex, if you're ever bored.

What does the bit do? Do we have documentation on it ;)? No seriously, what's the intent of the field?

It indicates that the vcpu is currently executing guest code. Which in turn is important if you need to force it out of guest mode in order to inject an interrupt or flush the tlb.

Well, a vcpu is either offline in halt state or in guest mode, no? So we can conclude that guest_mode == !offline&& !halt_state.

It can also be running host kernel or user code.

In that case it's the same as running guest code, no? We'll pass by the vcpu entry check asap.

Alex

#9 lore raw
Quoted 3 lines · Alexander Graf
Quoted 2 lines

It can also be running host kernel or user code.

In that case it's the same as running guest code, no? We'll pass by the vcpu entry check asap.

Sure, but the IPI is wasted. If you spend 10% of your time in host code, you can avoid 10% of the IPIs.

(actually less, since the atomic part of the guest switch has guest_mode enabled)

Signature · 1 line
error compiling committee.c: too many arguments to function
#10 lore raw
Quoted 8 lines · Avi Kivity
Quoted 3 lines · Alexander Graf
Quoted 2 lines

It can also be running host kernel or user code.

In that case it's the same as running guest code, no? We'll pass by the vcpu entry check asap.

Sure, but the IPI is wasted. If you spend 10% of your time in host code, you can avoid 10% of the IPIs.

(actually less, since the atomic part of the guest switch has guest_mode enabled)

Oh. My bad. I had a thinko here :).

Sure, sounds like a good idea.

Alex