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
