* [ltt-dev] Userspace RCU git fixes @ 2011-09-11 6:14 Mathieu Desnoyers 2011-09-12 7:32 ` Paolo Bonzini 0 siblings, 1 reply; 3+ messages in thread From: Mathieu Desnoyers @ 2011-09-11 6:14 UTC (permalink / raw) Hi Paul, Hi Paolo, Just to let you know that I pushed two updates into urcu: one fixes a grace period hang caused by a missing wakeup in the synchronize_rcu QSBR code. This appears to hit us due to the more fine-grained wakeup code brought by Paolo. The wakeup was really missing from the synchronize_rcu code (so Paolo's code just triggered an existing problem). I thought it would be good to let you know the effect: grace periods are delayed forever. This problem never appeared in a release (I caught it before). The second update registers the call_rcu worker threads as RCU reader threads, so we can take the read-side lock inside them. I put them offline when they wait, which makes them report quiescent state in QSBR. This turns out to be required for my hash table code, and I think this is generally a good thing. Thanks, Mathieu -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 3+ messages in thread
* [ltt-dev] Userspace RCU git fixes 2011-09-11 6:14 [ltt-dev] Userspace RCU git fixes Mathieu Desnoyers @ 2011-09-12 7:32 ` Paolo Bonzini 2011-09-13 17:53 ` Mathieu Desnoyers 0 siblings, 1 reply; 3+ messages in thread From: Paolo Bonzini @ 2011-09-12 7:32 UTC (permalink / raw) > Just to let you know that I pushed two updates into urcu: one fixes a > grace period hang caused by a missing wakeup in the synchronize_rcu QSBR > code. This appears to hit us due to the more fine-grained wakeup > code brought by Paolo. The wakeup was really missing from the > synchronize_rcu code (so Paolo's code just triggered an existing > problem). I thought it would be good to let you know the effect: grace > periods are delayed forever. This problem never appeared in a release (I > caught it before). Good catch. Why not use rcu_thread_offline/online in synchronize_rcu, instead of touching rcu_reader.ctr directly? I had this in my QEMU branch but hadn't posted yet because it was meant as a cleanup only. Paolo ----------------------- 8< --------------------- From 7ad6897f696034ef0651c912e43931a2b0bbe631 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini <pbonzini@redhat.com> Date: Mon, 12 Sep 2011 09:24:18 +0200 Subject: [PATCH] urcu-qsbr: use rcu_thread_offline/rcu_thread_online instead of inlining them --- urcu-qsbr.c | 40 +++++++++++++++++----------------------- 1 files changed, 17 insertions(+), 23 deletions(-) diff --git a/urcu-qsbr.c b/urcu-qsbr.c index 1dc9979..1adaa94 100644 --- a/urcu-qsbr.c +++ b/urcu-qsbr.c @@ -208,21 +208,17 @@ void synchronize_rcu(void) was_online = rcu_reader.ctr; /* All threads should read qparity before accessing data structure - * where new ptr points to. - */ - /* Write new ptr before changing the qparity */ - cmm_smp_mb(); - - /* + * where new ptr points to. In the "then" case, rcu_thread_offline + * includes a memory barrier. + * * Mark the writer thread offline to make sure we don't wait for * our own quiescent state. This allows using synchronize_rcu() * in threads registered as readers. */ - if (was_online) { - CMM_STORE_SHARED(rcu_reader.ctr, 0); - cmm_smp_mb(); /* write rcu_reader.ctr before read futex */ - wake_up_gp(); - } + if (was_online) + rcu_thread_offline(); + else + cmm_smp_mb(); mutex_lock(&rcu_gp_lock); @@ -263,9 +259,9 @@ out: * freed. */ if (was_online) - _CMM_STORE_SHARED(rcu_reader.ctr, - CMM_LOAD_SHARED(rcu_gp_ctr)); - cmm_smp_mb(); + rcu_thread_online(); + else + cmm_smp_mb(); } #else /* !(CAA_BITS_PER_LONG < 64) */ void synchronize_rcu(void) @@ -279,12 +275,10 @@ void synchronize_rcu(void) * our own quiescent state. This allows using synchronize_rcu() * in threads registered as readers. */ - cmm_smp_mb(); - if (was_online) { - CMM_STORE_SHARED(rcu_reader.ctr, 0); - cmm_smp_mb(); /* write rcu_reader.ctr before read futex */ - wake_up_gp(); - } + if (was_online) + rcu_thread_offline(); + else + cmm_smp_mb(); mutex_lock(&rcu_gp_lock); if (cds_list_empty(®istry)) @@ -294,9 +288,9 @@ out: mutex_unlock(&rcu_gp_lock); if (was_online) - _CMM_STORE_SHARED(rcu_reader.ctr, - CMM_LOAD_SHARED(rcu_gp_ctr)); - cmm_smp_mb(); + rcu_thread_online(); + else + cmm_smp_mb(); } #endif /* !(CAA_BITS_PER_LONG < 64) */ -- 1.7.6 ^ permalink raw reply [flat|nested] 3+ messages in thread
* [ltt-dev] Userspace RCU git fixes 2011-09-12 7:32 ` Paolo Bonzini @ 2011-09-13 17:53 ` Mathieu Desnoyers 0 siblings, 0 replies; 3+ messages in thread From: Mathieu Desnoyers @ 2011-09-13 17:53 UTC (permalink / raw) * Paolo Bonzini (pbonzini at redhat.com) wrote: > > Just to let you know that I pushed two updates into urcu: one fixes a > > grace period hang caused by a missing wakeup in the synchronize_rcu QSBR > > code. This appears to hit us due to the more fine-grained wakeup > > code brought by Paolo. The wakeup was really missing from the > > synchronize_rcu code (so Paolo's code just triggered an existing > > problem). I thought it would be good to let you know the effect: grace > > periods are delayed forever. This problem never appeared in a release (I > > caught it before). > > Good catch. Why not use rcu_thread_offline/online in synchronize_rcu, > instead of touching rcu_reader.ctr directly? I had this in my QEMU > branch but hadn't posted yet because it was meant as a cleanup only. Yep. I think in the initial QSBR versions, we could remove one barrier by inlining this, but I think I recall it was only a compiler barrier, so I agree that it's better to make the code easier to manage than save this possibly effect-less barrier (it is immediately followed by a mutex lock and preceded by a mutex unlock). Merged, Thanks! Mathieu > > Paolo > > ----------------------- 8< --------------------- > > From 7ad6897f696034ef0651c912e43931a2b0bbe631 Mon Sep 17 00:00:00 2001 > From: Paolo Bonzini <pbonzini@redhat.com> > Date: Mon, 12 Sep 2011 09:24:18 +0200 > Subject: [PATCH] urcu-qsbr: use rcu_thread_offline/rcu_thread_online instead of inlining them > > --- > urcu-qsbr.c | 40 +++++++++++++++++----------------------- > 1 files changed, 17 insertions(+), 23 deletions(-) > > diff --git a/urcu-qsbr.c b/urcu-qsbr.c > index 1dc9979..1adaa94 100644 > --- a/urcu-qsbr.c > +++ b/urcu-qsbr.c > @@ -208,21 +208,17 @@ void synchronize_rcu(void) > was_online = rcu_reader.ctr; > > /* All threads should read qparity before accessing data structure > - * where new ptr points to. > - */ > - /* Write new ptr before changing the qparity */ > - cmm_smp_mb(); > - > - /* > + * where new ptr points to. In the "then" case, rcu_thread_offline > + * includes a memory barrier. > + * > * Mark the writer thread offline to make sure we don't wait for > * our own quiescent state. This allows using synchronize_rcu() > * in threads registered as readers. > */ > - if (was_online) { > - CMM_STORE_SHARED(rcu_reader.ctr, 0); > - cmm_smp_mb(); /* write rcu_reader.ctr before read futex */ > - wake_up_gp(); > - } > + if (was_online) > + rcu_thread_offline(); > + else > + cmm_smp_mb(); > > mutex_lock(&rcu_gp_lock); > > @@ -263,9 +259,9 @@ out: > * freed. > */ > if (was_online) > - _CMM_STORE_SHARED(rcu_reader.ctr, > - CMM_LOAD_SHARED(rcu_gp_ctr)); > - cmm_smp_mb(); > + rcu_thread_online(); > + else > + cmm_smp_mb(); > } > #else /* !(CAA_BITS_PER_LONG < 64) */ > void synchronize_rcu(void) > @@ -279,12 +275,10 @@ void synchronize_rcu(void) > * our own quiescent state. This allows using synchronize_rcu() > * in threads registered as readers. > */ > - cmm_smp_mb(); > - if (was_online) { > - CMM_STORE_SHARED(rcu_reader.ctr, 0); > - cmm_smp_mb(); /* write rcu_reader.ctr before read futex */ > - wake_up_gp(); > - } > + if (was_online) > + rcu_thread_offline(); > + else > + cmm_smp_mb(); > > mutex_lock(&rcu_gp_lock); > if (cds_list_empty(®istry)) > @@ -294,9 +288,9 @@ out: > mutex_unlock(&rcu_gp_lock); > > if (was_online) > - _CMM_STORE_SHARED(rcu_reader.ctr, > - CMM_LOAD_SHARED(rcu_gp_ctr)); > - cmm_smp_mb(); > + rcu_thread_online(); > + else > + cmm_smp_mb(); > } > #endif /* !(CAA_BITS_PER_LONG < 64) */ > > -- > 1.7.6 > -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-09-13 17:53 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-09-11 6:14 [ltt-dev] Userspace RCU git fixes Mathieu Desnoyers 2011-09-12 7:32 ` Paolo Bonzini 2011-09-13 17:53 ` Mathieu Desnoyers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox