[PATCH] Add need_resched check before go to kvm_resched

[PATCH] Add need_resched check before go to kvm_resched

[Dong, Eddie]

[-- Attachment #1: Type: text/plain, Size: 460 bytes --]

A minor change to reduce vcpu_put/vcpu_load frequency (still base on
KVM-18). Not sure if you would like to see this?

Signed-off-by: Yaozu Dong <eddie.dong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

--- vmx.old	2007-04-25 20:28:19.000000000 +0800
+++ vmx.new	2007-04-25 20:28:10.000000000 +0800
@@ -1945,7 +1945,8 @@
 				return -EINTR;
 			}
 
-			kvm_resched(vcpu);
+			if ( need_resched() )
+				kvm_resched(vcpu);
 			goto again;
 		}
 	}

[-- Attachment #2: kvm_sched.diff --]
[-- Type: application/octet-stream, Size: 244 bytes --]

--- vmx.old	2007-04-25 20:28:19.000000000 +0800
+++ vmx.new	2007-04-25 20:28:10.000000000 +0800
@@ -1945,7 +1945,8 @@
 				return -EINTR;
 			}
 
-			kvm_resched(vcpu);
+			if ( need_resched() )
+				kvm_resched(vcpu);
 			goto again;
 		}
 	}

[-- Attachment #3: Type: text/plain, Size: 286 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] Add need_resched check before go to kvm_resched

[Avi Kivity]

Dong, Eddie wrote:
> A minor change to reduce vcpu_put/vcpu_load frequency (still base on
> KVM-18). Not sure if you would like to see this?
>
> Signed-off-by: Yaozu Dong <eddie.dong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
> --- vmx.old	2007-04-25 20:28:19.000000000 +0800
> +++ vmx.new	2007-04-25 20:28:10.000000000 +0800
> @@ -1945,7 +1945,8 @@
>  				return -EINTR;
>  			}
>  
> -			kvm_resched(vcpu);
> +			if ( need_resched() )
> +				kvm_resched(vcpu);
>  			goto again;
>  		}
>  	}
>   

This is good, especially if we move some msrs to vcpu_load().

But please, no spaces around need_resched().


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


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] Add need_resched check before go to kvm_resched

[Dong, Eddie]

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]

Avi Kivity wrote:
> Dong, Eddie wrote:
>> A minor change to reduce vcpu_put/vcpu_load frequency (still base on
>> KVM-18). Not sure if you would like to see this?
>> 
>> Signed-off-by: Yaozu Dong <eddie.dong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> 
>> --- vmx.old	2007-04-25 20:28:19.000000000 +0800
>> +++ vmx.new	2007-04-25 20:28:10.000000000 +0800
>> @@ -1945,7 +1945,8 @@
>>  				return -EINTR;
>>  			}
>> 
>> -			kvm_resched(vcpu);
>> +			if ( need_resched() )
>> +				kvm_resched(vcpu);
>>  			goto again;
>>  		}
>>  	}
>> 
> 
> This is good, especially if we move some msrs to vcpu_load().
> 
> But please, no spaces around need_resched().

Here it is.
Thx,eddie

[-- Attachment #2: p3 --]
[-- Type: application/octet-stream, Size: 242 bytes --]

--- vmx.old	2007-04-25 20:28:19.000000000 +0800
+++ vmx.new	2007-04-25 21:14:24.000000000 +0800
@@ -1945,7 +1945,8 @@
 				return -EINTR;
 			}
 
-			kvm_resched(vcpu);
+			if (need_resched())
+				kvm_resched(vcpu);
 			goto again;
 		}
 	}

[-- Attachment #3: Type: text/plain, Size: 286 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] Add need_resched check before go to kvm_resched

[Avi Kivity]

Dong, Eddie wrote:
> Here it is.
>   

Applied, thanks.

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


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] Add need_resched check before go to kvm_resched

[Anthony Liguori]

