> On Dec. 8, 2014, 7:53 p.m., Matt Jordan wrote: > > /branches/1.8/main/lock.c, lines 135-141 > > <https://reviewboard.asterisk.org/r/4247/diff/2/?file=69604#file69604line135> > > > > Given the code in __ast_rwlock_init, I'm not sure this code should > > still be commented out. Going _way_ back in time, we have the following > > when this was committed: > > > > ------------------------------------------------------------------------ > > r87740 | tilghman | 2007-10-30 18:08:59 -0500 (Tue, 30 Oct 2007) | 13 > > lines > > > > Merged revisions 87739 via svnmerge from > > https://origsvn.digium.com/svn/asterisk/branches/1.4 > > > > ........ > > r87739 | tilghman | 2007-10-30 18:02:22 -0500 (Tue, 30 Oct 2007) | 5 > > lines > > > > Fix for uninitialized mutexes on *BSD > > Reported by: ys > > Fixed by: ys > > Closes issue #11116 > > > > ........ > > > > ------------------------------------------------------------------------ > > > > > > I think we should either remove the commented out code, or uncomment > > it. Since this exists in similar mutex initialization, I'd say uncomment it.
Considering that I removed the DEBUG_THREADS "helper" indirect mutex initialization code, you are right that it probably should be uncommented now. - rmudgett ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4247/#review13929 ----------------------------------------------------------- On Dec. 8, 2014, 6:17 p.m., rmudgett wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4247/ > ----------------------------------------------------------- > > (Updated Dec. 8, 2014, 6:17 p.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-19463 and ASTERISK-22455 > https://issues.asterisk.org/jira/browse/ASTERISK-19463 > https://issues.asterisk.org/jira/browse/ASTERISK-22455 > > > Repository: Asterisk > > > Description > ------- > > This patch started with David Lee's patch at > https://reviewboard.asterisk.org/r/2826/ and includes a regression fix > introduced by the ASTERISK-22455 patch. > > The initialization of a mutex's lock tracking structure was not protected > in a critical section. This is fine for any mutex that is explicitly > initialized, but a static mutex may have its lock tracking double > initialized if multiple threads attempt the first lock simultaneously. > > * Added a global mutex to properly serialize initialization of the lock > tracking structure. The painful global lock can be mitigated by adding a > double checked lock flag as discussed on the original review request. > > * Defer lock tracking initialization until first use. > > * Don't be "helpful" and initialize an uninitialized lock when > DEBUG_THREADS is enabled. Debug code is not supposed to fix or change > normal code behavior. We don't need a lock initialization race that would > force a re-setup of lock tracking. Lock tracking already handles > initialization on first use. > > * Properly handle allocation failures of the lock tracking structure. > > * No need to initialize tracking data in __ast_pthread_mutex_destroy() > just to turn around and destroy it. > > > The regression introduced by ASTERISK-22455 is the result of manipulating > a pthread_mutex_t struct outside of the pthread library code. The > pthread_mutex_t struct seems to have a global linked list pointer member > that can get changed by other threads. Therefore, saving and restoring > the contents of a pthread_mutex_t struct is a bad thing. > > Thanks to Thomas Airmont for finding this obscure regression. > > * Don't overwrite the struct ast_lock_track.reentr_mutex member to restore > tracking data in __ast_cond_wait() and __ast_cond_timedwait(). The > pthread_mutex_t struct must be treated as a read-only opaque variable. > > > Miscellaneous other items fixed by this patch: > > * Match ast_suspend_lock_info() with ast_restore_lock_info() in > __ast_cond_timedwait(). > > * Made some uninitialized lock sanity checks return EINVAL and try a > DO_THREAD_CRASH. > > * Fix bad canlog initialization expressions. > > > NOTE: The first diff on this review is the unmodified > https://reviewboard.asterisk.org/r/2826/ patch for comparison with the > updated patch. > > > Diffs > ----- > > /branches/1.8/main/lock.c 429126 > /branches/1.8/include/asterisk/lock.h 429126 > > Diff: https://reviewboard.asterisk.org/r/4247/diff/ > > > Testing > ------- > > Without the patch on v1.8, I repeatedly ran the testsuite masquerade > supertest and it died an hour or two later. With the patch, it ran over > the weekend without a problem. > > Since the DEBUG_THREADS locking issues on Asterisk startup > (ASTERISK-19463) have been a hard problem to reproduce, I propose we setup > Bamboo to run the TestSuite with DEBUG_THREADS enabled on the > http://svn.asterisk.org/svn/asterisk/team/rmudgett/debug_threads branch > nightly for a few weeks. > > > Thanks, > > rmudgett > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev