<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1313/">https://reviewboard.asterisk.org/r/1313/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 8th, 2011, 4:01 p.m., <b>David Vossel</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I've looked over the code and functionally I don't see any problems. I'm still having a bit of trouble wrapping my head around why we need the new context_merge_lock though. If locking order inversion is taking place I don't understand how creating a new lock helps the situation. Can you clarify in a little more detail how this helps?</pre>
</blockquote>
<p>On July 8th, 2011, 4:23 p.m., <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The ast_merge_contexts_and_delete() function restructures the contexts and hints which is why nobody else should be doing anything with those containers when it is doing its job. Part of what ast_merge_contexts_and_delete() does is to remove all watcher callbacks from an old hint and then put them back into a new merged hint.
Since handle_statechange() cannot notify the watchers with the normal locks held because of deadlocks, a new lock is needed to ensure that those two functions do not interfere with each other. The other functions use the hints container lock to do the same thing.</pre>
</blockquote>
<p>On July 11th, 2011, 3:03 p.m., <b>David Vossel</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Essentially what this patch does is erase any concept of locking order between contexts and hints by introducing a global lock to serialize when those containers can be accessed. This is a solution, but I'm not convinced it is the right one yet.
so, Locking order is (contexts -> hints -> hint)
The source of this problem occurs at handle_statechange(). There we grab the hints and individual hint locks while calling the state change callback. I'm guessing that within a callback somewhere the context lock is locked which invalidates locking order. (hints -> hint -> contexts).
The right fix given the current architecture is to never call anything in the statechange callback that will result in the global context lock being held. This can be hard to guarantee though, and will likely occur again in the future given the complexity involved in some of those callbacks. Can we somehow remove the need to hold the hint/hints lock during the state change callback?</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">1) The patch does not erase the locking order concept for contexts -> hints -> hint. The new lock _only_ searializes access to the containers between handle_statechange() and ast_merge_contexts_and_delete() because ast_merge_contexts_and_delete() could conceivably radically change the links in the data structures. You could see it as changing the locking order to: new-lock -> contexts -> hints -> hint. However, the new lock only affects handle_statechange() and ast_merge_contexts_and_delete(); no other function. The ast_merge_contexts_and_delete() is only called during a module reload.
2) The handle_statechange() no longer holds those locks when notifying the watchers with this patch because of the catch-22 stated in the review description. So yes, the need to hold the hints/hint lock during the watcher notification callbacks is removed. One of the deadlocks (ASTERISK-16961) happen because of an interaction between another locking order group: contexts -> hints -> hint and channels -> channel -> channel private.
3) The handle_statechange() is effectively a thread since it is a callback of the task processor for pbx.c.</pre>
<br />
<p>- rmudgett</p>
<br />
<p>On July 8th, 2011, 11:01 a.m., rmudgett wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers, Russell Bryant and David Vossel.</div>
<div>By rmudgett.</div>
<p style="color: grey;"><i>Updated July 8, 2011, 11:01 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">There are two remaining different deadlocks reported dealing with dialplan
hints.
The deadlock in ASTERISK-17666 is caused by invalid locking order in
ast_remove_hint(). The hints container must be locked before the hint
object.
The deadlock in ASTERISK-17760 is caused by a catch-22 situation in
handle_statechange(). The deadlock is caused by not having the conlock
before calling the watcher callbacks. Unfortunately, having that lock
causes a different deadlock as reported in ASTERISK-16961.
* Fixed ast_remove_hint() locking order.
* Made handle_statechange() no longer call the watcher callbacks holding
any locks that matter.
* Made hint ao2 destructor do the watcher callbacks for extension
deactivation to guarantee that they get called.
* Fixed hint reference leak in ast_add_hint() if the callback container
constructor failed.
* Fixed hint reference leak in complete_core_show_hint() for every hint it
found for CLI tab completion.
* Adjusted locking in ast_merge_contexts_and_delete() for safety.
* Added context_merge_lock to prevent ast_merge_contexts_and_delete() and
handle_statechange() from interfering with each other.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">* Did some calls that changed the state of some hints.
* Reloaded the dialplan.
</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-17666">ASTERISK-17666</a>,
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-17760">ASTERISK-17760</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/1.8/main/pbx.c <span style="color: grey">(327043)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1313/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>