Re: [Qemu-devel] [PATCH 1/2] [RFC] Convert active timers list to use RCU V2

[Alex Bligh]

Mike,

This one's nice and clear. A few comments in line.

On 27 Feb 2014, at 19:35, Mike Day wrote:

> @@ -184,16 +178,17 @@ bool qemu_clock_has_timers(QEMUClockType type)
> bool timerlist_expired(QEMUTimerList *timer_list)
> {
>     int64_t expire_time;
> +    bool ret;
> 
> -    qemu_mutex_lock(&timer_list->active_timers_lock);
> -    if (!timer_list->active_timers) {
> -        qemu_mutex_unlock(&timer_list->active_timers_lock);
> +    rcu_read_lock();
> +    if (!atomic_rcu_read(&timer_list->active_timers)) {
> +        rcu_read_unlock();
>         return false;
>     }
>     expire_time = timer_list->active_timers->expire_time;
> -    qemu_mutex_unlock(&timer_list->active_timers_lock);
> -
> -    return expire_time < qemu_clock_get_ns(timer_list->clock->type);
> +    ret = (expire_time < qemu_clock_get_ns(timer_list->clock->type));
> +    rcu_read_unlock();
> +    return ret;
> }

I think it probably makes little difference, but why not:

       int type = timerlist->clock->type;
       rcu_read_unlock();
       return expire_time < qemu_clock_get_ns(type);



> bool qemu_clock_expired(QEMUClockType type)
> @@ -220,16 +215,16 @@ int64_t timerlist_deadline_ns(QEMUTimerList *timer_list)
>      * value but ->notify_cb() is called when the deadline changes.  Therefore
>      * the caller should notice the change and there is no race condition.
>      */
> -    qemu_mutex_lock(&timer_list->active_timers_lock);
> -    if (!timer_list->active_timers) {
> -        qemu_mutex_unlock(&timer_list->active_timers_lock);
> +
> +    rcu_read_lock();
> +    if (!atomic_rcu_read(&timer_list->active_timers)) {
> +        rcu_read_unlock();
>         return -1;
>     }
>     expire_time = timer_list->active_timers->expire_time;
> -    qemu_mutex_unlock(&timer_list->active_timers_lock);
> -
>     delta = expire_time - qemu_clock_get_ns(timer_list->clock->type);
> 
> +    rcu_read_unlock();
>     if (delta <= 0) {
>         return 0;
>     }

Similarly perhaps save 'type' and read the clock outside the
rcu read-sde lock.
> 
> static void timer_del_locked(QEMUTimerList *timer_list, QEMUTimer *ts)
> {
>     QEMUTimer **pt, *t;
> @@ -344,6 +346,8 @@ static void timer_del_locked(QEMUTimerList *timer_list, 
> QEMUTimer *ts)
>     ts->expire_time = -1;
>     pt = &timer_list->active_timers;
>     for(;;) {
> +        /* caller's lock causes cache updates, so we don't need to use */
> +        /* atomic_rcu_read or atomic_rcu_set */
>         t = *pt;
>         if (!t)
>             break;

I'm showing my RCU ignorance here, but does that mean we should be doing
an rmb() after taking the lock in (e.g.) timer_mod_ns? Or are mutexes
guaranteed to rmb()?

> @@ -461,12 +462,13 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
>     bool progress = false;
>     QEMUTimerCB *cb;
>     void *opaque;
> +    bool enabled;
> 
> -    qemu_event_reset(&timer_list->timers_done_ev);
> -    if (!timer_list->clock->enabled) {
> -        goto out;
> +    enabled = atomic_rcu_read(&timer_list->clock->enabled);
> +    if (!enabled) {
> +        return progress;
>     }
> -
> +    qemu_event_reset(&timer_list->timers_done_ev);
>     current_time = qemu_clock_get_ns(timer_list->clock->type);
>     for(;;) {
>         qemu_mutex_lock(&timer_list->active_timers_lock);

What's the introduction of 'enabled' for? Why not simply

    if (!atomic_rcu_read(&timer_list->clock->enabled)) {
        return progress;
    }

Also, previously if called on a clock that was disabled, this would set and
reset the qemu_event, which would wake up any waiters. It no longer
does that. Is this change intended?

> @@ -482,14 +484,13 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
>         ts->expire_time = -1;
>         cb = ts->cb;
>         opaque = ts->opaque;
> +        rcu_read_lock();
>         qemu_mutex_unlock(&timer_list->active_timers_lock);
> -
>         /* run the callback (the timer list can be modified) */
>         cb(opaque);
> +        rcu_read_unlock();
>         progress = true;
>     }
> -
> -out:
>     qemu_event_set(&timer_list->timers_done_ev);
>     return progress;
> }

Again showing my RCU ignorance here, but do qemu_mutex_lock() and
qemu_mutex_unlock() automatically take and drop the rcu read lock?
If not, we'd need to hold that rcu_read_lock whenever the mutex
is locked.

-- 
Alex Bligh