[asterisk-dev] [Code Review] 2824: Fix DEBUG_THREADS when lock is acquired in __constructor__

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2824/#review9595
-----------------------------------------------------------


Note that the comments I made in __ast_cond_wait() also apply to __ast_cond_timedwait(), but I did not feel the need to repeat myself.


/branches/1.8/main/lock.c
<https://reviewboard.asterisk.org/r/2824/#comment18776>

    It's still probably a good idea to check that lt->reentrancy is not already 0 at this point. The error message that was removed could be useful if you call ast_cond_wait() and provide a mutex that is not locked.



/branches/1.8/main/lock.c
<https://reviewboard.asterisk.org/r/2824/#comment18778>

    This call to ast_remove_lock_info() will need to be modified somehow. This decrements the number of times that a given lock has been locked by one. Even though the reentrancy of the lock has been zeroed, the stored lock info will only show it as if the recursive lock had been unlocked once.
    
    This means that "core show locks" output may show this lock being held even though it is not held at all during an ast_cond_wait().



/branches/1.8/main/lock.c
<https://reviewboard.asterisk.org/r/2824/#comment18777>

    The bt passed into this ast_store_lock_info() call is NULL. This won't cause anything catastrophic, but it will mean that "core show locks" will be unable to show the backtrace leading to lock acquisition.


- Mark Michelson


On Sept. 5, 2013, 3:21 a.m., David Lee wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2824/
> -----------------------------------------------------------
> 
> (Updated Sept. 5, 2013, 3:21 a.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
> 
> 
> Bugs: ASTERISK-22455
>     https://issues.asterisk.org/jira/browse/ASTERISK-22455
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch fixes some long-standing bugs in debug threads that were
> exacerbated with recent Optional API work in Asterisk 12.
> 
> With debug threads enabled, on some systems, there's a lock ordering
> problem between our mutex and glibc's mutex protecting its module list
> (Ubuntu Lucid, glibc 2.11.1 in this instance). In one thread, the module
> list will be locked before acquiring our mutex. In another thread, our
> mutex will be locked before locking the module list (which happens in
> the depths of calling backtrace()).
> 
> This patch fixes this issue by moving backtrace() calls outside of
> critical sections that have the mutex acquired. The bigger change was to
> reentrancy tracking for ast_cond_{timed,}wait, which wrongly assumed
> that waiting on the mutex was equivalent to a single unlock (it actually
> suspends all recursive locks on the mutex).
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/main/lock.c 398291 
> 
> Diff: https://reviewboard.asterisk.org/r/2824/diff/
> 
> 
> Testing
> -------
> 
> Asterisk 12, DEBUG_THREADS+OPTIONAL_API on Ubuntu Lucid starts without
> deadlock.
> 
> Asterisk 1.8, DEBUG_THREADS runs fine.
> 
> 
> Thanks,
> 
> David Lee
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130905/7ddbbd36/attachment-0001.htm>