[ltt-dev] [PATCH 0/7] urcu,call_rcu: minior fixes, round2

[ltt-dev] [PATCH 0/7] urcu,call_rcu: minior fixes, round2

[Lai Jiangshan]

The first patch is the last patch of round1.

next patchsets will not be minior fixes.

Thanks.

Lai Jiangshan (7):
  protects call_rcu_data_list when remove node
  fix missing respond to a cancellation request
  Avoid thread exit unexpected
  Simplify defer_rcu() encoding
  avoid create call_rcu_data for child when unneed
  Cleanup call_rcu_data pointers before used in child
  Array initialized before pointer is planted

 urcu-call-rcu-impl.h |   17 +++++++++++++++
 urcu-defer-impl.h    |   57 +++++++++++++++++++++++++++++++------------------
 2 files changed, 53 insertions(+), 21 deletions(-)

-- 
1.7.4.4

[ltt-dev] [PATCH 1/7] urcu, call_rcu: protects call_rcu_data_list when remove node

[Lai Jiangshan]

Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
---
 urcu-call-rcu-impl.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
index f9250e8..87d9157 100644
--- a/urcu-call-rcu-impl.h
+++ b/urcu-call-rcu-impl.h
@@ -617,7 +617,10 @@ void call_rcu_data_free(struct call_rcu_data *crdp)
 		wake_call_rcu_thread(default_call_rcu_data);
 	}
 
+	call_rcu_lock(&call_rcu_mutex);
 	cds_list_del(&crdp->list);
+	call_rcu_unlock(&call_rcu_mutex);
+
 	free(crdp);
 }
 
-- 
1.7.4.4

[ltt-dev] [PATCH 1/7] urcu, call_rcu: protects call_rcu_data_list when remove node

[Mathieu Desnoyers]

* Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
> Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>

Merged, thanks!

Mathieu

> ---
>  urcu-call-rcu-impl.h |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
> index f9250e8..87d9157 100644
> --- a/urcu-call-rcu-impl.h
> +++ b/urcu-call-rcu-impl.h
> @@ -617,7 +617,10 @@ void call_rcu_data_free(struct call_rcu_data *crdp)
>  		wake_call_rcu_thread(default_call_rcu_data);
>  	}
>  
> +	call_rcu_lock(&call_rcu_mutex);
>  	cds_list_del(&crdp->list);
> +	call_rcu_unlock(&call_rcu_mutex);
> +
>  	free(crdp);
>  }
>  
> -- 
> 1.7.4.4
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [PATCH 2/7] urcu, defer_rcu: fix missing respond to a cancellation request

[Lai Jiangshan]

thread1:				thread2
					thr_defer()
						pthread_testcancel();
stop_defer_thread()				.
	pthread_cancel(tid_defer);		.
	wake_up_defer();			.
						wait_defer();
	pthread_join(tid_defer)

thread2 missing respond to the cancellation request from thread1
and wait callback forever.

thread1 wait thread2 forver by pthread_join():

Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
---
 urcu-defer-impl.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/urcu-defer-impl.h b/urcu-defer-impl.h
