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

From: Dmitry Potapov <dpotapov@gmail.com>
Date: Jan 8, 2010, 03:32 UTC
Message-ID: <20100108033232.GA28263@dpotapov.dyndns.org>
In-Reply-To: <44c7183e43089c64fb65bd248f7fa5b9731067ea.1262895936.git.j6t@kdbg.org>

On Thu, Jan 07, 2010 at 10:54:57PM +0100, Johannes Sixt wrote:

Show 6 quoted lines

> +
> +int pthread_cond_init(pthread_cond_t *cond, const void *unused)
> +{
> +	cond->waiters = 0;
> +
> +	InitializeCriticalSection(&cond->waiters_lock);

Is waiters_lock really necessary?

Show 7 quoted lines

> +
> +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);

Show 16 quoted lines

> +
> +	/*
> +	 * 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);

Show 15 quoted lines

> +
> +	/* 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