From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56789) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKTdS-0007K5-Az for qemu-devel@nongnu.org; Mon, 03 Mar 2014 09:14:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WKTdM-0005pw-CR for qemu-devel@nongnu.org; Mon, 03 Mar 2014 09:14:26 -0500 Received: from mx1.redhat.com ([209.132.183.28]:17954) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKTdM-0005ps-4D for qemu-devel@nongnu.org; Mon, 03 Mar 2014 09:14:20 -0500 Message-ID: <53148E32.1030905@redhat.com> Date: Mon, 03 Mar 2014 15:14:10 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1393529758-12193-1-git-send-email-ncmike@ncultra.org> <1393529758-12193-3-git-send-email-ncmike@ncultra.org> <4191BEBA-A5A5-4F29-8FD5-DB62A7F78EF7@alex.org.uk> In-Reply-To: <4191BEBA-A5A5-4F29-8FD5-DB62A7F78EF7@alex.org.uk> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] [RFC] Convert Clock Timerlists to RCU V2 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Bligh , Mike Day Cc: qemu-devel@nongnu.org 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