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

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

[Xiao Guangrong]

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

Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
---
 include/linux/kvm_host.h |    1 +
 virt/kvm/kvm_main.c      |    5 ++++-
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index f2ecdd5..a2682cf 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -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;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 9a73b98..998cbc8 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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);
 }
 
-- 
1.7.0.4

Re: [PATCH] KVM: don't sent IPI if the vcpu is not online

[Avi Kivity]

  On 09/03/2010 07:12 AM, Xiao Guangrong wrote:
> It's no need sent IPI to the vcpu which is schedule out
>
>
> @@ -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?

>
>   	int fpu_active;
>   	int guest_fpu_loaded, guest_xcr0_loaded;
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 9a73b98..998cbc8 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -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.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH] KVM: don't sent IPI if the vcpu is not online

[Xiao Guangrong]

On 09/05/2010 03:18 PM, Avi Kivity wrote:
>  On 09/03/2010 07:12 AM, Xiao Guangrong wrote:
>> It's no need sent IPI to the vcpu which is schedule out
>>
>>
>> @@ -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' ?

Re: [PATCH] KVM: don't sent IPI if the vcpu is not online

[Avi Kivity]

  On 09/06/2010 04:48 AM, Xiao Guangrong wrote:
> On 09/05/2010 03:18 PM, Avi Kivity wrote:
>>   On 09/03/2010 07:12 AM, Xiao Guangrong wrote:
>>> It's no need sent IPI to the vcpu which is schedule out
>>>
>>>
>>> @@ -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.

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

> 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.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH] KVM: don't sent IPI if the vcpu is not online

[Alexander Graf]

On 06.09.2010, at 07:46, Avi Kivity wrote:

> On 09/06/2010 04:48 AM, Xiao Guangrong wrote:
>> On 09/05/2010 03:18 PM, Avi Kivity wrote:
>>>  On 09/03/2010 07:12 AM, Xiao Guangrong wrote:
>>>> It's no need sent IPI to the vcpu which is schedule out
>>>> 
>>>> 
>>>> @@ -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.
> 
> We can have a function kvm_vcpu_guest_mode() that is defined differently for x86 and the other.
> 
>> 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

Re: [PATCH] KVM: don't sent IPI if the vcpu is not online

[Avi Kivity]

  On 09/06/2010 11:51 AM, Alexander Graf wrote:
>
>> 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

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: don't sent IPI if the vcpu is not online

[Alexander Graf]

On 06.09.2010, at 10:55, Avi Kivity wrote:

> On 09/06/2010 11:51 AM, Alexander Graf wrote:
>> 
>>> 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

Re: [PATCH] KVM: don't sent IPI if the vcpu is not online

[Avi Kivity]

  On 09/06/2010 11:59 AM, Alexander Graf wrote:
> On 06.09.2010, at 10:55, Avi Kivity wrote:
>
>> On 09/06/2010 11:51 AM, Alexander Graf wrote:
>>>> 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.

> 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?

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?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: don't sent IPI if the vcpu is not online

[Alexander Graf]

On 06.09.2010, at 11:05, Avi Kivity wrote:

> On 09/06/2010 11:59 AM, Alexander Graf wrote:
>> On 06.09.2010, at 10:55, Avi Kivity wrote:
>> 
>>> On 09/06/2010 11:51 AM, Alexander Graf wrote:
>>>>> 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

Re: [PATCH] KVM: don't sent IPI if the vcpu is not online

[Avi Kivity]

  On 09/06/2010 12:09 PM, Alexander Graf wrote:
>
>> 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)

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: don't sent IPI if the vcpu is not online

[Alexander Graf]

On 06.09.2010, at 11:14, Avi Kivity wrote:

> On 09/06/2010 12:09 PM, Alexander Graf wrote:
>> 
>>> 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