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

[Hefty, Sean]

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;
 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: user_timer
Type: application/octet-stream
Size: 5058 bytes
Desc: user_timer
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20100624/1fc350d8/attachment.obj>