[PATCH] qemu-kvm: Fix test for I/O thread

[PATCH] qemu-kvm: Fix test for I/O thread

[Jan Kiszka]

Looks to me like this was rather intended. But given that 4e8b8a6d92c5ece048e65be3a3980d24f065b32b claims to actually fix a bug in
its original broken form, please have a careful look.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index 431e26d..c36f60f 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -89,7 +89,7 @@ static void qemu_cond_wait(pthread_cond_t *cond)
     pthread_cond_timedwait(cond, &qemu_mutex, &ts);
     /* If we're the I/O thread, some other thread may be waiting for aio
      * completion */
-    if (!vcpu_info)
+    if (!env)
         qemu_aio_poll();
     cpu_single_env = env;
 }

Re: [PATCH] qemu-kvm: Fix test for I/O thread

[Avi Kivity]

Jan Kiszka wrote:
> Looks to me like this was rather intended. But given that 4e8b8a6d92c5ece048e65be3a3980d24f065b32b claims to actually fix a bug in
> its original broken form, please have a careful look.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>
> diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
> index 431e26d..c36f60f 100644
> --- a/qemu/qemu-kvm.c
> +++ b/qemu/qemu-kvm.c
> @@ -89,7 +89,7 @@ static void qemu_cond_wait(pthread_cond_t *cond)
>      pthread_cond_timedwait(cond, &qemu_mutex, &ts);
>      /* If we're the I/O thread, some other thread may be waiting for aio
>       * completion */
> -    if (!vcpu_info)
> +    if (!env)
>          qemu_aio_poll();
>      cpu_single_env = env;
>  }
>   

Aren't the two lines equivalent?  vcpu_info is a thread-local-storage 
variable, and is unset for the iothread.

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

Re: [PATCH] qemu-kvm: Fix test for I/O thread

[Jan Kiszka]

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

Avi Kivity wrote:
> Jan Kiszka wrote:
>> Looks to me like this was rather intended. But given that
>> 4e8b8a6d92c5ece048e65be3a3980d24f065b32b claims to actually fix a bug in
>> its original broken form, please have a careful look.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
>> index 431e26d..c36f60f 100644
>> --- a/qemu/qemu-kvm.c
>> +++ b/qemu/qemu-kvm.c
>> @@ -89,7 +89,7 @@ static void qemu_cond_wait(pthread_cond_t *cond)
>>      pthread_cond_timedwait(cond, &qemu_mutex, &ts);
>>      /* If we're the I/O thread, some other thread may be waiting for aio
>>       * completion */
>> -    if (!vcpu_info)
>> +    if (!env)
>>          qemu_aio_poll();
>>      cpu_single_env = env;
>>  }
>>   
> 
> Aren't the two lines equivalent?  vcpu_info is a thread-local-storage
> variable, and is unset for the iothread.
> 

Then you probably wanted to type 'vcpu', don't you? :)

However, I would stick with what the function already uses, ie. 'env'.

Jan


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

Re: [PATCH] qemu-kvm: Fix test for I/O thread

[Avi Kivity]

Jan Kiszka wrote:
> Avi Kivity wrote:
>   
>> Jan Kiszka wrote:
>>     
>>> Looks to me like this was rather intended. But given that
>>> 4e8b8a6d92c5ece048e65be3a3980d24f065b32b claims to actually fix a bug in
>>> its original broken form, please have a careful look.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
>>> index 431e26d..c36f60f 100644
>>> --- a/qemu/qemu-kvm.c
>>> +++ b/qemu/qemu-kvm.c
>>> @@ -89,7 +89,7 @@ static void qemu_cond_wait(pthread_cond_t *cond)
>>>      pthread_cond_timedwait(cond, &qemu_mutex, &ts);
>>>      /* If we're the I/O thread, some other thread may be waiting for aio
>>>       * completion */
>>> -    if (!vcpu_info)
>>> +    if (!env)
>>>          qemu_aio_poll();
>>>      cpu_single_env = env;
>>>  }
>>>   
>>>       
>> Aren't the two lines equivalent?  vcpu_info is a thread-local-storage
>> variable, and is unset for the iothread.
>>
>>     
>
> Then you probably wanted to type 'vcpu', don't you? :)
>
>   

Doh.

> However, I would stick with what the function already uses, ie. 'env'.
>   

env might be null even in a non-aio-thread (during initialization, 
perhaps?).  I changed it to be vcpu since that's a stronger indicator.

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