[Qemu-devel] [PATCH for-2.5] rcu: Allow calling rcu_(un)register_thread() during synchronize_rcu()
If rcu_(un)register_thread() is called together with synchronize_rcu(),
it will wait for the synchronize_rcu() to finish. But when synchronize_rcu()
waits for some events, we can modify the list registry.
We also use the lock rcu_gp_lock to assume that synchronize_rcu() isn't
executed in more than one thread at the same time. Add a new mutex lock
rcu_sync_lock to assume it and rename rcu_gp_lock to rcu_registry_lock.
Release rcu_registry_lock when synchronize_rcu() waits for some events.
Signed-off-by: Wen Congyang
---
util/rcu.c | 36 +++-
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/util/rcu.c b/util/rcu.c
index cdcad67..18c5792 100644
--- a/util/rcu.c
+++ b/util/rcu.c
@@ -47,7 +47,8 @@
unsigned long rcu_gp_ctr = RCU_GP_LOCKED;
QemuEvent rcu_gp_event;
-static QemuMutex rcu_gp_lock;
+static QemuMutex rcu_registry_lock;
+static QemuMutex rcu_sync_lock;
/*
* Check whether a quiescent state was crossed between the beginning of
@@ -66,7 +67,7 @@ static inline int rcu_gp_ongoing(unsigned long *ctr)
*/
__thread struct rcu_reader_data rcu_reader;
-/* Protected by rcu_gp_lock. */
+/* Protected by rcu_registry_lock. */
typedef QLIST_HEAD(, rcu_reader_data) ThreadList;
static ThreadList registry = QLIST_HEAD_INITIALIZER(registry);
@@ -114,10 +115,14 @@ static void wait_for_readers(void)
break;
}
-/* Wait for one thread to report a quiescent state and
- * try again.
+/* Wait for one thread to report a quiescent state and try again.
+ * Release rcu_registry_lock, so rcu_(un)register_thread() doesn't
+ * wait too much time. Note: rcu_unregister_thread() may remove
+ * the node from qsreaders. That's a bit tricky, but it should work.
*/
+qemu_mutex_unlock(&rcu_registry_lock);
qemu_event_wait(&rcu_gp_event);
+qemu_mutex_lock(&rcu_registry_lock);
}
/* put back the reader list in the registry */
@@ -126,7 +131,8 @@ static void wait_for_readers(void)
void synchronize_rcu(void)
{
-qemu_mutex_lock(&rcu_gp_lock);
+qemu_mutex_lock(&rcu_sync_lock);
+qemu_mutex_lock(&rcu_registry_lock);
if (!QLIST_EMPTY(®istry)) {
/* In either case, the atomic_mb_set below blocks stores that free
@@ -149,7 +155,8 @@ void synchronize_rcu(void)
wait_for_readers();
}
-qemu_mutex_unlock(&rcu_gp_lock);
+qemu_mutex_unlock(&rcu_registry_lock);
+qemu_mutex_unlock(&rcu_sync_lock);
}
@@ -273,23 +280,24 @@ void call_rcu1(struct rcu_head *node, void (*func)(struct
rcu_head *node))
void rcu_register_thread(void)
{
assert(rcu_reader.ctr == 0);
-qemu_mutex_lock(&rcu_gp_lock);
+qemu_mutex_lock(&rcu_registry_lock);
QLIST_INSERT_HEAD(®istry, &rcu_reader, node);
-qemu_mutex_unlock(&rcu_gp_lock);
+qemu_mutex_unlock(&rcu_registry_lock);
}
void rcu_unregister_thread(void)
{
-qemu_mutex_lock(&rcu_gp_lock);
+qemu_mutex_lock(&rcu_registry_lock);
QLIST_REMOVE(&rcu_reader, node);
-qemu_mutex_unlock(&rcu_gp_lock);
+qemu_mutex_unlock(&rcu_registry_lock);
}
static void rcu_init_complete(void)
{
QemuThread thread;
-qemu_mutex_init(&rcu_gp_lock);
+qemu_mutex_init(&rcu_registry_lock);
+qemu_mutex_init(&rcu_sync_lock);
qemu_event_init(&rcu_gp_event, true);
qemu_event_init(&rcu_call_ready_event, false);
@@ -306,12 +314,14 @@ static void rcu_init_complete(void)
#ifdef CONFIG_POSIX
static void rcu_init_lock(void)
{
-qemu_mutex_lock(&rcu_gp_lock);
+qemu_mutex_lock(&rcu_sync_lock);
+qemu_mutex_lock(&rcu_registry_lock);
}
static void rcu_init_unlock(void)
{
-qemu_mutex_unlock(&rcu_gp_lock);
+qemu_mutex_unlock(&rcu_registry_lock);
+qemu_mutex_unlock(&rcu_sync_lock);
}
#endif
--
2.4.3
Re: [Qemu-devel] [PATCH for-2.5] rcu: Allow calling rcu_(un)register_thread() during synchronize_rcu()
On 27/07/2015 04:24, Wen Congyang wrote: > +/* Wait for one thread to report a quiescent state and try again. > + * Release rcu_registry_lock, so rcu_(un)register_thread() doesn't > + * wait too much time. Note: rcu_unregister_thread() may remove > + * the node from qsreaders. That's a bit tricky, but it should work. "It should work" is a bit optimistic. :D Does this description look okay? /* Wait for one thread to report a quiescent state and try again. * Release rcu_registry_lock, so rcu_(un)register_thread() doesn't * wait too much time. * * rcu_register_thread() may add nodes to ®istry; it will not * wake up synchronize_rcu, but that is okay because at least another * thread must exit its RCU read-side critical section before * synchronize_rcu is done. The next iteration of the loop will * process the new thread or set ->waiting for it. Hence, this can * at worst cause synchronize_rcu() to wait for longer. * * rcu_unregister_thread() may remove nodes from &qsreaders instead * of ®istry if it runs during qemu_event_wait. That's okay; * the node then will not be added back to ®istry by QLIST_SWAP * below. The invariant is that the node is part of one list when * rcu_registry_lock is released. */ Paolo
Re: [Qemu-devel] [PATCH for-2.5] rcu: Allow calling rcu_(un)register_thread() during synchronize_rcu()
On 07/27/2015 06:17 PM, Paolo Bonzini wrote: > > > On 27/07/2015 04:24, Wen Congyang wrote: >> +/* Wait for one thread to report a quiescent state and try again. >> + * Release rcu_registry_lock, so rcu_(un)register_thread() doesn't >> + * wait too much time. Note: rcu_unregister_thread() may remove >> + * the node from qsreaders. That's a bit tricky, but it should work. > > "It should work" is a bit optimistic. :D > > Does this description look okay? > > /* Wait for one thread to report a quiescent state and try again. > * Release rcu_registry_lock, so rcu_(un)register_thread() doesn't > * wait too much time. > * > * rcu_register_thread() may add nodes to ®istry; it will not > * wake up synchronize_rcu, but that is okay because at least another > * thread must exit its RCU read-side critical section before > * synchronize_rcu is done. The next iteration of the loop will > * process the new thread or set ->waiting for it. Hence, this can > * at worst cause synchronize_rcu() to wait for longer. I don't understand this. The next iteration of the loop will move the new thread's rcu_reader from registry to qsreaders even if we call rcu_read_lock() in the new thread. Because rcu_gp_ongoing() will return false. Thanks Wen Congyang > * > * rcu_unregister_thread() may remove nodes from &qsreaders instead > * of ®istry if it runs during qemu_event_wait. That's okay; > * the node then will not be added back to ®istry by QLIST_SWAP > * below. The invariant is that the node is part of one list when > * rcu_registry_lock is released. > */ > > Paolo > . >
Re: [Qemu-devel] [PATCH for-2.5] rcu: Allow calling rcu_(un)register_thread() during synchronize_rcu()
On 27/07/2015 12:44, Wen Congyang wrote: > > * rcu_register_thread() may add nodes to ®istry; it will not > > * wake up synchronize_rcu, but that is okay because at least > > another > > * thread must exit its RCU read-side critical section before > > * synchronize_rcu is done. The next iteration of the loop will > > * process the new thread or set ->waiting for it. Hence, this can > > * at worst cause synchronize_rcu() to wait for longer. > I don't understand this. The next iteration of the loop will move the new > thread's > rcu_reader from registry to qsreaders even if we call rcu_read_lock() in the > new thread. > Because rcu_gp_ongoing() will return false. You're right. This proves that a comment was necessary! :) Second try: * rcu_register_thread() may add nodes to ®istry; it will not * wake up synchronize_rcu, but that is okay because at least another * thread must exit its RCU read-side critical section before * synchronize_rcu is done. The next iteration of the loop will * move the new thread's rcu_reader from ®istry to &qsreaders, * because rcu_gp_ongoing() will return false. Paolo
Re: [Qemu-devel] [PATCH for-2.5] rcu: Allow calling rcu_(un)register_thread() during synchronize_rcu()
On 07/27/2015 06:52 PM, Paolo Bonzini wrote: > > > On 27/07/2015 12:44, Wen Congyang wrote: >>> * rcu_register_thread() may add nodes to ®istry; it will not >>> * wake up synchronize_rcu, but that is okay because at least >>> another >>> * thread must exit its RCU read-side critical section before >>> * synchronize_rcu is done. The next iteration of the loop will >>> * process the new thread or set ->waiting for it. Hence, this can >>> * at worst cause synchronize_rcu() to wait for longer. >> I don't understand this. The next iteration of the loop will move the new >> thread's >> rcu_reader from registry to qsreaders even if we call rcu_read_lock() in the >> new thread. >> Because rcu_gp_ongoing() will return false. > > You're right. This proves that a comment was necessary! :) Yes, I agree with it. > > Second try: > > * rcu_register_thread() may add nodes to ®istry; it will not > * wake up synchronize_rcu, but that is okay because at least another > * thread must exit its RCU read-side critical section before > * synchronize_rcu is done. The next iteration of the loop will > * move the new thread's rcu_reader from ®istry to &qsreaders, > * because rcu_gp_ongoing() will return false. I will update the comment and send it again. Thanks Wen Congyang > > Paolo > . >