index d1ab046..6339747 100644
--- a/urcu-defer-impl.h
+++ b/urcu-defer-impl.h
@@ -187,6 +187,7 @@ static void wait_defer(void)
 {
 	uatomic_dec(&defer_thread_futex);
 	cmm_smp_mb();	/* Write futex before read queue */
+	pthread_testcancel();
 	if (rcu_defer_num_callbacks()) {
 		cmm_smp_mb();	/* Read queue before write futex */
 		/* Callbacks are queued, don't wait. */
@@ -359,7 +360,6 @@ void _defer_rcu(void (*fct)(void *p), void *p)
 void *thr_defer(void *args)
 {
 	for (;;) {
-		pthread_testcancel();
 		/*
 		 * "Be green". Don't wake up the CPU if there is no RCU work
 		 * to perform whatsoever. Aims at saving laptop battery life by
@@ -387,6 +387,7 @@ static void start_defer_thread(void)
 {
 	int ret;
 
+	uatomic_set(&defer_thread_futex, 0);
 	ret = pthread_create(&tid_defer, NULL, thr_defer, NULL);
 	assert(!ret);
 }
-- 
1.7.4.4

[ltt-dev] [PATCH 2/7] urcu, defer_rcu: fix missing respond to a cancellation request

[Mathieu Desnoyers]

* Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
> thread1:				thread2
> 					thr_defer()
> 						pthread_testcancel();
> stop_defer_thread()				.
> 	pthread_cancel(tid_defer);		.
> 	wake_up_defer();			.
> 						wait_defer();
> 	pthread_join(tid_defer)
> 
> thread2 missing respond to the cancellation request from thread1
> and wait callback forever.
> 
> thread1 wait thread2 forver by pthread_join():
> 
> Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
> ---
>  urcu-defer-impl.h |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/urcu-defer-impl.h b/urcu-defer-impl.h
> index d1ab046..6339747 100644
> --- a/urcu-defer-impl.h
> +++ b/urcu-defer-impl.h
> @@ -187,6 +187,7 @@ static void wait_defer(void)
>  {
>  	uatomic_dec(&defer_thread_futex);
>  	cmm_smp_mb();	/* Write futex before read queue */
> +	pthread_testcancel();
>  	if (rcu_defer_num_callbacks()) {
>  		cmm_smp_mb();	/* Read queue before write futex */
>  		/* Callbacks are queued, don't wait. */
> @@ -359,7 +360,6 @@ void _defer_rcu(void (*fct)(void *p), void *p)
>  void *thr_defer(void *args)
>  {
>  	for (;;) {
> -		pthread_testcancel();
>  		/*
>  		 * "Be green". Don't wake up the CPU if there is no RCU work
>  		 * to perform whatsoever. Aims at saving laptop battery life by

Shouldn't we just modify futex_noasync and futex_async to add a
pthread_testcancel there instead ? We should then document, in futex.h,
that our futex implementations are pthread cancellation points.

> @@ -387,6 +387,7 @@ static void start_defer_thread(void)
>  {
>  	int ret;
>  
> +	uatomic_set(&defer_thread_futex, 0);

this would be to handle a start/stop/start sequence, right ? If we put
the testcancel within the region where the defer thread can have the
futex value set to non-zero, I guess setting it back to zero upon
restart will be required. Instead of setting it back to 0 in the
"start", we could theoretically let stop_defer_thread() do it, because
the first "start" will already have its value set to 0. We have to
notice that this whole rcu_defer scheme does not deal well with fork()
that are not followed by exec() though. We should probably just
document this and not care about it: call_rcu() is a superset of
defer_rcu anyway, and we should encourage use of call_rcu.

Thanks,

Mathieu

>  	ret = pthread_create(&tid_defer, NULL, thr_defer, NULL);
>  	assert(!ret);
>  }
> -- 
> 1.7.4.4
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [PATCH 3/7] urcu, defer_rcu: Avoid thread exit unexpected

[Lai Jiangshan]

When a cancellation was requested, the target thread will exit
when it find any cancellation points.

Pthread cancellation is useful when further operations of one or
more threads are undesirable or unnecessary.
But thr_defer() need to release all lock before exit, unexpected
exiting will break such semantic.

pthread_testcancel() is not the only function who can creates
a cancellation points. Some other functions may also have
cancellation points. Example: poll() which in synchronize_rcu().

Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
---
 urcu-defer-impl.h |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/urcu-defer-impl.h b/urcu-defer-impl.h
index 6339747..49570ff 100644
--- a/urcu-defer-impl.h
+++ b/urcu-defer-impl.h
@@ -122,6 +122,7 @@ static pthread_mutex_t rcu_defer_mutex = PTHREAD_MUTEX_INITIALIZER;
 static pthread_mutex_t defer_thread_mutex = PTHREAD_MUTEX_INITIALIZER;
 
 static int32_t defer_thread_futex;
+static int32_t defer_thread_stop;
 
 /*
  * Written to only by each individual deferer. Read by both the deferer and
@@ -148,7 +149,6 @@ static void mutex_lock_defer(pthread_mutex_t *mutex)
 			perror("Error in pthread mutex lock");
 			exit(-1);
 		}
-		pthread_testcancel();
 		poll(NULL,0,10);
 	}
 #endif /* #else #ifndef DISTRUST_SIGNALS_EXTREME */
@@ -187,7 +187,8 @@ static void wait_defer(void)
 {
 	uatomic_dec(&defer_thread_futex);
 	cmm_smp_mb();	/* Write futex before read queue */
-	pthread_testcancel();
+	if (defer_thread_stop)
+		pthread_exit(0);
 	if (rcu_defer_num_callbacks()) {
 		cmm_smp_mb();	/* Read queue before write futex */
 		/* Callbacks are queued, don't wait. */
@@ -397,10 +398,21 @@ static void stop_defer_thread(void)
 	int ret;
 	void *tret;
 
-	pthread_cancel(tid_defer);
+	/*
+	 * stop_defer_thread():		tid_defer():
+	 *
+	 * set defer_thread_stop	set futex
+	 * mb()				mb()
+	 * test futex and wake		test defer_thread_stop and exit
+	 */
+	defer_thread_stop = 1;
+	cmm_smp_mb();
 	wake_up_defer();
+
 	ret = pthread_join(tid_defer, &tret);
 	assert(!ret);
+
+	defer_thread_stop = 0;
 }
 
 int rcu_defer_register_thread(void)
-- 
1.7.4.4

[ltt-dev] [PATCH 3/7] urcu, defer_rcu: Avoid thread exit unexpected

[Mathieu Desnoyers]

Hi Lai,

I combined your patches 3 and 4, and enhanced them, including the
comments I sent for patch #2:


commit d7ff6cee89d7f8b4183b783d3d0a337b27d10bd2
Author: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
Date:   Thu Sep 29 13:28:04 2011 -0400

    urcu_defer: Use cancellation flag instead of pthread_cancel()
    
    - Provides better control over cancellation point location.
    - Set the futex to 0 before exiting the defer thread.
    
    This patch combines and enhances patches from Lai Jiangshan:
      urcu,defer_rcu: fix missing respond to a cancellation request
      urcu,defer_rcu: Avoid thread exit unexpected
    
    Reported-by: Lai Jiangshan <laijs at cn.fujitsu.com>
    Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>

diff --git a/urcu-defer-impl.h b/urcu-defer-impl.h
index d1ab046..4211f7c 100644
--- a/urcu-defer-impl.h
+++ b/urcu-defer-impl.h
@@ -122,6 +122,7 @@ static pthread_mutex_t rcu_defer_mutex = PTHREAD_MUTEX_INITIALIZER;
 static pthread_mutex_t defer_thread_mutex = PTHREAD_MUTEX_INITIALIZER;
 
 static int32_t defer_thread_futex;
+static int32_t defer_thread_stop;
 
 /*
  * Written to only by each individual deferer. Read by both the deferer and
@@ -148,7 +149,6 @@ static void mutex_lock_defer(pthread_mutex_t *mutex)
 			perror("Error in pthread mutex lock");
 			exit(-1);
 		}
-		pthread_testcancel();
 		poll(NULL,0,10);
 	}
 #endif /* #else #ifndef DISTRUST_SIGNALS_EXTREME */
@@ -186,7 +186,13 @@ static unsigned long rcu_defer_num_callbacks(void)
 static void wait_defer(void)
 {
 	uatomic_dec(&defer_thread_futex);
-	cmm_smp_mb();	/* Write futex before read queue */
+	/* Write futex before read queue */
+	/* Write futex before read defer_thread_stop */
+	cmm_smp_mb();
+	if (_CMM_LOAD_SHARED(defer_thread_stop)) {
+		uatomic_set(&defer_thread_futex, 0);
+		pthread_exit(0);
+	}
 	if (rcu_defer_num_callbacks()) {
 		cmm_smp_mb();	/* Read queue before write futex */
 		/* Callbacks are queued, don't wait. */
@@ -359,7 +365,6 @@ void _defer_rcu(void (*fct)(void *p), void *p)
 void *thr_defer(void *args)
 {
 	for (;;) {
-		pthread_testcancel();
 		/*
 		 * "Be green". Don't wake up the CPU if there is no RCU work
 		 * to perform whatsoever. Aims at saving laptop battery life by
@@ -396,10 +401,17 @@ static void stop_defer_thread(void)
 	int ret;
 	void *tret;
 
-	pthread_cancel(tid_defer);
+	_CMM_STORE_SHARED(defer_thread_stop, 1);
+	/* Store defer_thread_stop before testing futex */
+	cmm_smp_mb();
 	wake_up_defer();
+
 	ret = pthread_join(tid_defer, &tret);
 	assert(!ret);
+
+	CMM_STORE_SHARED(defer_thread_stop, 0);
+	/* defer thread should always exit when futex value is 0 */
+	assert(uatomic_read(&defer_thread_futex) == 0);
 }
 
 int rcu_defer_register_thread(void)

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [PATCH 4/7] urcu, defer_rcu: Simplify defer_rcu() encoding

[Lai Jiangshan]

use the same code as function changed even only
just the data is not aligned.

Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
---
 urcu-defer-impl.h |   38 ++++++++++++++++++++------------------
 1 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/urcu-defer-impl.h b/urcu-defer-impl.h
index 49570ff..b1478ab 100644
--- a/urcu-defer-impl.h
+++ b/urcu-defer-impl.h
@@ -61,7 +61,7 @@
  * Assumes that (void *)-2L is not used often. Used to encode non-aligned
  * functions and non-aligned data using extra space.
  * We encode the (void *)-2L fct as: -2L, fct, data.
- * We encode the (void *)-2L data as: -2L, fct, data.
+ * We encode the (void *)-2L data as: fct | DQ_FCT_BIT, data or -2L, fct, data.
  * Here, DQ_FCT_MARK == ~DQ_FCT_BIT. Required for the test order.
  */
 #define DQ_FCT_BIT		(1 << 0)
@@ -318,14 +318,27 @@ void _defer_rcu(void (*fct)(void *p), void *p)
 		assert(head - CMM_LOAD_SHARED(defer_queue.tail) == 0);
 	}
 
-	if (unlikely(defer_queue.last_fct_in != fct)) {
+	/*
+	 * Encode:
+	 * if the function is not changed and the data is aligned and it is
+	 * not the marker:
+	 *	store the data
+	 * otherwise if the function is aligned and its not the marker:
+	 *	store the funciotn with DQ_FCT_BIT
+	 *	store the data
+	 * otherwise:
+	 *	store the marker (DQ_FCT_MARK)
+	 *	store the funciotn
+	 *	store the data
+	 *
+	 * Decode: see the comments before 'struct defer_queue'
+	 *         or the code in rcu_defer_barrier_queue().
+	 */
+	if (unlikely(defer_queue.last_fct_in != fct
+			|| DQ_IS_FCT_BIT(p)
+			|| p == DQ_FCT_MARK)) {
 		defer_queue.last_fct_in = fct;
 		if (unlikely(DQ_IS_FCT_BIT(fct) || fct == DQ_FCT_MARK)) {
-			/*
-			 * If the function to encode is not aligned or the
-			 * marker, write DQ_FCT_MARK followed by the function
-			 * pointer.
-			 */
 			_CMM_STORE_SHARED(defer_queue.q[head++ & DEFER_QUEUE_MASK],
 				      DQ_FCT_MARK);
 			_CMM_STORE_SHARED(defer_queue.q[head++ & DEFER_QUEUE_MASK],
@@ -335,17 +348,6 @@ void _defer_rcu(void (*fct)(void *p), void *p)
 			_CMM_STORE_SHARED(defer_queue.q[head++ & DEFER_QUEUE_MASK],
 				      fct);
 		}
-	} else {
-		if (unlikely(DQ_IS_FCT_BIT(p) || p == DQ_FCT_MARK)) {
-			/*
-			 * If the data to encode is not aligned or the marker,
-			 * write DQ_FCT_MARK followed by the function pointer.
-			 */
-			_CMM_STORE_SHARED(defer_queue.q[head++ & DEFER_QUEUE_MASK],
-				      DQ_FCT_MARK);
-			_CMM_STORE_SHARED(defer_queue.q[head++ & DEFER_QUEUE_MASK],
-				      fct);
-		}
 	}
 	_CMM_STORE_SHARED(defer_queue.q[head++ & DEFER_QUEUE_MASK], p);
 	cmm_smp_wmb();	/* Publish new pointer before head */
-- 
1.7.4.4

[ltt-dev] [PATCH 4/7] urcu, defer_rcu: Simplify defer_rcu() encoding

[Mathieu Desnoyers]

Did an overhaul of this patch (mainly cleaned up comments and
changelog), merged, thanks!

Mathieu

commit 2ef1bfb2cb9abb657e077814e272801e197a3c47
Author: Lai Jiangshan <laijs at cn.fujitsu.com>
Date:   Thu Sep 29 13:47:13 2011 -0400

    urcu,defer_rcu: Make defer_rcu encoding more compact for marker
    
    When the function changes (and the function is aligned), and only the
    data is the marker, we can get away with using only 2 pointers rather
    than 3.
    
    [ Edit by Mathieu Desnoyers: patch cleanup, changelog updates ]
    
    Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
    Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>

diff --git a/urcu-defer-impl.h b/urcu-defer-impl.h
index 4211f7c..34d99c9 100644
--- a/urcu-defer-impl.h
+++ b/urcu-defer-impl.h
@@ -61,7 +61,9 @@
  * Assumes that (void *)-2L is not used often. Used to encode non-aligned
  * functions and non-aligned data using extra space.
  * We encode the (void *)-2L fct as: -2L, fct, data.
- * We encode the (void *)-2L data as: -2L, fct, data.
+ * We encode the (void *)-2L data as either:
+ *   fct | DQ_FCT_BIT, data (if fct is aligned), or
+ *   -2L, fct, data (if fct is not aligned).
  * Here, DQ_FCT_MARK == ~DQ_FCT_BIT. Required for the test order.
  */
 #define DQ_FCT_BIT		(1 << 0)
@@ -322,14 +324,27 @@ void _defer_rcu(void (*fct)(void *p), void *p)
 		assert(head - CMM_LOAD_SHARED(defer_queue.tail) == 0);
 	}
 
-	if (unlikely(defer_queue.last_fct_in != fct)) {
+	/*
+	 * Encode:
+	 * if the function is not changed and the data is aligned and it is
+	 * not the marker:
+	 *	store the data
+	 * otherwise if the function is aligned and its not the marker:
+	 *	store the function with DQ_FCT_BIT
+	 *	store the data
+	 * otherwise:
+	 *	store the marker (DQ_FCT_MARK)
+	 *	store the function
+	 *	store the data
+	 *
+	 * Decode: see the comments before 'struct defer_queue'
+	 *         or the code in rcu_defer_barrier_queue().
+	 */
+	if (unlikely(defer_queue.last_fct_in != fct
+			|| DQ_IS_FCT_BIT(p)
+			|| p == DQ_FCT_MARK)) {
 		defer_queue.last_fct_in = fct;
 		if (unlikely(DQ_IS_FCT_BIT(fct) || fct == DQ_FCT_MARK)) {
-			/*
-			 * If the function to encode is not aligned or the
-			 * marker, write DQ_FCT_MARK followed by the function
-			 * pointer.
-			 */
 			_CMM_STORE_SHARED(defer_queue.q[head++ & DEFER_QUEUE_MASK],
 				      DQ_FCT_MARK);
 			_CMM_STORE_SHARED(defer_queue.q[head++ & DEFER_QUEUE_MASK],
@@ -339,17 +354,6 @@ void _defer_rcu(void (*fct)(void *p), void *p)
 			_CMM_STORE_SHARED(defer_queue.q[head++ & DEFER_QUEUE_MASK],
 				      fct);
 		}
-	} else {
-		if (unlikely(DQ_IS_FCT_BIT(p) || p == DQ_FCT_MARK)) {
-			/*
-			 * If the data to encode is not aligned or the marker,
-			 * write DQ_FCT_MARK followed by the function pointer.
-			 */
-			_CMM_STORE_SHARED(defer_queue.q[head++ & DEFER_QUEUE_MASK],
-				      DQ_FCT_MARK);
-			_CMM_STORE_SHARED(defer_queue.q[head++ & DEFER_QUEUE_MASK],
-				      fct);
-		}
 	}
 	_CMM_STORE_SHARED(defer_queue.q[head++ & DEFER_QUEUE_MASK], p);
 	cmm_smp_wmb();	/* Publish new pointer before head */
-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [PATCH 5/7] urcu, call_rcu: avoid create call_rcu_data for child when unneed

[Lai Jiangshan]

Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
---
 urcu-call-rcu-impl.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
index 87d9157..65c1c7a 100644
--- a/urcu-call-rcu-impl.h
+++ b/urcu-call-rcu-impl.h
@@ -675,6 +675,10 @@ void call_rcu_after_fork_child(void)
 	/* Release the mutex. */
 	call_rcu_unlock(&call_rcu_mutex);
 
+	/* Do nothing when call_rcu() has not been used */
+	if (cds_list_empty(&call_rcu_data_list))
+		return;
+
 	/*
 	 * Allocate a new default call_rcu_data structure in order
 	 * to get a working call_rcu thread to go with it.
-- 
1.7.4.4

[ltt-dev] [PATCH 5/7] urcu, call_rcu: avoid create call_rcu_data for child when unneed

[Paul E. McKenney]

On Wed, Sep 28, 2011 at 04:34:31PM +0800, Lai Jiangshan wrote:
> Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>

Nice optimization!!!

Reviewed-by: Paul E. McKenney <paulmck at linux.vnet.ibm.com>

> ---
>  urcu-call-rcu-impl.h |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
> index 87d9157..65c1c7a 100644
> --- a/urcu-call-rcu-impl.h
> +++ b/urcu-call-rcu-impl.h
> @@ -675,6 +675,10 @@ void call_rcu_after_fork_child(void)
>  	/* Release the mutex. */
>  	call_rcu_unlock(&call_rcu_mutex);
> 
> +	/* Do nothing when call_rcu() has not been used */
> +	if (cds_list_empty(&call_rcu_data_list))
> +		return;
> +
>  	/*
>  	 * Allocate a new default call_rcu_data structure in order
>  	 * to get a working call_rcu thread to go with it.
> -- 
> 1.7.4.4
> 

[ltt-dev] [PATCH 5/7] urcu, call_rcu: avoid create call_rcu_data for child when unneed

[Mathieu Desnoyers]

* Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> On Wed, Sep 28, 2011 at 04:34:31PM +0800, Lai Jiangshan wrote:
> > Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
> 
> Nice optimization!!!
> 
> Reviewed-by: Paul E. McKenney <paulmck at linux.vnet.ibm.com>

Merged,

Thanks for both of you for the patch and review.

Mathieu

> 
> > ---
> >  urcu-call-rcu-impl.h |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
> > index 87d9157..65c1c7a 100644
> > --- a/urcu-call-rcu-impl.h
> > +++ b/urcu-call-rcu-impl.h
> > @@ -675,6 +675,10 @@ void call_rcu_after_fork_child(void)
> >  	/* Release the mutex. */
> >  	call_rcu_unlock(&call_rcu_mutex);
> > 
> > +	/* Do nothing when call_rcu() has not been used */
> > +	if (cds_list_empty(&call_rcu_data_list))
> > +		return;
> > +
> >  	/*
> >  	 * Allocate a new default call_rcu_data structure in order
> >  	 * to get a working call_rcu thread to go with it.
> > -- 
> > 1.7.4.4
> > 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [PATCH 6/7] urcu, call_rcu: Cleanup call_rcu_data pointers before used in child

[Lai Jiangshan]

Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
---
 urcu-call-rcu-impl.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
index 65c1c7a..3c68ae7 100644
--- a/urcu-call-rcu-impl.h
+++ b/urcu-call-rcu-impl.h
@@ -686,6 +686,12 @@ void call_rcu_after_fork_child(void)
 	default_call_rcu_data = NULL;
 	(void)get_default_call_rcu_data();
 
+	/* Cleanup call_rcu_data pointers before used */
+	maxcpus = 0;
+	free(per_cpu_call_rcu_data);
+	per_cpu_call_rcu_data = NULL;
+	thread_call_rcu_data = NULL;
+
 	/* Dispose of all of the rest of the call_rcu_data structures. */
 	cds_list_for_each_entry_safe(crdp, next, &call_rcu_data_list, list) {
 		if (crdp == default_call_rcu_data)
-- 
1.7.4.4

[ltt-dev] [PATCH 6/7] urcu, call_rcu: Cleanup call_rcu_data pointers before used in child

[Paul E. McKenney]

On Wed, Sep 28, 2011 at 04:34:32PM +0800, Lai Jiangshan wrote:
> Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
> ---
>  urcu-call-rcu-impl.h |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
> index 65c1c7a..3c68ae7 100644
> --- a/urcu-call-rcu-impl.h
> +++ b/urcu-call-rcu-impl.h
> @@ -686,6 +686,12 @@ void call_rcu_after_fork_child(void)
>  	default_call_rcu_data = NULL;
>  	(void)get_default_call_rcu_data();
> 
> +	/* Cleanup call_rcu_data pointers before used */
> +	maxcpus = 0;
> +	free(per_cpu_call_rcu_data);
> +	per_cpu_call_rcu_data = NULL;

Good catch!  I would expect that the number of CPUs would be the
same for the child as it was for the parent, but doesn't hurt to
make the child start over.

> +	thread_call_rcu_data = NULL;

Isn't thread_call_rcu_data already NULL due to the child being a new
thread and the C initialization-to-zero rules?

							Thanx, Paul

> +
>  	/* Dispose of all of the rest of the call_rcu_data structures. */
>  	cds_list_for_each_entry_safe(crdp, next, &call_rcu_data_list, list) {
>  		if (crdp == default_call_rcu_data)
> -- 
> 1.7.4.4
> 

[ltt-dev] [PATCH 6/7] urcu, call_rcu: Cleanup call_rcu_data pointers before used in child

[Mathieu Desnoyers]

* Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> On Wed, Sep 28, 2011 at 04:34:32PM +0800, Lai Jiangshan wrote:
> > Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
> > ---
> >  urcu-call-rcu-impl.h |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
> > index 65c1c7a..3c68ae7 100644
> > --- a/urcu-call-rcu-impl.h
> > +++ b/urcu-call-rcu-impl.h
> > @@ -686,6 +686,12 @@ void call_rcu_after_fork_child(void)
> >  	default_call_rcu_data = NULL;
> >  	(void)get_default_call_rcu_data();
> > 
> > +	/* Cleanup call_rcu_data pointers before used */
> > +	maxcpus = 0;
> > +	free(per_cpu_call_rcu_data);
> > +	per_cpu_call_rcu_data = NULL;
> 
> Good catch!  I would expect that the number of CPUs would be the
> same for the child as it was for the parent, but doesn't hurt to
> make the child start over.
> 
> > +	thread_call_rcu_data = NULL;
> 
> Isn't thread_call_rcu_data already NULL due to the child being a new
> thread and the C initialization-to-zero rules?

#include <unistd.h>
#include <stdio.h>

int __thread a;

int main()
{
	 int pid;

	 a = 1;
	 pid = fork();
	 if (pid > 0) {
		  printf("parent val %d\n", a);
		  return 0;
	 } else {
		  printf("child val %d\n", a);
		  return 0;
	 }
}

compudj at thinkos:/tmp$ gcc -o test test.c
compudj at thinkos:/tmp$ ./test
parent val 1
child val 1

AFAIK, the c initialization to zero rules apply to exec(), not fork.
Here we are taking care of after-fork in the child, in cases where the
child is not doing an exec.

Does that make more sense ?

Thanks,

Mathieu


> 
> 							Thanx, Paul
> 
> > +
> >  	/* Dispose of all of the rest of the call_rcu_data structures. */
> >  	cds_list_for_each_entry_safe(crdp, next, &call_rcu_data_list, list) {
> >  		if (crdp == default_call_rcu_data)
> > -- 
> > 1.7.4.4
> > 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [PATCH 6/7] urcu, call_rcu: Cleanup call_rcu_data pointers before used in child

[Paul E. McKenney]

On Thu, Sep 29, 2011 at 01:59:12PM -0400, Mathieu Desnoyers wrote:
> * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > On Wed, Sep 28, 2011 at 04:34:32PM +0800, Lai Jiangshan wrote:
> > > Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
> > > ---
> > >  urcu-call-rcu-impl.h |    6 ++++++
> > >  1 files changed, 6 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
> > > index 65c1c7a..3c68ae7 100644
> > > --- a/urcu-call-rcu-impl.h
> > > +++ b/urcu-call-rcu-impl.h
> > > @@ -686,6 +686,12 @@ void call_rcu_after_fork_child(void)
> > >  	default_call_rcu_data = NULL;
> > >  	(void)get_default_call_rcu_data();
> > > 
> > > +	/* Cleanup call_rcu_data pointers before used */
> > > +	maxcpus = 0;
> > > +	free(per_cpu_call_rcu_data);
> > > +	per_cpu_call_rcu_data = NULL;
> > 
> > Good catch!  I would expect that the number of CPUs would be the
> > same for the child as it was for the parent, but doesn't hurt to
> > make the child start over.
> > 
> > > +	thread_call_rcu_data = NULL;
> > 
> > Isn't thread_call_rcu_data already NULL due to the child being a new
> > thread and the C initialization-to-zero rules?
> 
> #include <unistd.h>
> #include <stdio.h>
> 
> int __thread a;
> 
> int main()
> {
> 	 int pid;
> 
> 	 a = 1;
> 	 pid = fork();
> 	 if (pid > 0) {
> 		  printf("parent val %d\n", a);
> 		  return 0;
> 	 } else {
> 		  printf("child val %d\n", a);
> 		  return 0;
> 	 }
> }
> 
> compudj at thinkos:/tmp$ gcc -o test test.c
> compudj at thinkos:/tmp$ ./test
> parent val 1
> child val 1
> 
> AFAIK, the c initialization to zero rules apply to exec(), not fork.
> Here we are taking care of after-fork in the child, in cases where the
> child is not doing an exec.
> 
> Does that make more sense ?

I would think that the forked child would be a new thread, but obviously
not!  I stand corrected.

							Thanx, Paul

> Thanks,
> 
> Mathieu
> 
> 
> > 
> > 							Thanx, Paul
> > 
> > > +
> > >  	/* Dispose of all of the rest of the call_rcu_data structures. */
> > >  	cds_list_for_each_entry_safe(crdp, next, &call_rcu_data_list, list) {
> > >  		if (crdp == default_call_rcu_data)
> > > -- 
> > > 1.7.4.4
> > > 
> 
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com
> 

[ltt-dev] [PATCH 6/7] urcu, call_rcu: Cleanup call_rcu_data pointers before used in child

[Mathieu Desnoyers]

* Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> On Thu, Sep 29, 2011 at 01:59:12PM -0400, Mathieu Desnoyers wrote:
> > * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > > On Wed, Sep 28, 2011 at 04:34:32PM +0800, Lai Jiangshan wrote:
> > > > Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
> > > > ---
> > > >  urcu-call-rcu-impl.h |    6 ++++++
> > > >  1 files changed, 6 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
> > > > index 65c1c7a..3c68ae7 100644
> > > > --- a/urcu-call-rcu-impl.h
> > > > +++ b/urcu-call-rcu-impl.h
> > > > @@ -686,6 +686,12 @@ void call_rcu_after_fork_child(void)
> > > >  	default_call_rcu_data = NULL;
> > > >  	(void)get_default_call_rcu_data();
> > > > 
> > > > +	/* Cleanup call_rcu_data pointers before used */
> > > > +	maxcpus = 0;
> > > > +	free(per_cpu_call_rcu_data);
> > > > +	per_cpu_call_rcu_data = NULL;
> > > 
> > > Good catch!  I would expect that the number of CPUs would be the
> > > same for the child as it was for the parent, but doesn't hurt to
> > > make the child start over.
> > > 
> > > > +	thread_call_rcu_data = NULL;
> > > 
> > > Isn't thread_call_rcu_data already NULL due to the child being a new
> > > thread and the C initialization-to-zero rules?
> > 
> > #include <unistd.h>
> > #include <stdio.h>
> > 
> > int __thread a;
> > 
> > int main()
> > {
> > 	 int pid;
> > 
> > 	 a = 1;
> > 	 pid = fork();
> > 	 if (pid > 0) {
> > 		  printf("parent val %d\n", a);
> > 		  return 0;
> > 	 } else {
> > 		  printf("child val %d\n", a);
> > 		  return 0;
> > 	 }
> > }
> > 
> > compudj at thinkos:/tmp$ gcc -o test test.c
> > compudj at thinkos:/tmp$ ./test
> > parent val 1
> > child val 1
> > 
> > AFAIK, the c initialization to zero rules apply to exec(), not fork.
> > Here we are taking care of after-fork in the child, in cases where the
> > child is not doing an exec.
> > 
> > Does that make more sense ?
> 
> I would think that the forked child would be a new thread, but obviously
> not!  I stand corrected.

It all makes more sense when you consider the following paragraph of
pthread_atfork(3):

       To understand the purpose of pthread_atfork, recall that fork(2) dupli?
       cates the whole memory space, including mutexes in their current  lock?
       ing  state,  but only the calling thread: other threads are not running
       in the child process.  The mutexes are not usable after  the  fork  and
       must be initialized with pthread_mutex_init in the child process.  This
       is a limitation of the current implementation and might or might not be
       present in future versions.

Thanks,

Mathieu

> 
> 							Thanx, Paul
> 
> > Thanks,
> > 
> > Mathieu
> > 
> > 
> > > 
> > > 							Thanx, Paul
> > > 
> > > > +
> > > >  	/* Dispose of all of the rest of the call_rcu_data structures. */
> > > >  	cds_list_for_each_entry_safe(crdp, next, &call_rcu_data_list, list) {
> > > >  		if (crdp == default_call_rcu_data)
> > > > -- 
> > > > 1.7.4.4
> > > > 
> > 
> > -- 
> > Mathieu Desnoyers
> > Operating System Efficiency R&D Consultant
> > EfficiOS Inc.
> > http://www.efficios.com
> > 
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [PATCH 6/7] urcu, call_rcu: Cleanup call_rcu_data pointers before used in child

[Paul E. McKenney]

On Thu, Sep 29, 2011 at 02:19:47PM -0400, Mathieu Desnoyers wrote:
> * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > On Thu, Sep 29, 2011 at 01:59:12PM -0400, Mathieu Desnoyers wrote:
> > > * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > > > On Wed, Sep 28, 2011 at 04:34:32PM +0800, Lai Jiangshan wrote:
> > > > > Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
> > > > > ---
> > > > >  urcu-call-rcu-impl.h |    6 ++++++
> > > > >  1 files changed, 6 insertions(+), 0 deletions(-)
> > > > > 
> > > > > diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
> > > > > index 65c1c7a..3c68ae7 100644
> > > > > --- a/urcu-call-rcu-impl.h
> > > > > +++ b/urcu-call-rcu-impl.h
> > > > > @@ -686,6 +686,12 @@ void call_rcu_after_fork_child(void)
> > > > >  	default_call_rcu_data = NULL;
> > > > >  	(void)get_default_call_rcu_data();
> > > > > 
> > > > > +	/* Cleanup call_rcu_data pointers before used */
> > > > > +	maxcpus = 0;
> > > > > +	free(per_cpu_call_rcu_data);
> > > > > +	per_cpu_call_rcu_data = NULL;
> > > > 
> > > > Good catch!  I would expect that the number of CPUs would be the
> > > > same for the child as it was for the parent, but doesn't hurt to
> > > > make the child start over.
> > > > 
> > > > > +	thread_call_rcu_data = NULL;
> > > > 
> > > > Isn't thread_call_rcu_data already NULL due to the child being a new
> > > > thread and the C initialization-to-zero rules?
> > > 
> > > #include <unistd.h>
> > > #include <stdio.h>
> > > 
> > > int __thread a;
> > > 
> > > int main()
> > > {
> > > 	 int pid;
> > > 
> > > 	 a = 1;
> > > 	 pid = fork();
> > > 	 if (pid > 0) {
> > > 		  printf("parent val %d\n", a);
> > > 		  return 0;
> > > 	 } else {
> > > 		  printf("child val %d\n", a);
> > > 		  return 0;
> > > 	 }
> > > }
> > > 
> > > compudj at thinkos:/tmp$ gcc -o test test.c
> > > compudj at thinkos:/tmp$ ./test
> > > parent val 1
> > > child val 1
> > > 
> > > AFAIK, the c initialization to zero rules apply to exec(), not fork.
> > > Here we are taking care of after-fork in the child, in cases where the
> > > child is not doing an exec.
> > > 
> > > Does that make more sense ?
> > 
> > I would think that the forked child would be a new thread, but obviously
> > not!  I stand corrected.
> 
> It all makes more sense when you consider the following paragraph of
> pthread_atfork(3):
> 
>        To understand the purpose of pthread_atfork, recall that fork(2) dupli?
>        cates the whole memory space, including mutexes in their current  lock?
>        ing  state,  but only the calling thread: other threads are not running
>        in the child process.  The mutexes are not usable after  the  fork  and
>        must be initialized with pthread_mutex_init in the child process.  This
>        is a limitation of the current implementation and might or might not be
>        present in future versions.

I clearly need to get out of the kernel more often.  ;-)

							Thanx, Paul

[ltt-dev] [PATCH 6/7] urcu, call_rcu: Cleanup call_rcu_data pointers before used in child

[Mathieu Desnoyers]

* Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
> Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
> ---
>  urcu-call-rcu-impl.h |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
> index 65c1c7a..3c68ae7 100644
> --- a/urcu-call-rcu-impl.h
> +++ b/urcu-call-rcu-impl.h
> @@ -686,6 +686,12 @@ void call_rcu_after_fork_child(void)
>  	default_call_rcu_data = NULL;
>  	(void)get_default_call_rcu_data();

We should probably free the old default_call_rcu_data before ? and
remove it from the call_rcu_data_list ? What about the cds queues
content ?

>  
> +	/* Cleanup call_rcu_data pointers before used */
> +	maxcpus = 0;
> +	free(per_cpu_call_rcu_data);

Shouldn't we clean the cds queues and remove it from call_rcu_data_list
too ?

> +	per_cpu_call_rcu_data = NULL;
> +	thread_call_rcu_data = NULL;

Same question applies to thread_call_rcu_data.

Thanks,

Mathieu

> +
>  	/* Dispose of all of the rest of the call_rcu_data structures. */
>  	cds_list_for_each_entry_safe(crdp, next, &call_rcu_data_list, list) {
>  		if (crdp == default_call_rcu_data)
> -- 
> 1.7.4.4
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [PATCH 6/7] urcu, call_rcu: Cleanup call_rcu_data pointers before used in child

[Mathieu Desnoyers]

* Mathieu Desnoyers (mathieu.desnoyers at efficios.com) wrote:
> * Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
> > Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
> > ---
> >  urcu-call-rcu-impl.h |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
> > index 65c1c7a..3c68ae7 100644
> > --- a/urcu-call-rcu-impl.h
> > +++ b/urcu-call-rcu-impl.h
> > @@ -686,6 +686,12 @@ void call_rcu_after_fork_child(void)
> >  	default_call_rcu_data = NULL;
> >  	(void)get_default_call_rcu_data();
> 
> We should probably free the old default_call_rcu_data before ? and
> remove it from the call_rcu_data_list ? What about the cds queues
> content ?
> 
> >  
> > +	/* Cleanup call_rcu_data pointers before used */
> > +	maxcpus = 0;
> > +	free(per_cpu_call_rcu_data);
> 
> Shouldn't we clean the cds queues and remove it from call_rcu_data_list
> too ?
> 
> > +	per_cpu_call_rcu_data = NULL;
> > +	thread_call_rcu_data = NULL;
> 
> Same question applies to thread_call_rcu_data.

Forget it, I see the 

        /* Dispose of all of the rest of the call_rcu_data structures. */
        cds_list_for_each_entry_safe(crdp, next, &call_rcu_data_list, list) {

iteration that follows. It takes care of my concerns.

Thanks,

Mathieu


> 
> Thanks,
> 
> Mathieu
> 
> > +
> >  	/* Dispose of all of the rest of the call_rcu_data structures. */
> >  	cds_list_for_each_entry_safe(crdp, next, &call_rcu_data_list, list) {
> >  		if (crdp == default_call_rcu_data)
> > -- 
> > 1.7.4.4
> > 
> 
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [PATCH 6/7] urcu, call_rcu: Cleanup call_rcu_data pointers before used in child

[Mathieu Desnoyers]

Merged, with some changes:

commit 60af049d5e1d17e7ffdfd139bb486bd969c6a76c
Author: Lai Jiangshan <laijs at cn.fujitsu.com>
Date:   Thu Sep 29 15:56:43 2011 -0400

    urcu,call_rcu: Cleanup call_rcu_data pointers before use in child
    
    [ Edit by Mathieu Desnoyers: create maxcpus_reset to handle cases where
      maxcpus is 0 and -1, depending on the configuration. ]
    
    Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
    Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>

diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
index d964c47..0a47d96 100644
--- a/urcu-call-rcu-impl.h
+++ b/urcu-call-rcu-impl.h
@@ -88,6 +88,11 @@ static struct call_rcu_data *default_call_rcu_data;
 static struct call_rcu_data **per_cpu_call_rcu_data;
 static long maxcpus;
 
+static void maxcpus_reset(void)
+{
+	maxcpus = 0;
+}
+
 /* Allocate the array if it has not already been allocated. */
 
 static void alloc_cpu_call_rcu_data(void)
@@ -123,6 +128,10 @@ static void alloc_cpu_call_rcu_data(void)
 static struct call_rcu_data **per_cpu_call_rcu_data = NULL;
 static const long maxcpus = -1;
 
+static void maxcpus_reset(void)
+{
+}
+
 static void alloc_cpu_call_rcu_data(void)
 {
 }
@@ -688,6 +697,12 @@ void call_rcu_after_fork_child(void)
 	default_call_rcu_data = NULL;
 	(void)get_default_call_rcu_data();
 
+	/* Cleanup call_rcu_data pointers before use */
+	maxcpus_reset();
+	free(per_cpu_call_rcu_data);
+	per_cpu_call_rcu_data = NULL;
+	thread_call_rcu_data = NULL;
+
 	/* Dispose of all of the rest of the call_rcu_data structures. */
 	cds_list_for_each_entry_safe(crdp, next, &call_rcu_data_list, list) {
 		if (crdp == default_call_rcu_data)

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [PATCH 7/7] urcu, call_rcu: Array initialized before pointer is planted

[Lai Jiangshan]

Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
---
 urcu-call-rcu-impl.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
index 3c68ae7..462139a 100644
--- a/urcu-call-rcu-impl.h
+++ b/urcu-call-rcu-impl.h
@@ -104,6 +104,10 @@ static void alloc_cpu_call_rcu_data(void)
 	p = malloc(maxcpus * sizeof(*per_cpu_call_rcu_data));
 	if (p != NULL) {
 		memset(p, '\0', maxcpus * sizeof(*per_cpu_call_rcu_data));
+
+		/* Array initialized before pointer is planted. */
+		cmm_smp_mb();
+
 		per_cpu_call_rcu_data = p;
 	} else {
 		if (!warned) {
-- 
1.7.4.4

[ltt-dev] [PATCH 7/7] urcu, call_rcu: Array initialized before pointer is planted

[Paul E. McKenney]

On Wed, Sep 28, 2011 at 04:34:33PM +0800, Lai Jiangshan wrote:
> Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>

This one I don't understand.  It looks to me that all dereferences
of the per_cpu_call_rcu_data pointer are done while holding the
call_rcu_mutex, so there should be no need for the memory barrier.

Yes, there is an access to the pointer without lock protection
in create_all_cpu_call_rcu_data(), but that access does not
do a dereference, so still no need for the memory barrier.

So, what am I missing?

							Thanx, Paul

> ---
>  urcu-call-rcu-impl.h |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
> index 3c68ae7..462139a 100644
> --- a/urcu-call-rcu-impl.h
> +++ b/urcu-call-rcu-impl.h
> @@ -104,6 +104,10 @@ static void alloc_cpu_call_rcu_data(void)
>  	p = malloc(maxcpus * sizeof(*per_cpu_call_rcu_data));
>  	if (p != NULL) {
>  		memset(p, '\0', maxcpus * sizeof(*per_cpu_call_rcu_data));
> +
> +		/* Array initialized before pointer is planted. */
> +		cmm_smp_mb();
> +
>  		per_cpu_call_rcu_data = p;
>  	} else {
>  		if (!warned) {
> -- 
> 1.7.4.4
> 

[ltt-dev] [PATCH 7/7] urcu, call_rcu: Array initialized before pointer is planted

[Mathieu Desnoyers]

* Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> On Wed, Sep 28, 2011 at 04:34:33PM +0800, Lai Jiangshan wrote:
> > Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
> 
> This one I don't understand.  It looks to me that all dereferences
> of the per_cpu_call_rcu_data pointer are done while holding the
> call_rcu_mutex, so there should be no need for the memory barrier.
> 
> Yes, there is an access to the pointer without lock protection
> in create_all_cpu_call_rcu_data(), but that access does not
> do a dereference, so still no need for the memory barrier.
> 
> So, what am I missing?

A concurrent get_cpu_call_rcu_data(), called by get_call_rcu_data(),
could dereference this pointer without holding any mutex. So this
situation would happen if we have a concurrent call_rcu() executing
while we do the create_all_cpu_call_rcu_data().

I think we would need to put a rcu_dereference() around
per_cpu_call_rcu_data read within get_cpu_call_rcu_data() too.
per_cpu_call_rcu_data should be done with rcu_set_pointer.

Also, a rcu read-side critical section would be required around any
usage of per_cpu_call_rcu_data, and the action of tearing down the
per-cpu data would require to wait for a quiescent state. So we would
basically require that the call_rcu users need to be registered as
RCU reader threads.

Or we clearly state that no call_rcu should happen while
create_all_cpu_call_rcu_data is executed. But given we allow call_rcu to
be called within call_rcu callbacks, this seems to be hard to guarantee.

Thoughts ?

Mathieu

> 
> 							Thanx, Paul
> 
> > ---
> >  urcu-call-rcu-impl.h |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
> > index 3c68ae7..462139a 100644
> > --- a/urcu-call-rcu-impl.h
> > +++ b/urcu-call-rcu-impl.h
> > @@ -104,6 +104,10 @@ static void alloc_cpu_call_rcu_data(void)
> >  	p = malloc(maxcpus * sizeof(*per_cpu_call_rcu_data));
> >  	if (p != NULL) {
> >  		memset(p, '\0', maxcpus * sizeof(*per_cpu_call_rcu_data));
> > +
> > +		/* Array initialized before pointer is planted. */
> > +		cmm_smp_mb();
> > +
> >  		per_cpu_call_rcu_data = p;
> >  	} else {
> >  		if (!warned) {
> > -- 
> > 1.7.4.4
> > 
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [PATCH 7/7] urcu, call_rcu: Array initialized before pointer is planted

[Mathieu Desnoyers]

* Mathieu Desnoyers (mathieu.desnoyers at efficios.com) wrote:
> * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > On Wed, Sep 28, 2011 at 04:34:33PM +0800, Lai Jiangshan wrote:
> > > Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
> > 
> > This one I don't understand.  It looks to me that all dereferences
> > of the per_cpu_call_rcu_data pointer are done while holding the
> > call_rcu_mutex, so there should be no need for the memory barrier.
> > 
> > Yes, there is an access to the pointer without lock protection
> > in create_all_cpu_call_rcu_data(), but that access does not
> > do a dereference, so still no need for the memory barrier.
> > 
> > So, what am I missing?
> 
> A concurrent get_cpu_call_rcu_data(), called by get_call_rcu_data(),
> could dereference this pointer without holding any mutex. So this
> situation would happen if we have a concurrent call_rcu() executing
> while we do the create_all_cpu_call_rcu_data().
> 
> I think we would need to put a rcu_dereference() around
> per_cpu_call_rcu_data read within get_cpu_call_rcu_data() too.
> per_cpu_call_rcu_data should be done with rcu_set_pointer.
> 
> Also, a rcu read-side critical section would be required around any
> usage of per_cpu_call_rcu_data, and the action of tearing down the
> per-cpu data would require to wait for a quiescent state. So we would
> basically require that the call_rcu users need to be registered as
> RCU reader threads.
> 
> Or we clearly state that no call_rcu should happen while
> create_all_cpu_call_rcu_data is executed. But given we allow call_rcu to
> be called within call_rcu callbacks, this seems to be hard to guarantee.
> 
> Thoughts ?

Implemented in this commit:

commit 618b25958fec4d76310f0d9c59e42128e73a8719
Author: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
Date:   Thu Sep 29 17:13:48 2011 -0400

    urcu call_rcu: Use RCU read-side protection for per-cpu call_rcu data
    
    A concurrent get_cpu_call_rcu_data(), called by get_call_rcu_data(),
    could dereference this pointer without holding any mutex. So this
    situation would happen if we have a concurrent call_rcu() executing
    while we do the create_all_cpu_call_rcu_data().
    
    I think we would need to put a rcu_dereference() around
    per_cpu_call_rcu_data read within get_cpu_call_rcu_data() too.
    per_cpu_call_rcu_data should be done with rcu_set_pointer.
    
    Also, a rcu read-side critical section would be required around any
    usage of per_cpu_call_rcu_data, and the action of tearing down the
    per-cpu data would require to wait for a quiescent state. So we would
    basically require that the call_rcu users need to be registered as
    RCU reader threads.
    
    Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>

diff --git a/urcu-bp.c b/urcu-bp.c
index 2ae3408..4c0ab54 100644
--- a/urcu-bp.c
+++ b/urcu-bp.c
@@ -39,6 +39,7 @@
 #include "urcu/wfqueue.h"
 #include "urcu/map/urcu-bp.h"
 #include "urcu/static/urcu-bp.h"
+#include "urcu-pointer.h"
 
 /* Do not #define _LGPL_SOURCE to ensure we can emit the wrapper symbols */
 #undef _LGPL_SOURCE
diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
index 0a47d96..d09adb1 100644
--- a/urcu-call-rcu-impl.h
+++ b/urcu-call-rcu-impl.h
@@ -82,7 +82,10 @@ static struct call_rcu_data *default_call_rcu_data;
 
 /*
  * Pointer to array of pointers to per-CPU call_rcu_data structures
- * and # CPUs.
+ * and # CPUs. per_cpu_call_rcu_data is a RCU-protected pointer to an
+ * array of RCU-protected pointers to call_rcu_data. call_rcu acts as a
+ * RCU read-side and reads per_cpu_call_rcu_data and the per-cpu pointer
+ * without mutex. The call_rcu_mutex protects updates.
  */
 
 static struct call_rcu_data **per_cpu_call_rcu_data;
@@ -109,7 +112,7 @@ static void alloc_cpu_call_rcu_data(void)
 	p = malloc(maxcpus * sizeof(*per_cpu_call_rcu_data));
 	if (p != NULL) {
 		memset(p, '\0', maxcpus * sizeof(*per_cpu_call_rcu_data));
-		per_cpu_call_rcu_data = p;
+		rcu_set_pointer(&per_cpu_call_rcu_data, p);
 	} else {
 		if (!warned) {
 			fprintf(stderr, "[error] liburcu: unable to allocate per-CPU pointer array\n");
@@ -330,13 +333,18 @@ static void call_rcu_data_init(struct call_rcu_data **crdpp,
  * CPU, returning NULL if there is none.  We cannot automatically
  * created it because the platform we are running on might not define
  * sched_getcpu().
+ *
+ * The call to this function and use of the returned call_rcu_data
+ * should be protected by RCU read-side lock.
  */
 
 struct call_rcu_data *get_cpu_call_rcu_data(int cpu)
 {
 	static int warned = 0;
+	struct call_rcu_data **pcpu_crdp;
 
-	if (per_cpu_call_rcu_data == NULL)
+	pcpu_crdp = rcu_dereference(per_cpu_call_rcu_data);
+	if (pcpu_crdp == NULL)
 		return NULL;
 	if (!warned && maxcpus > 0 && (cpu < 0 || maxcpus <= cpu)) {
 		fprintf(stderr, "[error] liburcu: get CPU # out of range\n");
@@ -344,7 +352,7 @@ struct call_rcu_data *get_cpu_call_rcu_data(int cpu)
 	}
 	if (cpu < 0 || maxcpus <= cpu)
 		return NULL;
-	return per_cpu_call_rcu_data[cpu];
+	return rcu_dereference(pcpu_crdp[cpu]);
 }
 
 /*
@@ -418,7 +426,7 @@ int set_cpu_call_rcu_data(int cpu, struct call_rcu_data *crdp)
 		return -EEXIST;
 	}
 
-	per_cpu_call_rcu_data[cpu] = crdp;
+	rcu_set_pointer(&per_cpu_call_rcu_data[cpu], crdp);
 	call_rcu_unlock(&call_rcu_mutex);
 	return 0;
 }
@@ -450,6 +458,9 @@ struct call_rcu_data *get_default_call_rcu_data(void)
  * structure assigned to the CPU on which the thread is running,
  * followed by the default call_rcu_data structure.  If there is not
  * yet a default call_rcu_data structure, one will be created.
+ *
+ * Calls to this function and use of the returned call_rcu_data should
+ * be protected by RCU read-side lock.
  */
 struct call_rcu_data *get_call_rcu_data(void)
 {
@@ -564,6 +575,8 @@ static void wake_call_rcu_thread(struct call_rcu_data *crdp)
  * need the first invocation of call_rcu() to be fast, make sure
  * to create a call_rcu thread first.  One way to accomplish this is
  * "get_call_rcu_data();", and another is create_all_cpu_call_rcu_data().
+ *
+ * call_rcu must be called by registered RCU read-side threads.
  */
 
 void call_rcu(struct rcu_head *head,
@@ -573,10 +586,13 @@ void call_rcu(struct rcu_head *head,
 
 	cds_wfq_node_init(&head->next);
 	head->func = func;
+	/* Holding rcu read-side lock across use of per-cpu crdp */
+	rcu_read_lock();
 	crdp = get_call_rcu_data();
 	cds_wfq_enqueue(&crdp->cbs, &head->next);
 	uatomic_inc(&crdp->qlen);
 	wake_call_rcu_thread(crdp);
+	rcu_read_unlock();
 }
 
 /*
@@ -641,17 +657,37 @@ void call_rcu_data_free(struct call_rcu_data *crdp)
 void free_all_cpu_call_rcu_data(void)
 {
 	int cpu;
-	struct call_rcu_data *crdp;
+	struct call_rcu_data **crdp;
+	static int warned = 0;
 
 	if (maxcpus <= 0)
 		return;
+
+	crdp = malloc(sizeof(*crdp) * maxcpus);
+	if (!crdp) {
+		if (!warned) {
+			fprintf(stderr, "[error] liburcu: unable to allocate per-CPU pointer array\n");
+		}
+		warned = 1;
+	}
+
 	for (cpu = 0; cpu < maxcpus; cpu++) {
-		crdp = get_cpu_call_rcu_data(cpu);
-		if (crdp == NULL)
+		crdp[cpu] = get_cpu_call_rcu_data(cpu);
+		if (crdp[cpu] == NULL)
 			continue;
 		set_cpu_call_rcu_data(cpu, NULL);
-		call_rcu_data_free(crdp);
 	}
+	/*
+	 * Wait for call_rcu sites acting as RCU readers of the
+	 * call_rcu_data to become quiescent.
+	 */
+	synchronize_rcu();
+	for (cpu = 0; cpu < maxcpus; cpu++) {
+		if (crdp[cpu] == NULL)
+			continue;
+		call_rcu_data_free(crdp[cpu]);
+	}
+	free(crdp);
 }
 
 /*
@@ -700,7 +736,7 @@ void call_rcu_after_fork_child(void)
 	/* Cleanup call_rcu_data pointers before use */
 	maxcpus_reset();
 	free(per_cpu_call_rcu_data);
-	per_cpu_call_rcu_data = NULL;
+	rcu_set_pointer(&per_cpu_call_rcu_data, NULL);
 	thread_call_rcu_data = NULL;
 
 	/* Dispose of all of the rest of the call_rcu_data structures. */
diff --git a/urcu-call-rcu.h b/urcu-call-rcu.h
index e76a844..5ea0c23 100644
--- a/urcu-call-rcu.h
+++ b/urcu-call-rcu.h
@@ -62,16 +62,28 @@ struct rcu_head {
 /*
  * Exported functions
  */
+
+/*
+ * get_cpu_call_rcu_data should be called with RCU read-side lock held.
+ * Callers should be registered RCU read-side threads.
+ */
 struct call_rcu_data *get_cpu_call_rcu_data(int cpu);
 pthread_t get_call_rcu_thread(struct call_rcu_data *crdp);
 struct call_rcu_data *create_call_rcu_data(unsigned long flags,
 					   int cpu_affinity);
 int set_cpu_call_rcu_data(int cpu, struct call_rcu_data *crdp);
 struct call_rcu_data *get_default_call_rcu_data(void);
+/*
+ * get_call_rcu_data should be called from registered RCU read-side
+ * threads.
+ */
 struct call_rcu_data *get_call_rcu_data(void);
 struct call_rcu_data *get_thread_call_rcu_data(void);
 void set_thread_call_rcu_data(struct call_rcu_data *crdp);
 int create_all_cpu_call_rcu_data(unsigned long flags);
+/*
+ * call_rcu should be called from registered RCU read-side threads.
+ */
 void call_rcu(struct rcu_head *head,
 	      void (*func)(struct rcu_head *head));
 void call_rcu_data_free(struct call_rcu_data *crdp);
diff --git a/urcu-qsbr.c b/urcu-qsbr.c
index 1adaa94..a59a87a 100644
--- a/urcu-qsbr.c
+++ b/urcu-qsbr.c
@@ -39,6 +39,7 @@
 #include "urcu/map/urcu-qsbr.h"
 #define BUILD_QSBR_LIB
 #include "urcu/static/urcu-qsbr.h"
+#include "urcu-pointer.h"
 
 /* Do not #define _LGPL_SOURCE to ensure we can emit the wrapper symbols */
 #undef _LGPL_SOURCE
diff --git a/urcu.c b/urcu.c
index 20bbf36..77f6888 100644
--- a/urcu.c
+++ b/urcu.c
@@ -39,6 +39,7 @@
 #include "urcu/wfqueue.h"
 #include "urcu/map/urcu.h"
 #include "urcu/static/urcu.h"
+#include "urcu-pointer.h"
 
 /* Do not #define _LGPL_SOURCE to ensure we can emit the wrapper symbols */
 #undef _LGPL_SOURCE

> 
> Mathieu
> 
> > 
> > 							Thanx, Paul
> > 
> > > ---
> > >  urcu-call-rcu-impl.h |    4 ++++
> > >  1 files changed, 4 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
> > > index 3c68ae7..462139a 100644
> > > --- a/urcu-call-rcu-impl.h
> > > +++ b/urcu-call-rcu-impl.h
> > > @@ -104,6 +104,10 @@ static void alloc_cpu_call_rcu_data(void)
> > >  	p = malloc(maxcpus * sizeof(*per_cpu_call_rcu_data));
> > >  	if (p != NULL) {
> > >  		memset(p, '\0', maxcpus * sizeof(*per_cpu_call_rcu_data));
> > > +
> > > +		/* Array initialized before pointer is planted. */
> > > +		cmm_smp_mb();
> > > +
> > >  		per_cpu_call_rcu_data = p;
> > >  	} else {
> > >  		if (!warned) {
> > > -- 
> > > 1.7.4.4
> > > 
> > 
> 
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com