Summary:	ASTERISK-04990: [patch] [post 1.2] Implement channel locking functions
Reporter:	Olle Johansson (oej)	Labels:
Date Opened:	2005-09-05 09:36:14	Date Closed:	2006-01-26 12:49:53.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) chanlock.txt ( 1) issue5124.patch.r7353
Description:	This patch implements channel locking functions in chan_sip and channel.c/h. These functions helped me a lot to debug channel locking in chan_sip and it makes it easier to read code - you see that someone locks or unlocks a channel. Also, you get the channel identifier in debug output. If this is accepted, we can convert more parts of asterisk to use this code.
Comments:	By: Michael Jerris (mikej) 2005-09-05 10:24:33 We need to start marking these types of changes post 1.2 or we will never have a release. By: Olle Johansson (oej) 2005-09-05 12:52:33 By moving this to post 1.2, you're making it harder to implement my SIP transfer patch that Kevin is waiting for... By: Michael Jerris (mikej) 2005-09-05 20:32:24 I am by no means the arbiter of what is post 1.2 and what is not..... That is between you and kevin.  If sip transfer is 100% needed for 1.2 that we push it out another month or two, so be it.  From a practical standpoint, it has waited for a year+ sense the last release, I say if people are so gung ho on some of this stuff, that we stablize 1.2 now, and schedule another release for 3 months out, instead of 14+ next time. By: Tilghman Lesher (tilghman) 2005-09-05 21:36:26 oej:  you missed the cutoff date for new features for 1.2 by over a month.  If you expect this to make it into 1.2, you need to request an exception from Mark or Kevin.  We're not informing you of this policy; we're reminding you of that decision which was made and published prior to the cutoff date -- and which you, of all people, should have been aware of. By: Olle Johansson (oej) 2005-09-06 00:45:32 Corydon76: The SIP transfer bug has been in the tracker for months previous to the cut-off. Of course I am aware of what you are saying, and this is all based on an agreement between me and Kevin after going through SIP bugs with him for a few days in his office. By: Kevin P. Fleming (kpfleming) 2005-09-07 15:04:51 It is true that we are trying to get the SIP transfer stuff fixed properly before releasing 1.2, as there have been open bugs for which the previous solutions were not adequate. However, that does not mean that a massively invasive change to chan_sip can go in at this date; we have to take it in steps. With that said: if these functions don't ptovide needed functionality, but are just wrappers over the existing methods that provide additional debugging, I don't see any reason they need to go in before 1.2 is released. If they can be utilized later by just doing a simple search-and-replace, that would be preferable. By: Michael Jerris (mikej) 2005-09-24 18:53:36 oej, so do these need to be in 1.2 or not? By: Michael Jerris (mikej) 2005-12-01 13:52:06.000-0600 Can we please get an updated patch for this for current svn trunk.  Thanks. By: Russell Bryant (russell) 2005-12-02 03:12:11.000-0600 A couple of comments about performance issues ... 1) Since these operations need to happen very quickly, it seems that they would be good candidates for implementation using the AST_INLINE_API. 2) I would vote for only including all of this debug code if DEBUG_THREADS is defined.  Otherwise, they should be an inlined direct wrapper of ast_mutex_lock, etc. By: Russell Bryant (russell) 2005-12-02 17:47:39.000-0600 Instead of AST_INLINE_API, maybe these should be declared using the "force_inline" directive that was added in the last few months ... By: Olle Johansson (oej) 2005-12-02 18:29:20.000-0600 Drumkilla: Debugging threads is another issue, I believe. I just want to have more clear debug on channel locking, not the full threads debug... But that's my experience after chasing deadlocks in SIP transfers all summer... I can't comment on AST_INLINE beacuse of incompetence in that regards. By: Russell Bryant (russell) 2005-12-05 17:45:19.000-0600 This patch implements some of my suggested changes.  In detail: - Update the patch to r7353 of the SVN trunk. - Fix a typo in the chan_sip conversion were ast_channel_unlock was given an 'struct ast_channel **' instead of a 'struct ast_channel *'. - I moved the functions completely into channel.h so that I could define them as inlined.  I also wrapped all of the additional debug code in an #ifdef CHANLOCK_DEBUG.  Since channel locking/unlocking is time sensitive, I don't think it should be there during regular use. This way, in the case when CHANLOCK_DEBUG is not defined, a call to ast_channel_lock will be optimized to be the exact same thing as a call to ast_mutex_lock. An inlined function is essentially the same thing as a macro.  It provides the speed benefits of executing the code 'inline' with the function caller's code.  However, using an inlined function provides the added benefit of type checking on the arguments as well as easier debugging.  It also suffers from the same downfall as macros in that it results in larger object code. In Asterisk, if we define a function with the AST_INLINE_API macro, the function will only be inlined if LOW_MEMORY is *not* defined.  This is because we want the resulting object code to be as small as possible for an embedded platform. For these functions, I have defined them with a directive called 'force_inline' which is something we have defined in Asterisk.  If we're using GNUC, force_inline will add an attribute to the function declaration that tells the compiler to *always* inline this function.  Otherwise, force_inline will just be replaced with 'inline'.  This is a good idea for operations that need to happen as fast as possible. I hope this all makes sense.  :) By: Olle Johansson (oej) 2006-01-04 05:08:09.000-0600 Guess this is up to Kevin to decide about now. By: Olle Johansson (oej) 2006-01-04 05:09:48.000-0600 Note: I did not find a way to check the lock status on FreeBSD. By: Olle Johansson (oej) 2006-01-26 12:49:48.000-0600 Olle - move this to a branch and continue to play with it later.