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

[Alex Bligh]

Mike,

On 15 Feb 2014, at 20:33, Mike Day wrote:
>> 
>> 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.

I think you mean "protect the list of timerlists somehow". I agree.

I suppose RCU would ultimately be better than a mutex as it's the
archetypal read lots write almost never list. I suppose I somewhat
grudgingly see the point! If you want to do that this time round, I'd
suggest you break it into two patches.

>> 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.

Fair enough.

> 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.

Agree.

>> 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.

It will. But I suppose it isn't a great concern.

-- 
Alex Bligh