Re: [Qemu-devel] [PATCH] [RFC] Convert Qemu Timer List and Active Timers to RCU

[Mike Day]

Alex Bligh <address@hidden> writes:

> Some comments:
Thanks for the thorough review!
> 1. You seem to be removing the use of the active_timers_lock and replacing it 
> by
>    rcu (fine). However, you seem to have left the qemu_mutex_destroy in
>    timerlist_free, and left the mutex in QEMUTimerList. Any reason why we need
>    both?
This was an oversight on my part - which I will correct.

> 2. You have introduced rcu not only to protect active_timers, the list of
>    active timers within one timerlist, but also to protect (I think)
>    the list of timerlists, as evidenced by the fact you have
>    reclaim_timer_list as well as reclaim_timer. We currently have no
>    locking in the creation/deletion of timerlists as these only happen on init
>    or on when an AioContext is created/destroyed (as these are the only things
>    that create or delete TimerListGroups); both these operations are
>    required to happen within the main thread and are protected by the
>    BQL. Indeed currently nothing every destroys an AioContext. I suspect
>    if there is really a need to destroy timerlists in threads other than
>    the main thread we are going to have more issues than locking of the list 
> of
>    timerlists. I suggest you remove these changes and solely look at 
> protecting
>    the list of active timers (as opposed to the list of timerlists) by RCU.

I thought about this a lot and and I'm glad you are bringing up the
timerlist here. As you said, I couldn't find anywhere in the code base
that manipulates this list. But I also noticed that the timer and clock
structures are accessed by unrelated code modules. I don't know enough
about the timer usage to predict how timers will be used by other code
modules so I decided to overprotect and see what you thought about it.

Removing the protection of the timerlist will vastly simplify this
patch. Eventually, though, when we remove the BQL, I'm assuming we will
need to protect the timerlist somehow. I'm happy now re-factoring toward
a much simpler patch that just protects the active timer list.

>
> 3. If you are going to protect the list of timerlists as well, do you
>    not need to at least take the rcu read lock in timerlist_new?

Thanks for catching this. It probably needs a proper mutex (which is the
BQL now as you stated).

> 4. In timerlist_notify(), you are holding the rcu_read_lock across
>    the callback or qemu_notify_event. Why is this necessary? EG can't you
>    do something like:
>
>     void timerlist_notify(QEMUTimerList *timer_list)
>     {
>          rcu_read_lock();
>          if (atomic_rcu_read(&timer_list->notify_cb)) {
>             QEMUTimerListNotifyCB *notify_cb = timer_list->notify_cb;
>             void *notify_opaque = timer_list->notify_opaque;
>             rcu_read_unlock();
>             notify_cb(notify_opaque);
>         } else {
>             rcu_read_unlock();
>             qemu_notify_event();
>         }
>     }

I thought a lot about this one and went back and forth a couple of
times, so I'm glad you are looking at it. My concern is that the timer
could be reclaimed during execution ofthe callback. And, the callback
may be referincing the timer itself. Even if the callback takes an
rcu_read_lock, there is a window between when the timer code leaves the
rcu critical section and calls the notification code, and when the
notification callback enters the rcu critical section via
rcu_read_lock. It may be possible though unlikely that a reclaim could
occur during this small window.

The downside, as far as I know, is that the notification callback will
take a long time to execute. This is not good, but it won't cause an
error by itself.

> 5. Similarly to the above, in qemu_clock_notify, you take the
>    rcu_read_lock, and then do a QLIST_FOREACH_RCU. That will mean
>    the rcu_read_lock is taken anyway in 99% of calls to timerlist_notify.
>    This is going through the list of timerlists, and that list is in
>    practice static (protected by the BQL). Similarly in
>    qemu_clock_deadline_ns_all, timerlist_get_clock, etc.

My thinking here is that rcu_read_lock can be called recursively with no
bad effect other than possibly taking up cache space. So when you have a
function like this that is usually called as an internal function but
occasionally called directly, take the rcu read lock in both
functions. I think this will be removed anyway with the timerlist
change.

> 6. In timerlist_expired, rcu_read_unlock() appears to get called
>    at the end after the return statement (last line of the function).
It does indeed, thanks. 

> 7. May be my reading of the diff, but in timer_mod_ns it seems to be
>    introducing two rcu_read_unlock()s at the end of the function
>    (after 'Rearm if necessary') - should the first not be a
>    rcu_read_lock()?
Yes, thank you.

> 8. Again, may be my reading of the diff, but you appear to have
>    introduced a rcu_read_lock() outside the loop, but then
>    unlock it within the loop. If the loop iterates more than once,
>    won't it do more unlocks than locks?

Yes, I think the right fix is to omit the last rcu_read_unlock.

-- 
Mike Day | "Endurance is a Virtue"