From mboxrd@z Thu Jan  1 00:00:00 1970
From: mathieu.desnoyers@efficios.com (Mathieu Desnoyers)
Date: Thu, 29 Sep 2011 13:13:36 -0400
Subject: [ltt-dev] [PATCH 2/7] urcu,
	defer_rcu: fix missing respond to a	cancellation request
In-Reply-To: <01215f498cd1a5eb588ba2599ed2a80a739a7547.1317198301.git.laijs@cn.fujitsu.com>
References: <cover.1317198301.git.laijs@cn.fujitsu.com>
	<01215f498cd1a5eb588ba2599ed2a80a739a7547.1317198301.git.laijs@cn.fujitsu.com>
Message-ID: <20110929171336.GB6749@Krystal>

* 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