git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Paolo Bonzini <bonzini@gnu.org>
To: Johannes Sixt <j.sixt@viscovery.net>
Cc: git@vger.kernel.org
Subject: Re: [RFT PATCH 2/2] win32: optimize pthread_cond_broadcast
Date: Tue, 08 Jun 2010 18:37:06 +0200	[thread overview]
Message-ID: <4C0E71B2.1060904@gnu.org> (raw)
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

  reply	other threads:[~2010-06-08 16:37 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-06-07 13:38 [RFT PATCH 0/2] win32: optimize emulation of condition variables Paolo Bonzini
2010-06-07 13:38 ` [RFT PATCH 1/2] win32: optimize condition variable implementation Paolo Bonzini
2010-06-08 16:16   ` Johannes Sixt
2010-06-08 16:27     ` Paolo Bonzini
2010-06-07 13:38 ` [RFT PATCH 2/2] win32: optimize pthread_cond_broadcast Paolo Bonzini
2010-06-08 16:30   ` Johannes Sixt
2010-06-08 16:37     ` Paolo Bonzini [this message]
2010-06-08 18:46       ` Johannes Sixt
2010-06-13 10:16 ` [PATCH 3/2] fix race in win32 pthread_cond_signal causing spurious wakeups Paolo Bonzini

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4C0E71B2.1060904@gnu.org \
    --to=bonzini@gnu.org \
    --cc=git@vger.kernel.org \
    --cc=j.sixt@viscovery.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).