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
