[prev in list] [next in list] [prev in thread] [next in thread]
List: illumos-developer
Subject: Re: [developer] Question about kernel condition variables and appropriate behavior
From: "Paul Dagnelie" <pcd () delphix ! com>
Date: 2016-02-25 16:35:13
Message-ID: CAJ-UWOdPRU1CQBbwP7WcWYqdFWN3KOryP1U+eJK2=897yy5JAQ () mail ! gmail ! com
[Download RAW message or body]
Marcel,
You're right that the premature return clause makes it impossible for this
code to work correctly without either holding the mutex or doing
thread_join() on thread B. I had forgotten about that clause in the man
page (because frankly I think it's a horrible property for a
synchronization primitive to have). That does sort of render this whole
issue moot.
On Tue, Feb 9, 2016 at 5:20 PM, Marcel Telka <marcel@telka.sk> wrote:
> On Tue, Feb 09, 2016 at 04:38:40PM -0800, Paul Dagnelie wrote:
> > Jeff,
> >
> > cv_signal already modifies cv_waiters before waking threads. Modifying
> > cv_broadcast to do so would be simple from a code perspective, and have
> > only positive performance impacts from what I can tell (cv_signals that
> > occur during a cv_broadcast would not block on the sleepq lock until the
> > broadcast is done, since cv_waiters would be equal to zero). As I said
> in
> > my opening email, I haven't exhaustively analyzed the thread interactions
> > that could result from this change, but the fact that cv_waiters is only
> > accessed in a few places without the lock held makes it seem relatively
> > safe to me.
>
> Paul,
>
> Is the pseudocode below representing your use case? If so, then AFAIK
> there is
> no simple and safe way (at lines marked with the CHECK comment) how to make
> sure that cv_wait() returned because the cv_broadcast() was called.
>
> The condvar(9F) man page says this:
>
> NOTES
> It is possible for cv_wait(), cv_wait_sig(), cv_timedwait(),
> cv_timedwait_sig(), cv_reltimedwait(), and cv_reltimedwait_sig() to
> return prematurely, that is, not due to a call to cv_signal() or
> cv_broadcast(). This occurs most commonly in the case of
> cv_wait_sig()
> and cv_timedwait_sig() when the thread is stopped and restarted by
> job
> control signals or by a debugger, but can happen in other cases as
> well, even for cv_wait(). Code that calls these functions must
> always
> recheck the reason for blocking and call again if the reason for
> blocking is still true.
>
>
> IOW it means that cv_wait() can return anytime, even when the thread B is
> at
> line marked with the HERE comment below and you have no way how to make
> sure it
> happened (or didn't happened). The issig() call is not sufficient for that
> (why it should be sufficient?).
>
> To make sure the cv_destroy() is called _after_ the cv_broadcast() you
> need to
> hold the mutex during the cv_broadcast() call.
>
>
>
> The pseudocode
> --------------
>
> int flag = 0;
>
> thread_A()
> {
> mutex_enter(&mutex);
> while (flag == 0)
> cv_wait(&cv, &mutex);
> /* CHECK: make sure the cv_broadcast was called */
> mutex_exit(&mutex);
>
> /* CHECK: make sure the cv_broadcast was called */
> cv_destroy(&cv);
> }
>
> thread_B()
> {
> ASSERT(!MUTEX_HELD(&mutex));
> flag = 1;
>
> /* HERE */
>
> cv_broadcast(&cv);
> }
>
>
>
> --
> +-------------------------------------------+
> | Marcel Telka e-mail: marcel@telka.sk |
> | homepage: http://telka.sk/ |
> | jabber: marcel@jabber.sk |
> +-------------------------------------------+
>
--
Paul Dagnelie
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/25758058-4e9228dc
Modify Your Subscription: https://www.listbox.com/member/?member_id=25758058&id_secret=25758058-c19b436a
Powered by Listbox: http://www.listbox.com
[Attachment #3 (text/html)]
<html><html><div dir="ltr"><div>Marcel,<br /><br /></div>You're right that the \
premature return clause makes it impossible for this code to work correctly without \
either holding the mutex or doing thread_join() on thread B. I had forgotten \
about that clause in the man page (because frankly I think it's a horrible \
property for a synchronization primitive to have). That does sort of render \
this whole issue moot.<br /></div><div class="gmail_extra"><br /><div \
class="gmail_quote">On Tue, Feb 9, 2016 at 5:20 PM, Marcel Telka <span \
dir="ltr"><<a href="mailto:marcel@telka.sk" \
target="_blank">marcel@telka.sk</a>></span> wrote:<br /><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><span class="">On Tue, Feb 09, 2016 at 04:38:40PM -0800, Paul \
Dagnelie wrote:<br /> > Jeff,<br />
><br />
> cv_signal already modifies cv_waiters before waking threads. Modifying<br \
/> > cv_broadcast to do so would be simple from a code perspective, and have<br />
> only positive performance impacts from what I can tell (cv_signals that<br />
> occur during a cv_broadcast would not block on the sleepq lock until the<br />
> broadcast is done, since cv_waiters would be equal to zero). As I said \
in<br /> > my opening email, I haven't exhaustively analyzed the thread \
interactions<br /> > that could result from this change, but the fact that \
cv_waiters is only<br /> > accessed in a few places without the lock held makes it \
seem relatively<br /> > safe to me.<br /><br /></span>Paul,<br /><br />
Is the pseudocode below representing your use case? If so, then AFAIK there \
is<br /> no simple and safe way (at lines marked with the CHECK comment) how to \
make<br /> sure that cv_wait() returned because the cv_broadcast() was called.<br \
/><br /> The condvar(9F) man page says this:<br /><br />
NOTES<br />
It is possible for cv_wait(), cv_wait_sig(), \
cv_timedwait(),<br /> cv_timedwait_sig(), \
cv_reltimedwait(), and cv_reltimedwait_sig() to<br /> \
return prematurely, that is, not due to a call to cv_signal() or<br /> \
cv_broadcast(). This occurs most commonly in the case of \
cv_wait_sig()<br /> and cv_timedwait_sig() when the thread \
is stopped and restarted by job<br /> control signals or \
by a debugger, but can happen in other cases as<br /> \
well, even for cv_wait(). Code that calls these functions must always<br /> \
recheck the reason for blocking and call again if the \
reason for<br /> blocking is still true.<br /><br /><br />
IOW it means that cv_wait() can return anytime, even when the thread B is at<br />
line marked with the HERE comment below and you have no way how to make sure it<br />
happened (or didn't happened). The issig() call is not sufficient for \
that<br /> (why it should be sufficient?).<br /><br />
To make sure the cv_destroy() is called _after_ the cv_broadcast() you need to<br />
hold the mutex during the cv_broadcast() call.<br /><br /><br /><br />
The pseudocode<br />
--------------<br /><br />
int flag = 0;<br /><br />
thread_A()<br />
{<br />
mutex_enter(&mutex);<br />
while (flag == 0)<br />
cv_wait(&cv, \
&mutex);<br /> /* CHECK: make sure the cv_broadcast \
was called */<br /> mutex_exit(&mutex);<br /><br />
/* CHECK: make sure the cv_broadcast was called */<br />
cv_destroy(&cv);<br />
}<br /><br />
thread_B()<br />
{<br />
ASSERT(!MUTEX_HELD(&mutex));<br />
flag = 1;<br /><br />
/* HERE */<br /><br />
cv_broadcast(&cv);<br />
}<br /><span class="HOEnZb"><font color="#888888"><br /><br /><br />
--<br />
+-------------------------------------------+<br />
> Marcel Telka e-mail: <a \
> href="mailto:marcel@telka.sk">marcel@telka.sk</a> |<br />
> homepage: <a \
> href="http://telka.sk/" rel="noreferrer" target="_blank">http://telka.sk/</a> |<br \
> />
> jabber: <a \
> href="mailto:marcel@jabber.sk">marcel@jabber.sk</a> |<br />
+-------------------------------------------+<br \
/></font></span></blockquote></div><br /><br clear="all" /><br />-- <br /><div \
class="gmail_signature"><div dir="ltr">Paul Dagnelie<br \
/></div></div></div></html><div bgcolor="#ffffff" id="listbox-footer" \
style="width:auto;margin:0;padding:5px;background-color:#fff;clear:both;border-top: \
1px solid #ccc;"><table bgcolor="#ffffff" border="0" cellpadding="0" cellspacing="0" \
style="background-color:#fff" width="100%"><tr><td padding="4px"><font \
color="#333333" size="1" style="font-family:helvetica, sans-serif;"> \
<strong>illumos-developer</strong> | <a \
href="https://www.listbox.com/member/archive/182179/=now" \
style="text-decoration:none;color:#669933;border-bottom: 1px solid #444444" title="Go \
to archives for illumos-developer">Archives</a> <a border="0" \
href="https://www.listbox.com/member/archive/rss/182179/25758058-4e9228dc" \
style="text-decoration:none;color:#669933" title="RSS feed for \
illumos-developer"><img border="0" \
src="http://postlink.www.listbox.com/2067862/833487e62783d55fe81f119fb93ef644/25758058 \
/bb3fe179.jpg?uri=aHR0cHM6Ly93d3cubGlzdGJveC5jb20vaW1hZ2VzL2ZlZWQtaWNvbi0xMHgxMC5qcGc" \
/></a> | <a href="https://www.listbox.com/member/?member_id=25758058&id_secret=25758058-c19b436a" \
style="text-decoration:none;color:#669933;border-bottom: 1px solid #444444" \
title="">Modify</a> Your Subscription<td align="right" valign="top"><a \
href="http://www.listbox.com" style="border-bottom:none;"> <img border="0" \
src="http://postlink.www.listbox.com/2067863/3379085af0f1cf7fc3708f04b4471ae2/25758058 \
/bb3fe179.png?uri=aHR0cHM6Ly93d3cubGlzdGJveC5jb20vaW1hZ2VzL2xpc3Rib3gtbG9nby1zbWFsbC5wbmc" \
title="Powered by Listbox" /></a></td></font></td></tr></table></div></html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic