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 <kw...@redhat.com>
>>> Signed-off-by: Jan Kiszka <jan.kis...@siemens.com>
>>> ---
>>> 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