Dong, Eddie wrote:
> A minor change to reduce vcpu_put/vcpu_load frequency (still base on
> KVM-18). Not sure if you would like to see this?
>
> Signed-off-by: Yaozu Dong <eddie.dong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
> --- vmx.old	2007-04-25 20:28:19.000000000 +0800
> +++ vmx.new	2007-04-25 20:28:10.000000000 +0800
> @@ -1945,7 +1945,8 @@
>  				return -EINTR;
>  			}
>  
> -			kvm_resched(vcpu);
> +			if ( need_resched() )
> +				kvm_resched(vcpu);
>  			goto again;
>  		}
>  	}

This should get moved to kvm_resched() since both VT/SVM would benefit 
from this.

Regards,

Anthony Liguori

> ------------------------------------------------------------------------
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by DB2 Express
> Download DB2 Express C - the FREE version of DB2 express and take
> control of your XML. No limits. Just data. Click to get it now.
> http://sourceforge.net/powerbar/db2/
> ------------------------------------------------------------------------
>
> _______________________________________________
> kvm-devel mailing list
> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>   


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] Add need_resched check before go to kvm_resched

[Avi Kivity]

Anthony Liguori wrote:
> Dong, Eddie wrote:
>> A minor change to reduce vcpu_put/vcpu_load frequency (still base on
>> KVM-18). Not sure if you would like to see this?
>>
>> Signed-off-by: Yaozu Dong <eddie.dong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>
>> --- vmx.old    2007-04-25 20:28:19.000000000 +0800
>> +++ vmx.new    2007-04-25 20:28:10.000000000 +0800
>> @@ -1945,7 +1945,8 @@
>>                  return -EINTR;
>>              }
>>  
>> -            kvm_resched(vcpu);
>> +            if ( need_resched() )
>> +                kvm_resched(vcpu);
>>              goto again;
>>          }
>>      }
>
> This should get moved to kvm_resched() since both VT/SVM would benefit 
> from this.
>

Good point.  I'll do it.


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


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] Add need_resched check before go to kvm_resched

[Dong, Eddie]

Anthony Liguori wrote:
> 
> This should get moved to kvm_resched() since both VT/SVM would benefit
> from this.
> 
I would suggest we just add similar code in SVM side. After we 
optimize the MSR/VMCS register save/restore to skip for 
those lightweight VM EXIT (handled by KVM). Giving up preemption
 disable in kvm_resched means we need to do a full context 
restore for host which is architecture specific.

Actually I am thinking to totally give up kvm_resched and just let 
control return to Qemu which is much clean and provide Qemu
 more chance to handle some kind of hardware event such as network 
packet arrive etc. Today Qemu is totally depending on heavyweight VM
Exit 
to handle those events and virtual IRQ injections which may be not
sufficient.

Thanks, eddie

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] Add need_resched check before go to kvm_resched

[Anthony Liguori]

Dong, Eddie wrote:
> Anthony Liguori wrote:
>   
>> This should get moved to kvm_resched() since both VT/SVM would benefit
>> from this.
>>
>>     
> I would suggest we just add similar code in SVM side. After we 
> optimize the MSR/VMCS register save/restore to skip for 
> those lightweight VM EXIT (handled by KVM). Giving up preemption
>  disable in kvm_resched means we need to do a full context 
> restore for host which is architecture specific.
>   

The only time we'll give up preemption disable in kvm_resched is if 
need_resched() is true.  This is fine since it's time to lose our time 
slice anyway.

I was thinking that kvm_resched should look like:

void kvm_resched(struct kvm_vcpu *vcpu)
{
    if (need_resched()) {
        vcpu_put(vcpu);
        cond_resched();
        vcpu_load(vcpu);
   }
}

Which is what I assume Avi did...

> Actually I am thinking to totally give up kvm_resched and just let 
> control return to Qemu which is much clean and provide Qemu
>  more chance to handle some kind of hardware event such as network 
> packet arrive etc. Today Qemu is totally depending on heavyweight VM
> Exit 
> to handle those events and virtual IRQ injections which may be not
> sufficient.
>   

