[ltt-dev] Userspace RCU git fixes

[ltt-dev] Userspace RCU git fixes

[Mathieu Desnoyers]

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

[ltt-dev] Userspace RCU git fixes

[Paolo Bonzini]

> 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(&registry))
@@ -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

[ltt-dev] Userspace RCU git fixes

[Mathieu Desnoyers]

* 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(&registry))
> @@ -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