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
next prev parent 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).