<p> Attention is currently required from: Sean Bright. </p>
<p><a href="https://gerrit.asterisk.org/c/asterisk/+/15227">View Change</a></p><p>2 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">Patchset:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/15227?tab=comments">Patch Set #3:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">re: volatile […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">unloading is only ever set once.  And will always be async to the reads thereof.  And it's absolutely critical that if an unload is started that wherever we get to in get_lock() we honour that, so if there is any chance that the compile will cache the unloading variable in a register it should be eliminated.  volatile is the easy way of ensuring that, without locking and memory barriers (much like the jiffies example in the document you referenced).</p><p style="white-space: pre-wrap; word-wrap: break-word;">As a general rule, yes, I agree that use of volatile must be avoided.  Looking at get_lock again for this, the only real risk is the last conditional for ast_cond_signal in my opinion.  I think it should be safe to drop volatile here since there all access to unloading prior to this are conditional anyway (and result in return), so unloading is extremely unlikely to be consider register cacheable anyway.  If you prefer I'll drop it, since I never unload this module except for shutdown anyway a crash at that point won't bother me too much but I'd still prefer to avoid it.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Your call.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/15227?tab=comments">Patch Set #3:</a> </p><p style="white-space: pre-wrap; word-wrap: break-word;">Thanks!  Anything else?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/15227">change 15227</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/15227"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I6f68b5ec82ff25b2909daf6e4d19ca864a463e29 </div>
<div style="display:none"> Gerrit-Change-Number: 15227 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Jaco Kroon <jaco@uls.co.za> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-CC: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-CC: Sean Bright <sean.bright@gmail.com> </div>
<div style="display:none"> Gerrit-Attention: Sean Bright <sean.bright@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 18 Dec 2020 20:04:42 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: Sean Bright <sean.bright@gmail.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>