Re: [RFT PATCH 1/2] win32: optimize condition variable implementation

[Johannes Sixt <j.sixt@viscovery.net>]

Am 07.06.2010 15:38, schrieb Paolo Bonzini:
>   int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)
>   {
> -	int last_waiter;
> +	int num_waiters;
>
> -	EnterCriticalSection(&cond->waiters_lock);
> +	/*
> +	 * This access is protected under the mutex.
> +	 */
>   	cond->waiters++;
> -	LeaveCriticalSection(&cond->waiters_lock);
>
>   	/*
>   	 * Unlock external mutex and wait for signal.
> @@ -105,17 +104,17 @@ int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)
>   	WaitForSingleObject(cond->sema, INFINITE);
>
>   	/*
> -	 * Decrease waiters count. If we are the last waiter, then we must
> +	 * Decrease waiters count.  The mutex prevents concurrent increments,
> +	 * so doing this decrement atomically is enough.
> +	 */
> +	num_waiters = InterlockedDecrement(&cond->waiters);
> +
> +	/* If we are the last waiter, then we must
>   	 * notify the broadcasting thread that it can continue.
>   	 * But if we continued due to cond_signal, we do not have to do that
>   	 * because the signaling thread knows that only one waiter continued.
>   	 */
> -	EnterCriticalSection(&cond->waiters_lock);
> -	cond->waiters--;
> -	last_waiter = cond->was_broadcast&&  cond->waiters == 0;
> -	LeaveCriticalSection(&cond->waiters_lock);
> -
> -	if (last_waiter) {
> +	if (num_waiters == 0&&  cond->was_broadcast) {
>   		/*
>   		 * cond_broadcast was issued while mutex was held. This means
>   		 * that all other waiters have continued, but are contending

This is not correct. While it is not possible that two threads increment 
waiters at the same time due to the external mutex, it is still possible 
that on thread increments, and a different one decrements. You lost all 
provisions to avoid that.

Furthermore, waiters_lock not only protects waiters, but also the combined 
state of waiters and was_broadcast. You break this protection. See also here:

> @@ -168,12 +168,18 @@ int pthread_cond_signal(pthread_cond_t *cond)
>    */
>   int pthread_cond_broadcast(pthread_cond_t *cond)
>   {
> -	EnterCriticalSection(&cond->waiters_lock);
> +	/*
> +	 * As in pthread_cond_signal, access to cond->waiters and
> +	 * cond->was_broadcast is locked via the external mutex.
> +	 */
>
>   	if ((cond->was_broadcast = cond->waiters>  0)) {
> +		BOOLEAN result;
>   		/* wake up all waiters */
> -		ReleaseSemaphore(cond->sema, cond->waiters, NULL);
> -		LeaveCriticalSection(&cond->waiters_lock);
> +		result = ReleaseSemaphore(cond->sema, cond->waiters, NULL);
> +		if (!result)
> +			return err_win_to_posix(GetLastError());
> +
>   		/*
>   		 * At this point all waiters continue. Each one takes its
>   		 * slice of the semaphor. Now it's our turn to wait: Since

-- Hannes