Re: [PATCH] vhost: don't forget to schedule()

[Avi Kivity <avi@redhat.com>]

On 02/28/2012 04:00 PM, Nadav Har'El wrote:
> On Tue, Feb 28, 2012, Avi Kivity wrote about "Re: [PATCH] vhost: don't forget to schedule()":
> > > +			if (need_resched())
> > > +				schedule();
> > 
> > This is cond_resched(), no?
>
> It indeed looks similar, but it appears there are some slightly
> different things happening in both cases, especially for a preemptive
> kernel... Unfortunately, I am not astute (or experienced) enough to tell 
> which of the two idioms are better or more appropriate for this case.

I'd have expected that cond_resched() is a no-op with preemptible
kernels, but I see this is not the case.

>
> The idiom that I used seemed right, and seemed to work in my tests.
> Moreover I also noticed it was used in vmx.c. Also, vhost.c was already
> calling schedule(), not cond_resched(), so I thought it made sense to
> call the same thing...
>
> But I now see that in kvm_main.c, there's also this:
>
>         if (!need_resched())
>                 return;
>         cond_resched();
>
> Which seems to combine both idioms ;-) Can anybody shed a light on what
> is the right way to do it?
>

It's bogus.  Look at commit 3fca03653010:

Author: Yaozu Dong <eddie.dong@intel.com>
Date:   Wed Apr 25 16:49:19 2007 +0300

    KVM: VMX: Avoid unnecessary vcpu_load()/vcpu_put() cycles
   
    By checking if a reschedule is needed, we avoid dropping the vcpu.
   
    [With changes by me, based on Anthony Liguori's observations]
   
    Signed-off-by: Avi Kivity <avi@qumranet.com>

diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index 03c0ee7..f535635 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -1590,6 +1590,8 @@ static int set_msr(struct kvm_vcpu *vcpu, u32
msr_index, u64 data)
 
 void kvm_resched(struct kvm_vcpu *vcpu)
 {
+       if (!need_resched())
+               return;
        vcpu_put(vcpu);
        cond_resched();
        vcpu_load(vcpu);

at that time, it made sense to do the extra check to avoid the expensive
vcpu_put/vcpu_load.  Later preempt notifiers made them redundant
(15ad71460d75), and they were removed, but the extra check remained.

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