Re: [Qemu-devel] [PATCH 3/6] Switch POSIX compat AIO to QEMU abstractions

[Kevin Wolf]

Am 21.09.2011 16:02, schrieb Jan Kiszka:
> On 2011-09-21 15:57, Kevin Wolf wrote:
>> Am 20.09.2011 18:53, schrieb Jan Kiszka:
>>> Although there is nothing to wrap for non-POSIX here, redirecting thread
>>> and synchronization services to our core simplifies managements jobs
>>> like scheduling parameter adjustment. It also frees compat AIO from some
>>> duplicate code (/wrt qemu-thread).
>>>
>>> CC: Kevin Wolf <address@hidden>
>>> Signed-off-by: Jan Kiszka <address@hidden>
>>> ---
>>>  posix-aio-compat.c |  115 
>>> ++++++++++++++-------------------------------------
>>>  1 files changed, 32 insertions(+), 83 deletions(-)

>>> @@ -311,27 +279,22 @@ static void posix_aio_notify_event(void);
>>>  
>>>  static void *aio_thread(void *unused)
>>>  {
>>> -    mutex_lock(&lock);
>>> +    qemu_mutex_lock(&lock);
>>>      pending_threads--;
>>> -    mutex_unlock(&lock);
>>> +    qemu_mutex_unlock(&lock);
>>>      do_spawn_thread();
>>>  
>>>      while (1) {
>>>          struct qemu_paiocb *aiocb;
>>> -        ssize_t ret = 0;
>>> -        qemu_timeval tv;
>>> -        struct timespec ts;
>>> -
>>> -        qemu_gettimeofday(&tv);
>>> -        ts.tv_sec = tv.tv_sec + 10;
>>> -        ts.tv_nsec = 0;
>>> +        bool timed_out = false;
>>> +        ssize_t ret;
>>>  
>>> -        mutex_lock(&lock);
>>> +        qemu_mutex_lock(&lock);
>>>  
>>> -        while (QTAILQ_EMPTY(&request_list) &&
>>> -               !(ret == ETIMEDOUT)) {
>>> +        while (QTAILQ_EMPTY(&request_list) && !timed_out) {
>>>              idle_threads++;
>>> -            ret = cond_timedwait(&cond, &lock, &ts);
>>> +            timed_out = qemu_cond_timedwait(&cond, &lock,
>>> +                                            AIO_THREAD_IDLE_TIMEOUT) != 0;
>>
>> Maybe I'm confused by too many negations, but isn't this the wrong way
>> round?
> 
> You mean design-wise? Maybe. In any case, I think this code would also
> win if we just do
> 
>       if (timed_out)
>               break;
> 
> in the loop instead of testing the inverse on entry.

Design-wise I'm not sure. Maybe it would be more consistent if
qemu_cond_timedwait returned 0/ETIMEDOUT, maybe it doesn't really make a
difference. I just felt a bit confused when reading it.

What I really meant is that I think it should be == instead of !=:

timed_out = qemu_cond_timedwait(...) == 0;

>> +    err = pthread_cond_timedwait(&cond->cond, &mutex->lock, &ts);
>> +    if (err && err != ETIMEDOUT) {
>> +        error_exit(err, __func__);
>> +    }
>> +    return err == 0;
>>
>> So if there was an timeout, qemu_cond_timedwait returns 0 (should it
>> return a bool? Also documenting the return value wouldn't hurt) and
>> timed_out becomes false (0 != 0).
> 
> Will switch to a bool return code (and document it).

Ok.

Kevin