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

[Smith, Stan]

What degree of testing has been applied to this patch?

Hefty, Sean wrote:
> 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;