Hrm, how would this work?  The heavy weight MSR stuff is mostly fast 
syscall related so it needs to be restored to do a sysret back to QEMU.

Plus, we'd have to drop the preempt lock which means that QEMU isn't 
guaranteed to be on the same CPU anymore.  We would also need scheduler 
hooks to know when QEMU got descheduled to restore the remaining cpu state.

Regards,

Anthony Liguori

> Thanks, eddie
>
>   


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] Add need_resched check before go to kvm_resched

[Dong, Eddie]

Anthony Liguori wrote:
> Dong, Eddie wrote:
>> Anthony Liguori wrote:
>> 
>>> This should get moved to kvm_resched() since both VT/SVM would
>>> benefit from this. 
>>> 
>>> 
>> I would suggest we just add similar code in SVM side. After we
>> optimize the MSR/VMCS register save/restore to skip for
>> those lightweight VM EXIT (handled by KVM). Giving up preemption
>>  disable in kvm_resched means we need to do a full context
>> restore for host which is architecture specific.
>> 
> 
> The only time we'll give up preemption disable in kvm_resched is if
> need_resched() is true.  This is fine since it's time to lose our time
> slice anyway.
> 
> I was thinking that kvm_resched should look like:
> 
> void kvm_resched(struct kvm_vcpu *vcpu)
> {
>     if (need_resched()) {
>         vcpu_put(vcpu);
>         cond_resched();
>         vcpu_load(vcpu);
>    }
> }

Yes, this is true for today. But if the heavy MSR restore is only be
done for non lightweight VM Exit, that means
 void kvm_resched(struct kvm_vcpu *vcpu)
 {
     if (need_resched()) {
         /* restore host MSR register etc */
         vcpu_put(vcpu);
         cond_resched();
         vcpu_load(vcpu);
    }
 }

Then the whole vmx_vcpu_run will look like:
vmx_vcpu_run()
{
	/* save host MSR register etc. */
again:	/* lightweight VM EXIT loop start here */
	....
	VM RESUME
kvm_vmx_return:	
	r = kvm_handle_exit()
	if ( r > 0 ) {
		...
		...
		if ( !need_resched())  goto again	/* light weight
VM EXIT & no reschedule */
	}
out:	/* heavy weight VM EXIT or need_resched */
	/* restore host MSR register etc. */
	return r;
}

In this case, IOCTL return to Qemu will trigger scheduling at least.

Maybe I miss something.

thx,eddie

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] Add need_resched check before go to kvm_resched

[Anthony Liguori]

Dong, Eddie wrote:
> In this case, IOCTL return to Qemu will trigger scheduling at least.
>   

I think a scheduling change won't happen until the next timer tick.  
AFAICT, there's nothing explicit in the ioctl return path that will 
result in rescheduling.

I'm not entirely confident in how the timer interrupt gets handled when 
it arrives in the guest.  My understanding is that while the timer 
interrupt is delivered in the host, since preempt is disabled, the 
rescheduling opportunity is lost even though the current task is marked 
as needing rescheduling.  I think the next time that bit will get 
checked is the next timer interrupt but I may be wrong.

Regards,

Anthony Liguori

> Maybe I miss something.
>
> thx,eddie
>
>   


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] Add need_resched check before go to kvm_resched

[Dong, Eddie]

Anthony Liguori wrote:
> Dong, Eddie wrote:
>> In this case, IOCTL return to Qemu will trigger scheduling at least.
>> 
> 
> I think a scheduling change won't happen until the next timer tick.
> AFAICT, there's nothing explicit in the ioctl return path that will
> result in rescheduling.

Do u mean Linux system call (IOCTL here) will not do scheudling check?
(check TIF_NEED_RESCHED flag)

Also another chance is vcpu_put() in kvm_vcpu_ioctl_run that
will trigger rescheduling check IMO.

