[ofw] [PATCH] complib/user: fix timer race conditions

[Fab Tillier]

Why the per-timer timer queue?  The only time you take advantage of it is in cl_timer_destroy, but at that point the timer should no longer be in the process of being queued.  You could just as easily call __clear_timer_handle, and then do a blocking cancel of the timer.

-Fab

> -----Original Message-----
> From: Hefty, Sean [mailto:sean.hefty at intel.com]
> Sent: Thursday, June 24, 2010 5:56 PM
> To: Hefty, Sean; Smith, Stan; Fab Tillier; Tzachi Dar;
> ofw at lists.openfabrics.org
> Cc: Uri Habusha; Yevgeny Kliteynik
> Subject: [PATCH] complib/user: fix timer race conditions
> 
> This serializes the callbacks for a timer and provides synchronization
> for
> multi-threaded timer usage.
> 
> cl_timer_stop is not allowed to be called from a timer callback.
> 
> cl_timer_start simply calls cl_timer_trim
> 
> cl_timer_trim cancels any queued timer and inserts a new one if the
> timeout
> time is less than the last last set timeout.
> 
> This eliminates the need to track the callback thread ID and any races
> associated
> with trying to do so.  The synchronization provided is straightforward.
> 
> Signed-off-by: Sean Hefty <sean.hefty at intel.com>
> ---
>  trunk/core/complib/user/cl_timer.c    |   95 ++++++++++++++++++++-----
> --------
>  trunk/inc/user/complib/cl_timer_osd.h |    4 +
>  2 files changed, 60 insertions(+), 39 deletions(-)
> 
> diff --git a/trunk/core/complib/user/cl_timer.c
> b/trunk/core/complib/user/cl_timer.c
> index 4f2ce0f..37c5448 100644
> --- a/trunk/core/complib/user/cl_timer.c
> +++ b/trunk/core/complib/user/cl_timer.c
> @@ -34,21 +34,33 @@
>  #include "complib/cl_timer.h"
> 
> 
> +static HANDLE
> +__clear_timer_handle(
> +	IN	cl_timer_t* const p_timer )
> +{
> +	HANDLE timer;
> +
> +	EnterCriticalSection( &p_timer->lock );
> +	timer = p_timer->h_timer;
> +	p_timer->h_timer = NULL;
> +	LeaveCriticalSection( &p_timer->lock );
> +
> +	return timer;
> +}
> +
>  static void CALLBACK
>  __timer_callback(
>  	IN cl_timer_t* const p_timer,
>  	IN BOOLEAN timer_signalled )
>  {
> -	/* timer_signalled is always TRUE, and has no value. */
> -	CL_ASSERT( timer_signalled );
>  	UNUSED_PARAM( timer_signalled );
> 
> -	p_timer->timeout_time = 0;
> -	p_timer->thread_id = GetCurrentThreadId();
> +	if( !__clear_timer_handle( p_timer ))
> +		return;
> 
> +	EnterCriticalSection( &p_timer->cb_lock );
>  	(p_timer->pfn_callback)( (void*)p_timer->context );
> -
> -	p_timer->thread_id = 0;
> +	LeaveCriticalSection( &p_timer->cb_lock );
>  }
> 
> 
> @@ -56,9 +68,7 @@ void
>  cl_timer_construct(
>  	IN	cl_timer_t* const	p_timer )
>  {
> -	p_timer->h_timer = NULL;
> -	p_timer->timeout_time = 0;
> -	p_timer->thread_id = 0;
> +	memset(p_timer, 0, sizeof *p_timer);
>  }
> 
> 
> @@ -76,6 +86,12 @@ cl_timer_init(
> 
>  	p_timer->pfn_callback = pfn_callback;
>  	p_timer->context = context;
> +	p_timer->h_timer_queue = CreateTimerQueue();
> +	if( !p_timer->h_timer_queue )
> +		return CL_ERROR;
> +
> +	InitializeCriticalSection( &p_timer->lock );
> +	InitializeCriticalSection( &p_timer->cb_lock );
>  	return( CL_SUCCESS );
>  }
> 
> @@ -86,7 +102,12 @@ cl_timer_destroy(
>  {
>  	CL_ASSERT( p_timer );
> 
> -	cl_timer_stop( p_timer );
> +	if( p_timer->h_timer_queue )
> +	{
> +		DeleteTimerQueueEx( p_timer->h_timer_queue,
> INVALID_HANDLE_VALUE );
> +		DeleteCriticalSection( &p_timer->lock );
> +		DeleteCriticalSection( &p_timer->cb_lock );
> +	}
>  }
> 
> 
> @@ -95,19 +116,7 @@ cl_timer_start(
>  	IN	cl_timer_t* const	p_timer,
>  	IN	const uint32_t		time_ms )
>  {
> -	CL_ASSERT( p_timer );
> -
> -	cl_timer_stop( p_timer );
> -
> -	p_timer->timeout_time = cl_get_time_stamp() +
> (((uint64_t)time_ms) * 1000);
> -
> -	if( !CreateTimerQueueTimer( &p_timer->h_timer, NULL,
> __timer_callback,
> -		p_timer, time_ms, 0, WT_EXECUTEINIOTHREAD ) )
> -	{
> -		return( CL_ERROR );
> -	}
> -
> -	return( CL_SUCCESS );
> +	return cl_timer_trim( p_timer, time_ms );
>  }
> 
> 
> @@ -116,19 +125,30 @@ cl_timer_trim(
>  	IN	cl_timer_t* const	p_timer,
>  	IN	const uint32_t		time_ms )
>  {
> -	uint64_t		timeout_time;
> +	uint64_t	timeout;
> +	cl_status_t	status = CL_SUCCESS;
> 
>  	CL_ASSERT( p_timer );
>  	CL_ASSERT( p_timer->pfn_callback );
> 
> -	/* Calculate the timeout time in the timer object. */
> -	timeout_time = cl_get_time_stamp() + (((uint64_t)time_ms) *
> 1000);
> -
> -	/* Only pull in the timeout time. */
> -	if( p_timer->timeout_time && p_timer->timeout_time < timeout_time
> )
> -		return( CL_SUCCESS );
> +	timeout = cl_get_time_stamp() + (((uint64_t)time_ms) * 1000);
> 
> -	return cl_timer_start( p_timer, time_ms );
> +	EnterCriticalSection( &p_timer->lock );
> +	if ( !p_timer->h_timer || timeout < p_timer->timeout_time )
> +	{
> +		if( p_timer->h_timer )
> +			DeleteTimerQueueTimer( p_timer->h_timer_queue,
> p_timer->h_timer, NULL );
> +
> +		p_timer->timeout_time = timeout;
> +		if( !CreateTimerQueueTimer( &p_timer->h_timer, p_timer-
> >h_timer_queue,
> +			__timer_callback, p_timer, time_ms, 0,
> WT_EXECUTEINIOTHREAD ) )
> +		{
> +			p_timer->h_timer = NULL;
> +			status = CL_ERROR;
> +		}
> +	}
> +	LeaveCriticalSection( &p_timer->lock );
> +	return status;
>  }
> 
> 
> @@ -136,15 +156,14 @@ void
>  cl_timer_stop(
>  	IN	cl_timer_t* const	p_timer )
>  {
> +	HANDLE timer;
> +
>  	CL_ASSERT( p_timer );
> 
> -	if( p_timer->h_timer && p_timer->thread_id !=
> GetCurrentThreadId() )
> -	{
> -		/* Make sure we block until the timer is cancelled. */
> -		DeleteTimerQueueTimer( NULL, p_timer->h_timer,
> INVALID_HANDLE_VALUE );
> -		p_timer->h_timer = NULL;
> -	}
> -	p_timer->timeout_time = 0;
> +	timer = __clear_timer_handle( p_timer );
> +
> +	if( timer )
> +		DeleteTimerQueueTimer( p_timer->h_timer_queue, timer,
> INVALID_HANDLE_VALUE );
>  }
> 
> 
> diff --git a/trunk/inc/user/complib/cl_timer_osd.h
> b/trunk/inc/user/complib/cl_timer_osd.h
> index 0a023ba..370a175 100644
> --- a/trunk/inc/user/complib/cl_timer_osd.h
> +++ b/trunk/inc/user/complib/cl_timer_osd.h
> @@ -42,11 +42,13 @@
>  /* Timer object definition. */
>  typedef struct _cl_timer
>  {
> +	HANDLE					h_timer_queue;
>  	HANDLE					h_timer;
>  	cl_pfn_timer_callback_t	pfn_callback;
>  	const void				*context;
>  	uint64_t				timeout_time;
> -	DWORD					thread_id;
> +	CRITICAL_SECTION		lock;
> +	CRITICAL_SECTION		cb_lock;
> 
>  } cl_timer_t;
>