From: Paolo Bonzini <bonzini@gnu.org>
Date: Tue, 08 Jun 2010 16:37:06 GMT
Subject: Re: [RFT PATCH 2/2] win32: optimize pthread_cond_broadcast
Message-ID: <4C0E71B2.1060904@gnu.org>
In-Reply-To: <4C0E7015.8030504@viscovery.net>

On 06/08/2010 06:30 PM, Johannes Sixt wrote:
> Am 07.06.2010 15:38, schrieb Paolo Bonzini:
>> @@ -172,9 +172,10 @@ int pthread_cond_broadcast(pthread_cond_t *cond)
>> * 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)) {
>> + if (cond->waiters> 0) {
>> BOOLEAN result;
>> + cond->was_broadcast = cond->waiters> 1;
>> +
>
> It is possible that you set was_broadcast to 1 here, while another
> thread still sees was_broadcast == 0 in cond_wait.

That still cannot happen, because pthread_cond_wait will be locked on 
the semaphore until the ReleaseSemaphore.  The only race that exists is 
between broadcast/signal's ReleaseSemaphore and wait's 
WaitForSingleObject.  This is benign, and exists before my patch.  But 
in all cases the code before ReleaseSemaphore is serialized WRT to the 
code after wait's WaitForSingleObject.

> That said, as long as this series buys performance only at the expense
> of clarity, I'm rather opposed to it because we do not call cond_wait
> and cond_broadcast in time-critical paths.

Yes, it is less clear indeed.  I tried to compensate with comments but 
that was not enough apparently.  As I said I did this patch for another 
project where condvars are used in time-critical paths; if you do not 
want to keep it, that's not a problem.

Paolo