[asterisk-dev] [Code Review] A chan_local with no busy loop deadlock avoidance (and no deadlocks, too)

[Mark Michelson]

> On 2009-04-13 07:27:44, Russell Bryant wrote:
> > Part of this patch has to deal with deadlock avoidance issues when queuing a frame on to a channel.  A few weeks ago, there was a discussion on the asterisk-dev list about a more generic solution to simplifying locking when queuing frames.
> > 
> > http://lists.digium.com/pipermail/asterisk-dev/2009-March/037448.html
> > 
> > Given that we have a more generic solution on the horizon, I would like to hold off on this until we see how that pans out.  Even with the other changes, I'm sure there will be things left that could be improved in chan_local.

Well, I'm all for a more generic solution than what I have written. However, I did not read what you linked and glean that we "have a more generic solution on the horizon." If anything, it looked as though there was something suggested and all you stated was that the conversion would be difficult. I'll post a reply to that thread with my opinion on the matter.

If something more generic were used with regards to queueing frames, then my changes to local_queue_frame would not be necessary. However, the changes I have made to local_hangup are still relevant.


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/219/#review684
-----------------------------------------------------------


On 2009-04-11 18:01:50, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/219/
> -----------------------------------------------------------
> 
> (Updated 2009-04-11 18:01:50)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> chan_local is notorious for its ability to deadlock. Some functions in chan_local consist mainly of deadlock avoidance using busy loop constructs.
> 
> Upon inspecting the code closely, I found that local_queue_frame and local_hangup were the two functions which were the worst as far as this construct is concerned. This is because these two functions were attempting to lock two channels as well as the local_pvt. What I noticed was that both of these functions really didn't need to synchronously take its actions. It would be fine to push the tasks to another thread and have them taken care of there instead. That's where this patch comes in. With it, I have made use of a task processor in order to avoid all the deadlock avoidance code which had plagued chan_local before. The only place where you'll see deadlock avoidance used is in local_call, because we really do need to have both channels locked there.
> 
> I made this patch against the ast_channel_ao2 branch because I found that it was not possible to implement all the fixes I wanted to without the use of ref-counted channels. The problem was that the channel thread could free the channel out from under a task processor execution. With ref-counted channels, you don't have to worry about that at all since we can prevent the channel from being released until all references to it are removed. I found while working on this that I could also get rid of the LOCAL_GLARE_DETECT and LOCAL_CANCEL_QUEUE flags.
> 
> 
> Diffs
> -----
> 
>   /team/russell/ast_channel_ao2/channels/chan_local.c 188030 
> 
> Diff: http://reviewboard.digium.com/r/219/diff
> 
> 
> Testing
> -------
> 
> I have attempted placing calls to Local channels with and without the /n modifier. I also have attempted operations which use local channels internally, such as attended transfers and call-forwards. I ran both plain and under valgrind and have no errors.
> 
> In the interest of full disclosure, I discovered a crash at one point stemming from my use of realtime music on hold. I tested with vanilla trunk and found that the same crash occurred there, too, so it has nothing to do with my changes or the ast_channel_ao2 branch's changes. When I re-ran my tests with statically-configured music on hold, all went well.
> 
> 
> Thanks,
> 
> Mark
> 
>