<p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/10293">View Change</a></p><p>1 comment:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10293/1/include/asterisk/lock.h">File include/asterisk/lock.h:</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/10293/1/include/asterisk/lock.h@130">Patch Set #1, Line 130:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">struct ast_mutex_info {<br>   pthread_mutex_t mutex;<br>        /*! Track which thread holds this mutex */<br>    struct ast_lock_track *tracker;<br>};<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm not in favor of this patch as it just about precludes the most needed improvement to DEBUG_THREADS:  It needs to be made much faster.  It currently competes with valgrind for sluggishness. :)</p><p style="white-space: pre-wrap; word-wrap: break-word;">I think that a patch to improve the performance can be done by declaring the structs something like this:</p><p style="white-space: pre-wrap; word-wrap: break-word;">I am pretty sure this does not break ABI.</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">struct ast_lock_tracking_flags {<br>        /*! non-zero if lock tracking is enabled */<br>   unsigned int tracking:1;<br>      /*! non-zero if track is setup */<br>     unsigned int setup:1;<br>};</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">struct ast_mutex_info {<br>    pthread_mutex_t mutex;<br>        /*! Track which thread holds this mutex */<br>    struct ast_lock_track *track;<br> struct ast_lock_tracking_flags flags;<br>};</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">struct ast_rwlock_info {<br>   pthread_rwlock_t lock;<br>        /*! Track which thread holds this lock */<br>     struct ast_lock_track *track;<br> struct ast_lock_tracking_flags flags;<br>};</pre><p style="white-space: pre-wrap; word-wrap: break-word;">Then in ast_get_reentrancy():</p><p style="white-space: pre-wrap; word-wrap: break-word;">We take the advise of the comment and do a double-check.</p><p style="white-space: pre-wrap; word-wrap: break-word;">We can test setup for non-zero and immediately return the track pointer.  If it is zero then lock the reentrancy lock and check setup again.  If it is non-zero then unlock and return track.  Otherwise, we are the first to setup the lock tracking struct.  We then proceed to setup the track pointer and then set the setup flag.  We have to use this order to prevent an incomplete track pointer set from being used since setting a pointer is not atomic.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Obviously the parameters to ast_get_reentrancy() would need alteration to provide the needed access for mutex and rwlocks. :)</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/10293">change 10293</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/10293"/><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-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Iabd650908901843e9fff47ef1c539f0e1b8cb13b </div>
<div style="display:none"> Gerrit-Change-Number: 10293 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 (1000185) </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 27 Sep 2018 19:04:51 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>