[PATCH] qemu-kvm: Consolidate kvm_eat_signals

[PATCH] qemu-kvm: Consolidate kvm_eat_signals

[Jan Kiszka]

Having both kvm_eat_signal and kvm_eat_signals makes the code harder to
read. Moreover, given the single caller of kvm_eat_signals, there is no
real reason to keep it in a separate function.

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
---
 qemu/qemu-kvm.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Index: b/qemu/qemu-kvm.c
===================================================================
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -210,11 +210,12 @@ static int kvm_eat_signal(CPUState *env,
     return ret;
 }
 
-
-static void kvm_eat_signals(CPUState *env, int timeout)
+static void kvm_main_loop_wait(CPUState *env, int timeout)
 {
     int r = 0;
 
+    pthread_mutex_unlock(&qemu_mutex);
+
     while (kvm_eat_signal(env, 0))
 	r = 1;
     if (!r && timeout) {
@@ -223,14 +224,10 @@ static void kvm_eat_signals(CPUState *en
 	    while (kvm_eat_signal(env, 0))
 		;
     }
-}
 
-static void kvm_main_loop_wait(CPUState *env, int timeout)
-{
-    pthread_mutex_unlock(&qemu_mutex);
-    kvm_eat_signals(env, timeout);
     pthread_mutex_lock(&qemu_mutex);
     cpu_single_env = env;
+
     vcpu_info[env->cpu_index].signalled = 0;
 }
 

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [PATCH] qemu-kvm: Consolidate kvm_eat_signals

[Avi Kivity]

Jan Kiszka wrote:
> Having both kvm_eat_signal and kvm_eat_signals makes the code harder to
> read. Moreover, given the single caller of kvm_eat_signals, there is no
> real reason to keep it in a separate function.
>
>   

Given that with the iothread we spend very little time processing 
signals in vcpu threads, maybe it's better to drop the loop completely.  
The common case is zero or one pending signals.  The uncommon case of 
two or more pending signals will be handled by the KVM_RUN ioctl 
returning immediately with -EINTR (i.e. in the outer loop).

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


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [PATCH] qemu-kvm: Consolidate kvm_eat_signals

[Jan Kiszka]

[-- Attachment #1.1: Type: text/plain, Size: 893 bytes --]

Avi Kivity wrote:
> Jan Kiszka wrote:
>> Having both kvm_eat_signal and kvm_eat_signals makes the code harder to
>> read. Moreover, given the single caller of kvm_eat_signals, there is no
>> real reason to keep it in a separate function.
>>
>>   
> 
> Given that with the iothread we spend very little time processing
> signals in vcpu threads, maybe it's better to drop the loop completely. 
> The common case is zero or one pending signals.  The uncommon case of
> two or more pending signals will be handled by the KVM_RUN ioctl
> returning immediately with -EINTR (i.e. in the outer loop).
> 

You mean

static void kvm_main_loop_wait(CPUState *env, int timeout)
{
    pthread_mutex_unlock(&qemu_mutex);
    kvm_eat_signal(env, timeout);
    pthread_mutex_lock(&qemu_mutex);
    cpu_single_env = env;

    vcpu_info[env->cpu_index].signalled = 0;
}

?

Jan


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

[-- Attachment #2: Type: text/plain, Size: 320 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

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

_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] qemu-kvm: Consolidate kvm_eat_signals

[Avi Kivity]

Jan Kiszka wrote:
>> Given that with the iothread we spend very little time processing
>> signals in vcpu threads, maybe it's better to drop the loop completely. 
>> The common case is zero or one pending signals.  The uncommon case of
>> two or more pending signals will be handled by the KVM_RUN ioctl
>> returning immediately with -EINTR (i.e. in the outer loop).
>>
>>     
>
> You mean
>
> static void kvm_main_loop_wait(CPUState *env, int timeout)
> {
>     pthread_mutex_unlock(&qemu_mutex);
>     kvm_eat_signal(env, timeout);
>     pthread_mutex_lock(&qemu_mutex);
>     cpu_single_env = env;
>
>     vcpu_info[env->cpu_index].signalled = 0;
> }
>
> ?
>   

Yes.  The loop was a (perhaps premature) optimization that is now 
totally unnecessary, unless I'm missing something quite large.

Oh.  There used to be a bug where we didn't check for a pending signal 
before the first guest entry, so this would add a lot of latency 
(effectively making the bug window much larger).  That was only closed 
in 2.6.24 (by 7e66f350).

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


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [PATCH] qemu-kvm: Consolidate kvm_eat_signals

[Anthony Liguori]

Avi Kivity wrote:
> Jan Kiszka wrote:
>   
>>> Given that with the iothread we spend very little time processing
>>> signals in vcpu threads, maybe it's better to drop the loop completely. 
>>> The common case is zero or one pending signals.  The uncommon case of
>>> two or more pending signals will be handled by the KVM_RUN ioctl
>>> returning immediately with -EINTR (i.e. in the outer loop).
>>>
>>>     
>>>       
>> You mean
>>
>> static void kvm_main_loop_wait(CPUState *env, int timeout)
>> {
>>     pthread_mutex_unlock(&qemu_mutex);
>>     kvm_eat_signal(env, timeout);
>>     pthread_mutex_lock(&qemu_mutex);
>>     cpu_single_env = env;
>>
>>     vcpu_info[env->cpu_index].signalled = 0;
>> }
>>
>> ?
>>   
>>     
>
> Yes.  The loop was a (perhaps premature) optimization that is now 
> totally unnecessary, unless I'm missing something quite large.
>   

It used to be that kvm_eat_signal() selected after consuming as many 
signals as possible while only sleeping once.  That's why there's a 
combination of sleeping and polling.

Now the VCPU threads never select so the whole loop can be simplified to 
a single sigtimedwait() that always blocks.

In reality, I don't think sigtimedwait() is really needed/useful for 
VCPUs anymore.  We only use it to catch SIG_IPI and we only use SIG_IPI 
to break out of sleeping.  I don't see any reason why we couldn't switch 
over to using a file descriptor for notification (or a pthread 
condition).  In the very least, we could just select() on nothing and 
allow SIG_IPI to break us out of the select.

Regards,

Anthony Liguori

> Oh.  There used to be a bug where we didn't check for a pending signal 
> before the first guest entry, so this would add a lot of latency 
> (effectively making the bug window much larger).  That was only closed 
> in 2.6.24 (by 7e66f350).
>
>   


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [PATCH] qemu-kvm: Consolidate kvm_eat_signals

[Avi Kivity]

Anthony Liguori wrote:
>>
>> Yes.  The loop was a (perhaps premature) optimization that is now 
>> totally unnecessary, unless I'm missing something quite large.
>>   
>
> It used to be that kvm_eat_signal() selected after consuming as many 
> signals as possible while only sleeping once.  That's why there's a 
> combination of sleeping and polling.
>

Yes.

> Now the VCPU threads never select so the whole loop can be simplified 
> to a single sigtimedwait() that always blocks.
>
> In reality, I don't think sigtimedwait() is really needed/useful for 
> VCPUs anymore.  We only use it to catch SIG_IPI and we only use 
> SIG_IPI to break out of sleeping.  I don't see any reason why we 
> couldn't switch over to using a file descriptor for notification (or a 
> pthread condition).

How would you stop a vcpu running in guest mode?

> In the very least, we could just select() on nothing and allow SIG_IPI 
> to break us out of the select.

sigtimedwait() (or just sigwait, now) doesn't require the signal to be 
delivered, so it's faster.

If there's nothing to select, why call select()?

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


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [PATCH] qemu-kvm: Consolidate kvm_eat_signals

[Anthony Liguori]

Avi Kivity wrote:
>> Now the VCPU threads never select so the whole loop can be simplified 
>> to a single sigtimedwait() that always blocks.
>>
>> In reality, I don't think sigtimedwait() is really needed/useful for 
>> VCPUs anymore.  We only use it to catch SIG_IPI and we only use 
>> SIG_IPI to break out of sleeping.  I don't see any reason why we 
>> couldn't switch over to using a file descriptor for notification (or 
>> a pthread condition).
>
> How would you stop a vcpu running in guest mode?

Yeah, I forgot about that.

>> In the very least, we could just select() on nothing and allow 
>> SIG_IPI to break us out of the select.
>
> sigtimedwait() (or just sigwait, now) doesn't require the signal to be 
> delivered, so it's faster.

Yeah, sigtimedwait() is probably the right thing to do since we have to 
use a signal for IPI.

Regards,

Anthony Liguori

> If there's nothing to select, why call select()?
>


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone