[ofw] [PATCH v2] 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, 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;
 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: cl_timer2
Type: application/octet-stream
Size: 5030 bytes
Desc: cl_timer2
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20100628/c8487182/attachment.obj>