Thx, eddie

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] Add need_resched check before go to kvm_resched

[Anthony Liguori]

Dong, Eddie wrote:
> Anthony Liguori wrote:
>   
>> Dong, Eddie wrote:
>>     
>>> In this case, IOCTL return to Qemu will trigger scheduling at least.
>>>
>>>       
>> I think a scheduling change won't happen until the next timer tick.
>> AFAICT, there's nothing explicit in the ioctl return path that will
>> result in rescheduling.
>>     
>
> Do u mean Linux system call (IOCTL here) will not do scheudling check?
> (check TIF_NEED_RESCHED flag)
>   

Walking through the syscall path, I don't see any place where it would 
explicity check for rescheduling.  That seems logical to me too since 
the only time you should invoke the scheduler is after a timer interrupt 
or within a long running task that would have pre-emption disabled 
(which is what I think KVM qualifies as).

> Also another chance is vcpu_put() in kvm_vcpu_ioctl_run that
> will trigger rescheduling check IMO.
>   

The fast path of vcpu_put() is mutex_unlock which assuming noone is 
waiting on the mutex to be unlocked is just an atomic increment.  I'm 
not entirely sure if the waking up in the slow path will explicitly 
trigger a rescheduling.

Regards,

Anthony Liguori

> Thx, eddie
>
>   


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] Add need_resched check before go to kvm_resched

[Avi Kivity]

Anthony Liguori wrote:
> Dong, Eddie wrote:
>> In this case, IOCTL return to Qemu will trigger scheduling at least.
>>   
>
> I think a scheduling change won't happen until the next timer tick. 
> AFAICT, there's nothing explicit in the ioctl return path that will
> result in rescheduling.
>
> I'm not entirely confident in how the timer interrupt gets handled
> when it arrives in the guest.  My understanding is that while the
> timer interrupt is delivered in the host, since preempt is disabled,
> the rescheduling opportunity is lost even though the current task is
> marked as needing rescheduling.  I think the next time that bit will
> get checked is the next timer interrupt but I may be wrong.
>

No, any syscall exit triggers a rescheduling check, otherwise a task
that spends most of its time in a non-preemt kernel would rarely get
rescheduled.

See arch/x86_64/kernel/entry.S, the various checks for TIF_NEED_RESCHED.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] Add need_resched check before go to kvm_resched

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>   
>> Dong, Eddie wrote:
>>     
>>> In this case, IOCTL return to Qemu will trigger scheduling at least.
>>>   
>>>       
>> I think a scheduling change won't happen until the next timer tick. 
>> AFAICT, there's nothing explicit in the ioctl return path that will
>> result in rescheduling.
>>
>> I'm not entirely confident in how the timer interrupt gets handled
>> when it arrives in the guest.  My understanding is that while the
>> timer interrupt is delivered in the host, since preempt is disabled,
>> the rescheduling opportunity is lost even though the current task is
>> marked as needing rescheduling.  I think the next time that bit will
>> get checked is the next timer interrupt but I may be wrong.
>>
>>     
>
> No, any syscall exit triggers a rescheduling check, otherwise a task
> that spends most of its time in a non-preemt kernel would rarely get
> rescheduled.
>   

Ah, okay.

Regards,

Anthony Liguori

> See arch/x86_64/kernel/entry.S, the various checks for TIF_NEED_RESCHED.
>   



-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] Add need_resched check before go to kvm_resched

[Avi Kivity]

Dong, Eddie wrote:
> Actually I am thinking to totally give up kvm_resched and just let 
> control return to Qemu which is much clean and provide Qemu
>  more chance to handle some kind of hardware event such as network 
> packet arrive etc. Today Qemu is totally depending on heavyweight VM
> Exit 
> to handle those events and virtual IRQ injections which may be not
> sufficient.
>   

Qemu needing service is indicated by the signal_pending() check.  All
I/O sources are set to send a signal when service is needed.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/