<br><br><div class="gmail_quote">2008/7/4 Russell Bryant <<a href="mailto:russell@digium.com">russell@digium.com</a>>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="Ih2E3d"><br>
----- "Stephen Davies" <<a href="mailto:stephen.l.davies@gmail.com">stephen.l.davies@gmail.com</a>> wrote:<br>
> I think the discussion reveals that the magic trylock while loop looks<br></div><div class="Ih2E3d">
> neato but doesn't really solve the problem. Tzafrir and Luigi pointed<br>
> out some flaws.<br>
<br>
</div>It doesn't just look neato. It does actually work, except for when the recursive property of the locks are used.<br>
<div class="Ih2E3d"></div></blockquote><div><br><br>OK point taken. Ish. Because interestingly I've fixed a deadlock recently in chan_local that was to do with the fact that a mutex had two outstanding locks. So when the magic trylock look in local_queue_frame tried to do its thing, the underlying lock never got released and you STILL end up with a deadlock (or livelock)<br>
<br>Here was the fix for that:<br><br><div>Index: channels/chan_local.c</div><div>===================================================================</div><div>--- channels/chan_local.c<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 105553)</div>
<div>+++ channels/chan_local.c<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -153,6 +153,7 @@</div><div> static int local_queue_frame(struct local_pvt *p, int isoutbound, struct ast_frame *f, struct ast_channel *us)</div>
<div> {</div><div> <span class="Apple-tab-span" style="white-space:pre"> </span>struct ast_channel *other = NULL;</div><div>+<span class="Apple-tab-span" style="white-space:pre"> </span>int doublelock = 0;</div><div> </div>
<div> <span class="Apple-tab-span" style="white-space:pre"> </span>/* Recalculate outbound channel */</div><div> <span class="Apple-tab-span" style="white-space:pre"> </span>other = isoutbound ? p->owner : p->chan;</div>
<div>@@ -175,11 +176,23 @@</div><div> <span class="Apple-tab-span" style="white-space:pre"> </span>/* Ensure that we have both channels locked */</div><div> <span class="Apple-tab-span" style="white-space:pre"> </span>while (other && ast_channel_trylock(other)) {</div>
<div> <span class="Apple-tab-span" style="white-space:pre"> </span>ast_mutex_unlock(&p->lock);</div><div>-<span class="Apple-tab-span" style="white-space:pre"> </span>if (us)</div><div>+<span class="Apple-tab-span" style="white-space:pre"> </span>if (us) {</div>
<div> <span class="Apple-tab-span" style="white-space:pre"> </span>ast_channel_unlock(us);</div><div>+<span class="Apple-tab-span" style="white-space:pre"> </span>/* SLD: In some circumstances with generators running on the local channel it is possible</div>
<div>+<span class="Apple-tab-span" style="white-space:pre"> </span> for there to be two locks on the us channel. Essential to drop them both to avoid a</div><div>+<span class="Apple-tab-span" style="white-space:pre"> </span> deadlock */</div>
<div>+<span class="Apple-tab-span" style="white-space:pre"> </span>if (!ast_channel_unlock(us)) {</div><div>+<span class="Apple-tab-span" style="white-space:pre"> </span>doublelock = 1;</div><div>+<span class="Apple-tab-span" style="white-space:pre"> </span>if (option_debug > 2)</div>
<div>+<span class="Apple-tab-span" style="white-space:pre"> </span>ast_log(LOG_VERBOSE, "local_queue_frame had to unlock \"us\" twice\n");</div><div>+<span class="Apple-tab-span" style="white-space:pre"> </span>}</div>
<div>+<span class="Apple-tab-span" style="white-space:pre"> </span>}</div><div> <span class="Apple-tab-span" style="white-space:pre"> </span>usleep(1);</div><div>-<span class="Apple-tab-span" style="white-space:pre"> </span>if (us)</div>
<div>+<span class="Apple-tab-span" style="white-space:pre"> </span>if (us) {</div><div> <span class="Apple-tab-span" style="white-space:pre"> </span>ast_channel_lock(us);</div><div>+<span class="Apple-tab-span" style="white-space:pre"> </span>if (doublelock)</div>
<div>+<span class="Apple-tab-span" style="white-space:pre"> </span>ast_mutex_lock(us);</div><div>+<span class="Apple-tab-span" style="white-space:pre"> </span>}</div><div> <span class="Apple-tab-span" style="white-space:pre"> </span>ast_mutex_lock(&p->lock);</div>
<div> <span class="Apple-tab-span" style="white-space:pre"> </span>other = isoutbound ? p->owner : p->chan;</div><div> <span class="Apple-tab-span" style="white-space:pre"> </span>}</div><div><br></div><br>Thinking back, there was a point in the past when the use of recursive locks was started. Or maybe I'm remembering wrong and it was a point at which they became mandatory.<br>
<br>Anyway - I guess that the magic trylock look technique was used way back before the recursive mutexes came in.<br><br><goes to look at <a href="http://1.0.9.">1.0.9.</a>..><br><br>Yep - I can see it in there; though often without the while.<br>
<br>But even 1.0.9 Asterisk required recursive mutexes - so it was before then...<br><br>Steve<br></div></div>