[ofw] patch: Fix a race in the cl_timer code that caused deadlocks in opensm
Tzachi Dar
tzachid at mellanox.co.il
Tue Jun 22 00:50:56 PDT 2010
While debugging a deadlock in the opensm executable, we have found that there is a problem in the cl_timer code:
The problem starts from the fact that there can be two call backs that are running simultaneously.
On each call back that runs there is the following code:
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();
(p_timer->pfn_callback)( (void*)p_timer->context );
p_timer->thread_id = 0;
}
This should promise that if a callback is running, p_timer->thread_id will have the thread_id of the running thread.
This field is later used when cl_timer_stop() is being called in order to prevent deadlocks. (please note that cl_timer_stop() is being called from cl_timer_start(). Please also note that the deadlock happens if the caller of cl_stop() is actually executing in the context of the call back itself).
The problem happens when two callbacks will be running and than the p_timer->thread_id doesn't show the correct field.
The fix itself is to sync the start and stop calls and now only one callback will be running (actually, there is one exception to this code: if from the callback, a new timer is started, the new callback might be running on the same time, but: (1) stop has already being called. (2) the p_timer->thread_id is not being touched after the callback.
Thanks
Tzachi
Index: B:/users/tzachid/MLNX_WinOF-2_1_1/MLNX_WinOF-2_1_1/core/complib/user/cl_timer.c
===================================================================
--- B:/users/tzachid/MLNX_WinOF-2_1_1/MLNX_WinOF-2_1_1/core/complib/user/cl_timer.c (revision 5977)
+++ B:/users/tzachid/MLNX_WinOF-2_1_1/MLNX_WinOF-2_1_1/core/complib/user/cl_timer.c (revision 5978)
@@ -33,25 +33,27 @@
#include "complib/cl_timer.h"
-
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. */
+
+ DWORD thread_id = GetCurrentThreadId();
CL_ASSERT( timer_signalled );
+
UNUSED_PARAM( timer_signalled );
+ CL_ASSERT(thread_id!=0);
+
p_timer->timeout_time = 0;
- p_timer->thread_id = GetCurrentThreadId();
+ p_timer->thread_id = thread_id;
(p_timer->pfn_callback)( (void*)p_timer->context );
- p_timer->thread_id = 0;
}
-
void
cl_timer_construct(
IN cl_timer_t* const p_timer )
@@ -76,7 +78,8 @@
p_timer->pfn_callback = pfn_callback;
p_timer->context = context;
- return( CL_SUCCESS );
+ return cl_spinlock_init(&p_timer->spinlock);
+
}
@@ -97,6 +100,8 @@
{
CL_ASSERT( p_timer );
+ cl_spinlock_acquire(&p_timer->spinlock);
+
cl_timer_stop( p_timer );
p_timer->timeout_time = cl_get_time_stamp() + (((uint64_t)time_ms) * 1000);
@@ -104,9 +109,12 @@
if( !CreateTimerQueueTimer( &p_timer->h_timer, NULL, __timer_callback,
p_timer, time_ms, 0, WT_EXECUTEINIOTHREAD ) )
{
+
+ cl_spinlock_release(&p_timer->spinlock);
return( CL_ERROR );
}
+ cl_spinlock_release(&p_timer->spinlock);
return( CL_SUCCESS );
}
@@ -117,26 +125,34 @@
IN const uint32_t time_ms )
{
uint64_t timeout_time;
+ cl_status_t status;
CL_ASSERT( p_timer );
CL_ASSERT( p_timer->pfn_callback );
+ cl_spinlock_acquire(&p_timer->spinlock);
+
/* 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 )
+ if( p_timer->timeout_time && p_timer->timeout_time < timeout_time ) {
+ cl_spinlock_release(&p_timer->spinlock);
return( CL_SUCCESS );
+ }
- return cl_timer_start( p_timer, time_ms );
+ status = cl_timer_start( p_timer, time_ms );
+
+ cl_spinlock_release(&p_timer->spinlock);
+ return status;
}
-
void
cl_timer_stop(
IN cl_timer_t* const p_timer )
{
CL_ASSERT( p_timer );
+ cl_spinlock_acquire(&p_timer->spinlock);
if( p_timer->h_timer && p_timer->thread_id != GetCurrentThreadId() )
{
@@ -145,6 +161,8 @@
p_timer->h_timer = NULL;
}
p_timer->timeout_time = 0;
+
+ cl_spinlock_release(&p_timer->spinlock);
}
Index: B:/users/tzachid/MLNX_WinOF-2_1_1/MLNX_WinOF-2_1_1/inc/complib/cl_timer.h
===================================================================
--- B:/users/tzachid/MLNX_WinOF-2_1_1/MLNX_WinOF-2_1_1/inc/complib/cl_timer.h (revision 5977)
+++ B:/users/tzachid/MLNX_WinOF-2_1_1/MLNX_WinOF-2_1_1/inc/complib/cl_timer.h (revision 5978)
@@ -45,8 +45,8 @@
#include <complib/cl_types.h>
+#include <complib/cl_spinlock.h>
-
/****h* Component Library/Timer
* NAME
* Timer
Index: B:/users/tzachid/MLNX_WinOF-2_1_1/MLNX_WinOF-2_1_1/inc/user/complib/cl_timer_osd.h
===================================================================
--- B:/users/tzachid/MLNX_WinOF-2_1_1/MLNX_WinOF-2_1_1/inc/user/complib/cl_timer_osd.h (revision 5977)
+++ B:/users/tzachid/MLNX_WinOF-2_1_1/MLNX_WinOF-2_1_1/inc/user/complib/cl_timer_osd.h (revision 5978)
@@ -47,8 +47,9 @@
const void *context;
uint64_t timeout_time;
DWORD thread_id;
+ cl_spinlock_t spinlock;
} cl_timer_t;
-#endif // _CL_TIMER_OSD_H_
\ No newline at end of file
+#endif // _CL_TIMER_OSD_H_
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20100622/c78bc9ff/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: timer.patch
Type: application/octet-stream
Size: 3964 bytes
Desc: timer.patch
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20100622/c78bc9ff/attachment.obj>
More information about the ofw
mailing list