Re: [Qemu-devel] [PATCH v5 1/3] qemu-thread: Add qemu_cond_timedwait

[Eric Blake <eblake@redhat.com>]

On 8/26/19 5:37 AM, Yury Kotov wrote:
> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
> ---

Rather sparse on the commit message details.

>  include/qemu/thread.h    | 18 ++++++++++++++++++
>  util/qemu-thread-posix.c | 40 ++++++++++++++++++++++++++++------------
>  util/qemu-thread-win32.c | 16 ++++++++++++++++
>  util/qsp.c               | 18 ++++++++++++++++++
>  4 files changed, 80 insertions(+), 12 deletions(-)
> 

> +++ b/util/qemu-thread-posix.c
> @@ -36,6 +36,18 @@ static void error_exit(int err, const char *msg)
>      abort();
>  }
>  
> +static void compute_abs_deadline(struct timespec *ts, int ms)
> +{
> +    struct timeval tv;
> +    gettimeofday(&tv, NULL);
> +    ts->tv_nsec = tv.tv_usec * 1000 + (ms % 1000) * 1000000;
> +    ts->tv_sec = tv.tv_sec + ms / 1000;
> +    if (ts->tv_nsec >= 1000000000) {
> +        ts->tv_sec++;
> +        ts->tv_nsec -= 1000000000;
> +    }

I don't know if any named constants would make this easier or harder to
read (such as USEC_PER_SEC 1000000 or NSEC_PER_SEC 1000000000), but the
conversion from relative ms to absolute timespec looks correct. [1]

> +void qemu_cond_timedwait_impl(QemuCond *cond, QemuMutex *mutex, int ms,
> +                              const char *file, const int line)
> +{
> +    int err;
> +    struct timespec ts;
> +
> +    assert(cond->initialized);
> +    trace_qemu_mutex_unlock(mutex, file, line);
> +    compute_abs_deadline(&ts, ms);
> +    err = pthread_cond_timedwait(&cond->cond, &mutex->lock, &ts);
> +    trace_qemu_mutex_locked(mutex, file, line);
> +    if (err && err != ETIMEDOUT) {
> +        error_exit(err, __func__);
> +    }
> +}

However, this function returning void looks odd.  Although ETIMEDOUT is
the one error that guarantees that mutex is reobtained (all other errors
occur before the mutex is given up in the first place), and even though
the man page warns that you MUST recheck the condition variable in a
while loop regardless of success or failure (it might be a spurious
successful wake-up due to a broadcast where neither the condition nor
the timeout has actually been reached yet; or it might be a race where
the function reports a timeout immediately before the condition variable
became available after all), it still seems like callers might like to
know if a timeout happened, without having to calculate an ending
absolute time themselves.


>  
> -static void compute_abs_deadline(struct timespec *ts, int ms)
> -{
> -    struct timeval tv;

[1] Oh, you mixed code motion with new code, but the commit message
didn't mention that.  It's not necessarily worth splitting the patch,
but at least mentioning it would be worthwhile.

> +++ b/util/qemu-thread-win32.c
> @@ -145,6 +145,22 @@ void qemu_cond_wait_impl(QemuCond *cond, QemuMutex *mutex, const char *file, con
>      qemu_mutex_post_lock(mutex, file, line);
>  }
>  
> +void qemu_cond_timedwait_impl(QemuCond *cond, QemuMutex *mutex, int ms,
> +                              const char *file, const int line)
> +{
> +    int rc = 0;
> +
> +    assert(cond->initialized);
> +    trace_qemu_mutex_unlock(mutex, file, line);
> +    if (!SleepConditionVariableSRW(&cond->var, &mutex->lock, ms, 0)) {
> +        rc = GetLastError();
> +    }
> +    trace_qemu_mutex_locked(mutex, file, line);
> +    if (rc && rc != ERROR_TIMEOUT) {
> +        error_exit(rc, __func__);
> +    }
> +}

I am less certain that this implementation is correct, but on the
surface it seems okay.


>  
> +static void
> +qsp_cond_timedwait(QemuCond *cond, QemuMutex *mutex, int ms,
> +                   const char *file, int line)
> +{
> +    QSPEntry *e;
> +    int64_t t0, t1;
> +
> +    t0 = get_clock();
> +    qemu_cond_timedwait_impl(cond, mutex, ms, file, line);
> +    t1 = get_clock();
> +
> +    e = qsp_entry_get(cond, file, line, QSP_CONDVAR);
> +    qsp_entry_record(e, t1 - t0);
> +}

Another function where a bool or int return (to distinguish success from
timeout) might be worthwhile to some callers.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org