[LONG] Why the new Win32 pthread implementation didn't work.
Gildas Bazin
gbazin at netcourrier.com
Mon Jul 29 19:49:52 CEST 2002
Hi fellow coders,
I just though I would write a quick summary about where we are in our Win32
pthread implementation and why the new win32 pthread implementation which
has been introduced recently didn't work (just so we don't make the same
mistakes again and again).
Our implementation includes a separate code for Win9x and WinNT because NT
provides a crucial synchronisation function that Win9x unfortunately lacks.
WinNT implementation:
--------------------------
The important bits here are the vlc_cond_*() functions. Here is the new
implementation that has been introduced recently:
int vlc_cond_init( vlc_cond_t *p_condvar )
{
/* Initialize counter */
p_condvar->i_waiting_threads = 0;
/* Create an auto-reset event and a semaphore. */
p_condvar->signal = CreateEvent( NULL, FALSE, FALSE, NULL );
p_condvar->semaphore = CreateSemaphore( NULL, 0, 0x7fffffff, NULL );
p_condvar->b_broadcast = 0;
...
}
int vlc_cond_signal( vlc_cond_t *p_condvar )
{
if( p_condvar->i_waiting_threads )
{
ReleaseSemaphore( p_condvar->semaphore, 1, 0 );
}
...
}
int vlc_cond_broadcast( vlc_cond_t *p_condvar )
{
/* Release all waiting threads. */
if( p_condvar->i_waiting_threads )
{
p_condvar->b_broadcast = 1;
/* This call is atomic */
ReleaseSemaphore( p_condvar->semaphore,
p_condvar->i_waiting_threads, 0 );
/* Wait for all threads to get the semaphore */
WaitForSingleObject( p_condvar->signal, INFINITE );
p_condvar->b_broadcast = 0;
}
...
}
int _vlc_cond_wait( char * psz_file, int i_line,
vlc_cond_t *p_condvar,
vlc_mutex_t *p_mutex )
{
/* Increase our wait count */
p_condvar->i_waiting_threads++;
/* It is only possible to atomically release the mutex and initiate the
* waiting on WinNT/2K/XP. Win9x doesn't have SignalObjectAndWait(). */
p_main->p_sys->SignalObjectAndWait( p_mutex->mutex,
p_condvar->semaphore,
INFINITE, FALSE );
/* XXX: we should protect i_waiting_threads with a mutex, but
* is it really worth it ? */
p_condvar->i_waiting_threads--;
if( p_condvar->b_broadcast
&& p_condvar->i_waiting_threads == 0 )
{
p_main->p_sys->SignalObjectAndWait( p_condvar->signal,
p_mutex->mutex,
INFINITE, FALSE );
}
else
{
/* Just take back the lock */
WaitForSingleObject( p_mutex->mutex, INFINITE );
}
...
}
Problems with this implementation ?
----------------------------------------
Well, at least 2 things.
The most important one being that a race condition exists which allows a
thread that will start waiting on the condvar after it has been signaled to
steal this signal from another waiting thread.
The problem exists because the Win32 semaphore on which vlc_cond_wait() will
start waiting will stay signaled until all the waiting threads are
awakened. But at this time it is likely that the thread which has called
vlc_cond_signal() will have finished to excute this function some time ago
and thus won't hold the mutex associated with the condvar anymore, allowing
a new thread to start waiting on the condvar and steal the signal.
A simple solution would then be to ensure that all the waiting threads are
awake before we leave vlc_cond_signal().
We would then have:
int vlc_cond_signal( vlc_cond_t *p_condvar )
{
if( p_condvar->i_waiting_threads )
{
ReleaseSemaphore( p_condvar->semaphore, 1, 0 );
/* Wait for the thread to get the semaphore */
WaitForSingleObject( p_condvar->signal, INFINITE );
}
...
}
int _vlc_cond_wait( char * psz_file, int i_line,
vlc_cond_t *p_condvar,
vlc_mutex_t *p_mutex )
{
/* Increase our wait count */
p_condvar->i_waiting_threads++;
p_main->p_sys->SignalObjectAndWait( p_mutex->mutex,
p_condvar->semaphore,
INFINITE, FALSE );
p_condvar->i_waiting_threads--;
if( p_condvar->i_waiting_threads == 0 )
{
SetEvent( p_condvar->signal);
}
vlc_mutex_lock(p_mutex);
...
}
The second problem is the unneeded complexity (which also leads to slow
code) because Windows provides us with a really interesting function which
can wake a single thread at a time.
Solution:
----------
Quote from MSDN: For an auto-reset event object, PulseEvent() will reset the
state to nonsignaled and returns after releasing a single waiting thread,
even if multiple threads are waiting. If no threads are waiting, or if no
thread can be released immediately, PulseEvent() simply sets the event
object's state to nonsignaled and returns.
This turns our pthread implementation code into this (which by the way looks
awfully like our old pthread implementation ;-) :
int vlc_cond_init( vlc_cond_t *p_condvar )
{
/* Initialize counter */
p_condvar->i_waiting_threads = 0;
/* Create an auto-reset event. */
p_condvar->event = CreateEvent( NULL, /* no security */
FALSE, /* auto-reset event */
FALSE, /* start non-signaled */
NULL ); /* unnamed */
...
}
int vlc_cond_signal( vlc_cond_t *p_condvar )
{
/* Release one waiting thread if one is available. */
PulseEvent( p_condvar->event );
...
}
int vlc_cond_broadcast( vlc_cond_t *p_condvar )
{
/* Release all waiting threads. */
int i;
for( i = p_condvar->i_waiting_threads; i > 0; i-- )
PulseEvent( p_condvar->event );
}
int _vlc_cond_wait( char * psz_file, int i_line,
vlc_cond_t *p_condvar,
vlc_mutex_t *p_mutex )
{
/* Increase our wait count */
p_condvar->i_waiting_threads++;
SignalObjectAndWait( p_mutex->mutex,
p_condvar->event,
INFINITE, FALSE );
/* Reacquire the mutex before returning. */
vlc_mutex_lock( p_mutex );
...
}
Win9x Implementation:
--------------------------
I won't dive into the details of this implementation as it can be quite
complex (because of the lack of SignalObjectAndWait()). Look at the actual
source code if you want to know the gorry details.
What I can say is that the new implementation also had the same correctness
problem as the WinNT implementation described above (thread stealing the
signal).
I just commited a new implementation or to be correct I should say 3 new
implementations. They can be selected with the --win9x-cv-method=<integer>
config option. Here is a quick description of what they do:
0 - old implementation. Ii uses PulseEvent exactly as the WinNT
implementation. This implementation should be the quickest but is also
prone to race-conditions because of the lack of SignalObjectAndWait() .
(This race condition is possible even though I never managed to trigger it
except with the --vdec-smp option which is a really good torture test ;)
1 - implementation based on the first solution we introduced above for WinNT
(using a semaphore), except that it has been optimized so that we don't
spend our time waking up and going back to sleep in vlc_cond_signal and
vlc_cond_wait because of resource contentions, and we don't need to wait
anymore in vlc_cond_signal for all the threads to be awakened.
2 - implementation based on the first solution we introduced above for WinNT
(but non-optimized).
Of course this config option should be for developpers only and may be
removed if/when we discover which method works the best.
That's all folks, I hope you didn't fell asleep during this reading :)
--
Gildas
--
This is the vlc-devel mailing-list, see http://www.videolan.org/vlc/
To unsubscribe, please read http://www.videolan.org/lists.html
If you are in trouble, please contact <postmaster at videolan.org>
More information about the vlc-devel
mailing list