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:[email protected]] > Sent: Thursday, June 24, 2010 5:56 PM > To: Hefty, Sean; Smith, Stan; Fab Tillier; Tzachi Dar; > [email protected] > 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 <[email protected]> > --- > 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; > _______________________________________________ ofw mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
