Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API

[Dmitry Potapov <dpotapov@gmail.com>]

On Thu, Jan 07, 2010 at 10:54:57PM +0100, Johannes Sixt wrote:
> +
> +int pthread_cond_init(pthread_cond_t *cond, const void *unused)
> +{
> +	cond->waiters = 0;
> +
> +	InitializeCriticalSection(&cond->waiters_lock);

Is waiters_lock really necessary?

> +
> +int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)
> +{
> +	/* serialize access to waiters count */
> +	EnterCriticalSection(&cond->waiters_lock);
> +	++cond->waiters;
> +	LeaveCriticalSection(&cond->waiters_lock);

InterlockedIncrement(&cond->waiters);

> +
> +	/*
> +	 * Unlock external mutex and wait for signal.
> +	 * NOTE: we've held mutex locked long enough to increment
> +	 * waiters count above, so there's no problem with
> +	 * leaving mutex unlocked before we wait on semaphore.
> +	 */
> +	LeaveCriticalSection(mutex);
> +
> +	/* let's wait - ignore return value */
> +	WaitForSingleObject(cond->sema, INFINITE);
> +
> +	/* we're done waiting, so make sure we decrease waiters count */
> +	EnterCriticalSection(&cond->waiters_lock);
> +	--cond->waiters;
> +	LeaveCriticalSection(&cond->waiters_lock);

InterlockedDecrement(&cond->waiters);

> +
> +	/* lock external mutex again */
> +	EnterCriticalSection(mutex);
> +
> +	return 0;
> +}
> +
> +int pthread_cond_signal(pthread_cond_t *cond)
> +{
> +	int have_waiters;
> +
> +	/* serialize access to waiters count */
> +	EnterCriticalSection(&cond->waiters_lock);
> +	have_waiters = cond->waiters > 0;
> +	LeaveCriticalSection(&cond->waiters_lock);

AFAIK, Win32 API assumes that reading LONG is always atomic, so
the critical section is not really necesary here, but you need
to declare 'waiters' as 'volatile':

> + */
> +typedef struct {
> +	LONG waiters;

	volatile LONG waiters;

> +	CRITICAL_SECTION waiters_lock;
> +	HANDLE sema;
> +} pthread_cond_t;
> +


Dmitry