<html>
<head>
<base href="https://wiki.asterisk.org/wiki">
<link rel="stylesheet" href="/wiki/s/2042/1/7/_/styles/combined.css?spaceKey=AST&forWysiwyg=true" type="text/css">
</head>
<body style="background: white;" bgcolor="white" class="email-body">
<div id="pageContent">
<div id="notificationFormat">
<div class="wiki-content">
<div class="email">
<h2><a href="https://wiki.asterisk.org/wiki/display/AST/Locking+in+Asterisk">Locking in Asterisk</a></h2>
<h4>Page <b>edited</b> by <a href="https://wiki.asterisk.org/wiki/display/~lmadsen">Leif Madsen</a>
</h4>
<div id="versionComment">
<b>Comment:</b>
Add comments I found on reviewboard from David Vossel about deadlock avoidance.<br />
</div>
<br/>
<h4>Changes (1)</h4>
<div id="page-diffs">
<table class="diff" cellpadding="0" cellspacing="0">
<tr><td class="diff-snipped" >...<br></td></tr>
<tr><td class="diff-unchanged" >Please try to use locks only when strictly necessary, and only for the minimum amount of time required to run critical sections of code. A common use of locks in the current code is to protect a data structure from being released while you use it. With the use of reference-counted objects (astobj2) this should not be necessary anymore. <br> <br></td></tr>
<tr><td class="diff-added-lines" style="background-color: #dfd;">There is only one time in the entire code base where deadlock avoidance is absolutely necessary and that is when multiple channels are being locked at the same time. This is because there is no locking order established for locking multiple channels. <br> <br>With everything else, there is a locking order established. In this case the channels container must always be locked before an individual channel. Deadlock avoidance is not necessary here. <br> <br>So, never use deadlock avoidance unless you have to grab two channel locks at the same time, otherwise it is not necessary. I know the code has misuses of deadlock avoidance all over the place, ignore them. They will go away in time. <br> <br>{note} <br>As a side note, if you ever find yourself in the position where you are designing a program and think using deadlock avoidance is a quick solution for a concurrency problem you run into, it is not. Running into a deadlock almost certainly means your design is flawed. I'd go as far as to say if you ever find yourself designing a system with multiple locks held at the same time, your design is flawed. Seriously, anyone who reads this remember this, and say it to yourself every time you create a new mutex. <br> -- David Vossel <br>{note} <br> <br></td></tr>
<tr><td class="diff-unchanged" >h5. Do not sleep while holding a lock <br> <br></td></tr>
<tr><td class="diff-snipped" >...<br></td></tr>
</table>
</div> <h4>Full Content</h4>
<div class="notificationGreySide">
<h3><a name="LockinginAsterisk-LockingFundamentals"></a>Locking Fundamentals</h3>
<p>Asterisk is a heavily multithreaded application. It makes extensive use of locking to ensure safe access to shared resources between different threads.</p>
<p>When more that one lock is involved in a given code path, there is the potential for deadlocks. A deadlock occurs when a thread is stuck waiting for a resource that it will never acquire. Here is a classic example of a deadlock:</p>
<div class="code panel" style="border-width: 1px;"><div class="codeContent panelContent">
<script type="syntaxhighlighter" class="toolbar: false; theme: Confluence; brush: java; gutter: false"><![CDATA[
Thread 1 Thread 2
------------ ------------
Holds Lock A Holds Lock B
Waiting for Lock B Waiting for Lock A
]]></script>
</div></div>
<p>In this case, there is a deadlock between threads 1 and 2. This deadlock would have been avoided if both threads had agreed that one must acquire Lock A before Lock B.</p>
<p>In general, the fundamental rule for dealing with multiple locks is: <b>an order <em>must</em> be established to acquire locks, and then all threads must respect that order when acquiring locks</b>.</p>
<h5><a name="LockinginAsterisk-Establishingalockingorder"></a>Establishing a locking order</h5>
<p>Because any ordering for acquiring locks is ok, one could establish the rule arbitrarily, e.g. ordering by address, or by some other criterion.<br/>
The main issue, though, is defining an order that</p>
<ol>
<li>is easy to check at runtime;</li>
<li>reflects the order in which the code executes.</li>
</ol>
<p>As an example, if a data structure B is only accessible through a data structure A, and both require locking, then the natural order is locking first A and then B. As another example, if we have some unrelated data structures to be locked in pairs, then a possible order can be based on the address of the data structures themselves.</p>
<h3><a name="LockinginAsterisk-MindingtheboundarybetweenchanneldriversandtheAsteriskcore"></a>Minding the boundary between channel drivers and the Asterisk core</h3>
<p>The #1 cause of deadlocks in Asterisk is by not properly following the locking rules that exist at the boundary between Channel Drivers and<br/>
the Asterisk core. The Asterisk core allocates an ast_channel, and Channel Drivers allocate "technology specific private data" (PVT) that is<br/>
associated with an ast_channel. Typically, both the ast_channel and PVT have their own lock. There are <em>many</em> code paths that require both objects to be locked.</p>
<p>The locking order in this situation is the following:</p>
<ol>
<li>ast_channel</li>
<li>PVT</li>
</ol>
<p>Channel Drivers implement the ast_channel_tech interface to provide a channel implementation for Asterisk. Most of the channel_tech<br/>
interface callbacks are called with the associated ast_channel locked. When accessing technology specific data, the PVT can be locked directly because the locking order is respected.</p>
<h3><a name="LockinginAsterisk-Preventinglockorderingreversals."></a>Preventing lock ordering reversals.</h3>
<p>There are some code paths which make it extremely difficult to respect the locking order.<br/>
Consider for example the following situation:</p>
<ol>
<li>A message comes in over the "network"</li>
<li>The Channel Driver (CD) monitor thread receives the message</li>
<li>The CD associates the message with a PVT and locks the PVT</li>
<li>While processing the message, the CD must do something that requires locking the ast_channel associated to the PVT</li>
</ol>
<p>This is the point that must be handled carefully.</p>
<p>The following psuedo-code</p>
<div class="code panel" style="border-width: 1px;"><div class="codeContent panelContent">
<script type="syntaxhighlighter" class="toolbar: false; theme: Confluence; brush: java; gutter: false"><![CDATA[
unlock(pvt);
lock(ast_channel);
lock(pvt);
]]></script>
</div></div>
<p>is <em>not</em> correct for two reasons:</p>
<ol>
<li>first and foremost, unlocking the PVT means that other threads can acquire the lock and believe it is safe to modify the associated data. When reacquiring the lock, the original thread might find unexpected changes in the protected data structures. This essentially means that the original thread must behave as if the lock on the pvt was not held, in which case it could have released it itself altogether;</li>
<li>Asterisk uses the so called "recursive" locks, which allow a thread to issue a lock() call multiple times on the same lock. Recursive locks count the number of calls, and they require an equivalent number of unlock() to be actually released.</li>
</ol>
<p>For this reason, just calling unlock() once does not guarantee that the lock is actually released – it all depends on how many times lock() was called before.</p>
<p>An alternative, but still incorrect, construct is widely used in the asterisk code to try and improve the situation:</p>
<div class="code panel" style="border-width: 1px;"><div class="codeContent panelContent">
<script type="syntaxhighlighter" class="toolbar: false; theme: Confluence; brush: java; gutter: false"><![CDATA[
while (trylock(ast_channel) == FAILURE) {
unlock(pvt);
usleep(1); /* yield to other threads */
lock(pvt);
}
]]></script>
</div></div>
<p>Here the trylock() is non blocking, so we do not deadlock if the ast_channel is already locked by someone else: in this case, we try to unlock the PVT (which happens only if the PVT lock counter is 1), yield the CPU to give other threads a chance to run, and then acquire the lock again.</p>
<p>This code is not correct for two reasons:</p>
<ol>
<li>same as in the previous example, it releases the lock when the thread probably did not expect it;</li>
<li>if the PVT lock counter is greater than 1 we will not really release the lock on the PVT. We might be lucky and have the other contender actually release the lock itself, and so we will "win" the race, but if both contenders have their lock counts > 1 then they will loop forever (basically replacing deadlock with livelock).</li>
</ol>
<p>Another variant of this code is the following:</p>
<div class="code panel" style="border-width: 1px;"><div class="codeContent panelContent">
<script type="syntaxhighlighter" class="toolbar: false; theme: Confluence; brush: java; gutter: false"><![CDATA[
if (trylock(ast_channel) == FAILURE) {
unlock(pvt);
lock(ast_channel);
lock(pvt);
}
]]></script>
</div></div>
<p>which has the same issues as the while(trylock...) code, but just deadlocks instead of looping forever in case of lock counts > 1.</p>
<p>The deadlock/livelock could be in principle spared if one had an unlock_all() function that calls unlock as many times as needed to actually release the lock, and reports the count. Then we could do:</p>
<div class="code panel" style="border-width: 1px;"><div class="codeContent panelContent">
<script type="syntaxhighlighter" class="toolbar: false; theme: Confluence; brush: java; gutter: false"><![CDATA[
if (trylock(ast_channel) == FAILURE) {
n = unlock_all(pvt);
lock(ast_channel)
while (n-- > 0) lock(pvt);
}
]]></script>
</div></div>
<p>The issue with unexpected unlocks remains, though.</p>
<h3><a name="LockinginAsterisk-Lockingmultiplechannels."></a>Locking multiple channels.</h3>
<p>The next situation to consider is what to do when you need a lock on multiple ast_channels (or multiple unrelated data structures).</p>
<p>If we are sure that we do not hold any of these locks, then the following construct is sufficient:</p>
<div class="code panel" style="border-width: 1px;"><div class="codeContent panelContent">
<script type="syntaxhighlighter" class="toolbar: false; theme: Confluence; brush: java; gutter: false"><![CDATA[
lock(MIN(chan1, chan2));
lock(MAX(chan1, chan2));
]]></script>
</div></div>
<p>That type of code would follow an established locking order of always locking the channel that has a lower address first. Also keep in mind<br/>
that to use this construct for channel locking, one would have to go through the entire codebase to ensure that when two channels are locked,<br/>
this locking order is used.</p>
<p>However, if we enter the above section of code with some lock held (which would be incorrect using non-recursive locks, but is completely legal using recursive mutexes) then the locking order is not guaranteed anymore because it depends on which locks we already hold. So we have to go through the same tricks used for the channel+PVT case.</p>
<h3><a name="LockinginAsterisk-Recommendations"></a>Recommendations</h3>
<p>As you can see from the above discussion, getting locking right is all but easy. So please follow these recommendations when using locks:</p>
<h5><a name="LockinginAsterisk-Uselocksonlywhenreallynecessary"></a>Use locks only when really necessary</h5>
<p>Please try to use locks only when strictly necessary, and only for the minimum amount of time required to run critical sections of code. A common use of locks in the current code is to protect a data structure from being released while you use it. With the use of reference-counted objects (astobj2) this should not be necessary anymore.</p>
<p>There is only one time in the entire code base where deadlock avoidance is absolutely necessary and that is when multiple channels are being locked at the same time. This is because there is no locking order established for locking multiple channels.</p>
<p>With everything else, there is a locking order established. In this case the channels container must always be locked before an individual channel. Deadlock avoidance is not necessary here.</p>
<p>So, never use deadlock avoidance unless you have to grab two channel locks at the same time, otherwise it is not necessary. I know the code has misuses of deadlock avoidance all over the place, ignore them. They will go away in time.</p>
<div class='panelMacro'><table class='noteMacro'><colgroup><col width='24'><col></colgroup><tr><td valign='top'><img src="/wiki/images/icons/emoticons/warning.gif" width="16" height="16" align="absmiddle" alt="" border="0"></td><td>As a side note, if you ever find yourself in the position where you are designing a program and think using deadlock avoidance is a quick solution for a concurrency problem you run into, it is not. Running into a deadlock almost certainly means your design is flawed. I'd go as far as to say if you ever find yourself designing a system with multiple locks held at the same time, your design is flawed. Seriously, anyone who reads this remember this, and say it to yourself every time you create a new mutex.<br/>
– David Vossel</td></tr></table></div>
<h5><a name="LockinginAsterisk-Donotsleepwhileholdingalock"></a>Do not sleep while holding a lock</h5>
<p> If possible, do not run any blocking code while holding a lock,<br/>
because you will also block other threads trying to access the same<br/>
lock. In many cases, you can hold a reference to the object to avoid<br/>
that it is deleted while you sleep, perhaps set a flag in the object<br/>
itself to report other threads that you have some pending work to<br/>
complete, then release and acquire the lock around the blocking path,<br/>
checking the status of the object after you acquire the lock to make<br/>
sure that you can still perform the operation you wanted to.</p>
<h5><a name="LockinginAsterisk-Trynottoexploitthe%27recursive%27featureoflocks."></a>Try not to exploit the 'recursive' feature of locks.</h5>
<p> Recursive locks are very convenient when coding, as you don't have to<br/>
worry, when entering a section of code, whether or not you already<br/>
hold the lock – you can just protect the section with a lock/unlock<br/>
pair and let the lock counter track things for you.<br/>
But as you have seen, exploiting the features of recursive locks<br/>
make it a lot harder to implement proper deadlock avoidance strategies.<br/>
So please try to analyse your code and determine statically whether you<br/>
already hold a lock when entering a section of code.<br/>
If you need to call some function foo() with and without a lock held,<br/>
you could define two function as below:</p>
<div class="code panel" style="border-width: 1px;"><div class="codeContent panelContent">
<script type="syntaxhighlighter" class="toolbar: false; theme: Confluence; brush: java; gutter: false"><![CDATA[
foo_locked(...) {
... do something, assume lock held
}
foo(...) {
lock(xyz)
ret = foo_locked(...)
unlock(xyz)
return ret;
}
]]></script>
</div></div>
<p> and call them according to the needs.</p>
<h5><a name="LockinginAsterisk-Documentlockingrules."></a>Document locking rules.</h5>
<p>Please document the locking order rules are documented for every lock introduced into Asterisk. This is done almost nowhere in the existing code. However, it will be expected to be there for newly introduced code. Over time, this information should be added for all of the existing lock usage.</p>
</div>
<div id="commentsSection" class="wiki-content pageSection">
<div style="float: right;">
<a href="https://wiki.asterisk.org/wiki/users/viewnotifications.action" class="grey">Change Notification Preferences</a>
</div>
<a href="https://wiki.asterisk.org/wiki/display/AST/Locking+in+Asterisk">View Online</a>
|
<a href="https://wiki.asterisk.org/wiki/pages/diffpagesbyversion.action?pageId=5243132&revisedVersion=4&originalVersion=3">View Changes</a>
|
<a href="https://wiki.asterisk.org/wiki/display/AST/Locking+in+Asterisk?showComments=true&showCommentArea=true#addcomment">Add Comment</a>
</div>
</div>
</div>
</div>
</div>
</body>
</html>