Re: [Qemu-devel] [PATCH 2/2] [RFC] Convert Clock Timerlists to RCU V2

[Paolo Bonzini]

Il 28/02/2014 21:05, Alex Bligh ha scritto:
> Mike
>
> On 27 Feb 2014, at 19:35, Mike Day wrote:
>
> > timerlists is a list of lists that holds active timers, among other
> > items. It is read-mostly. This patch converts read access to the
> > timerlists to use RCU.
> >
> > Rather than introduce a second mutex for timerlists, which would
> > require nested mutexes to in orderwrite to the timerlists, use one
> > QemuMutex in the QemuClock structure for all write access to any list
> > hanging off the QemuClock structure.
>
> I sort of wonder why you don't just use one Mutex across the whole
> of QEMU rather than one per clock.

I think this is not enough; separate iothreads could have separate 
timerlist, and higher-priority iothreads should not be slowed down by 
lower-priority iothreads.

> > QEMUClockType timerlist_get_clock(QEMUTimerList *timer_list)
> > {
> > -    return timer_list->clock->type;
> > +    return atomic_rcu_read(&timer_list->clock->type);
> > }
>
> I don't think this works because of the double indirection.

It doesn't but you can of course do this:

        QEMUClock *clock = atomic_rcu_read(&timer_list->clock);
        return atomic_rcu_read(&clock->type);

> It's The '&' means it's not actually dereferencing clock->type, but it
> is dereferencing timer_list->clock outside of either an rcu read
> lock or an atomic read. I think you'd be better with than
> rcu_read_lock() etc. (sadly).

You could also assume that the caller does it.  Right now, this function 
has no user, so you just have to document it.

> > QEMUTimerList *qemu_clock_get_main_loop_timerlist(QEMUClockType type)
> > @@ -261,11 +272,13 @@ QEMUTimerList 
> > *qemu_clock_get_main_loop_timerlist(QEMUClockType type)
> >
> > void timerlist_notify(QEMUTimerList *timer_list)
> > {
> > -    if (timer_list->notify_cb) {
> > +    rcu_read_lock();
> > +    if (atomic_rcu_read(&timer_list->notify_cb)) {
> >         timer_list->notify_cb(timer_list->notify_opaque);
> >     } else {
> >         qemu_notify_event();
> >     }
> > +    rcu_read_unlock();
> > }
>
> If you have the rcu_read_lock why do you need the atomic_rcu_read?
> And if you need it, why do you not need it on the following line?

Read the documentation. :)  It was also posted on the list.

atomic_rcu_read() is only about blocking invalid compiler optimizations; 
it is not required if you are in the *write* side, but it is always 
required in the read side.

However, I agree that it is not needed here.  atomic_rcu_read() is not 
needed when reading a "leaf" element of the data structure.

> However, as any QEMUTimerList can (now) be reclaimed, surely all
> callers of this function are going to need to hold the rcu_read_lock
> anyway?
>
> I think this is used only by timerlist_rearm and qemu_clock_notify.
> Provided these call this function holding the rcu_read_lock
> I don't think this function needs changing.

Yes.

Paolo