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

[Tzachi Dar]

OK, a very nice trick to use a periodic timer.

I also believe that stop can be called from a callback since the criticalsections are re-entrant.
Indeed in that case you will not know if the callback is running until one will call cl_timer_destroy.

Thanks
Tzachi

> -----Original Message-----
> From: ofw-bounces at lists.openfabrics.org [mailto:ofw-
> bounces at lists.openfabrics.org] On Behalf Of Hefty, Sean
> Sent: Tuesday, June 29, 2010 12:51 AM
> To: ofw at lists.openfabrics.org
> Subject: [ofw] [PATCH v2] 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, and
> will block until any callback has completed.
> 
> cl_timer_start simply calls cl_timer_trim
> 
> cl_timer_trim changes the timeout of a queued timer if the new timeout
> time is less than the last set timeout.
> 
> Signed-off-by: Sean Hefty <sean.hefty at intel.com>
> ---
> Changes from v1:
> Eliminated TimerQueue.  The default one is used.
> Replaced the one-shot timer with a periodic timer.  This lets us use
> the ChangeTimerQueueTimer (who names these things, really) function to
> adjust the next timeout.
> Moved the calculation of the timeout time to inside the critical
> section to improve its accuracy in case an attempt to acquire the
> critical section blocks.
> 
>  trunk/core/complib/user/cl_timer.c    |   83 ++++++++++++++++++-------
> --------
>  trunk/inc/user/complib/cl_timer_osd.h |    5 +-
>  2 files changed, 47 insertions(+), 41 deletions(-)
> 
> diff --git a/trunk/core/complib/user/cl_timer.c
> b/trunk/core/complib/user/cl_timer.c
> index 4f2ce0f..5cab378 100644
> --- a/trunk/core/complib/user/cl_timer.c
> +++ b/trunk/core/complib/user/cl_timer.c
> @@ -32,23 +32,31 @@
> 
> 
>  #include "complib/cl_timer.h"
> +
> 
> +#define CL_MAX_TIME 0xFFFFFFFF
> 
> +
> +/*
> + * If timeout_time is 0, the timer has been stopped.
> + */
>  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 );
> +	uint64_t timeout;
>  	UNUSED_PARAM( timer_signalled );
> 
> +	EnterCriticalSection( &p_timer->cb_lock );
> +	EnterCriticalSection( &p_timer->lock );
> +	timeout = p_timer->timeout_time;
>  	p_timer->timeout_time = 0;
> -	p_timer->thread_id = GetCurrentThreadId();
> +	LeaveCriticalSection( &p_timer->lock );
> 
> -	(p_timer->pfn_callback)( (void*)p_timer->context );
> -
> -	p_timer->thread_id = 0;
> +	if( timeout )
> +		(p_timer->pfn_callback)( (void*)p_timer->context );
> +	LeaveCriticalSection( &p_timer->cb_lock );
>  }
> 
> 
> @@ -56,9 +64,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 +82,11 @@ cl_timer_init(
> 
>  	p_timer->pfn_callback = pfn_callback;
>  	p_timer->context = context;
> +	InitializeCriticalSection( &p_timer->lock );
> +	InitializeCriticalSection( &p_timer->cb_lock );
> +	if( !CreateTimerQueueTimer( &p_timer->h_timer, NULL,
> __timer_callback,
> +		p_timer, CL_MAX_TIME, CL_MAX_TIME, WT_EXECUTEINIOTHREAD ) )
> +		return CL_ERROR;
>  	return( CL_SUCCESS );
>  }
> 
> @@ -86,7 +97,9 @@ cl_timer_destroy(
>  {
>  	CL_ASSERT( p_timer );
> 
> -	cl_timer_stop( p_timer );
> +	DeleteTimerQueueTimer( NULL, p_timer->h_timer,
> INVALID_HANDLE_VALUE );
> +	DeleteCriticalSection( &p_timer->lock );
> +	DeleteCriticalSection( &p_timer->cb_lock );
>  }
> 
> 
> @@ -95,19 +108,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,35 +117,41 @@ 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 );
> -
> -	return cl_timer_start( p_timer, time_ms );
> +	EnterCriticalSection( &p_timer->lock );
> +	timeout = cl_get_time_stamp() + (((uint64_t)time_ms) * 1000);
> +	if ( !p_timer->timeout_time || timeout < p_timer->timeout_time )
> +	{
> +		if( ChangeTimerQueueTimer( NULL, p_timer->h_timer, time_ms,
> CL_MAX_TIME ) )
> +			p_timer->timeout_time = timeout;
> +		else
> +			status = CL_ERROR;
> +	}
> +	LeaveCriticalSection( &p_timer->lock );
> +	return status;
>  }
> 
> 
> +/*
> + * Acquire cb_lock to ensure that all callbacks have completed.
> + */
>  void
>  cl_timer_stop(
>  	IN	cl_timer_t* const	p_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;
> -	}
> +	EnterCriticalSection( &p_timer->cb_lock );
> +	EnterCriticalSection( &p_timer->lock );
>  	p_timer->timeout_time = 0;
> +	ChangeTimerQueueTimer( NULL, p_timer->h_timer, CL_MAX_TIME,
> CL_MAX_TIME );
> +	LeaveCriticalSection( &p_timer->lock );
> +	LeaveCriticalSection( &p_timer->cb_lock );
>  }
> 
> 
> diff --git a/trunk/inc/user/complib/cl_timer_osd.h
> b/trunk/inc/user/complib/cl_timer_osd.h
> index 0a023ba..a4a1195 100644
> --- a/trunk/inc/user/complib/cl_timer_osd.h
> +++ b/trunk/inc/user/complib/cl_timer_osd.h
> @@ -38,15 +38,14 @@
> 
>  #include "cl_types.h"
> 
> -
> -/* Timer object definition. */
>  typedef struct _cl_timer
>  {
>  	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;
>