Summary:	ASTERISK-03171: [PATCH] support lockless ASTOBJ FIND and FIND_UNLINK
Reporter:	Kevin P. Fleming (kpfleming)	Labels:
Date Opened:	2005-01-03 10:41:37.000-0600	Date Closed:	2008-01-15 15:20:21.000-0600
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) astobj_find_lockless_rev1.diff.txt
Description:	(this is a little complex, so please bear with me) In preparation for introduction of hashtable-based ASTOBJ containers, it becomes necessary to treat the "name" assigned to an ASTOBJ-based object as immutable (you cannot change it directly once you create the object). This is not a problem for chan_sip, since it has no need to do that, and for any other modules I will add ASTOBJ_RENAME or something similar to do the right thing(s). In any case, since the name cannot change, there is no need to lock the object just to compare the name when searching for it. This patch introduces optimized versions of ASTOBJ_CONTAINER_FIND and ASTOBJ_CONTAINER_FIND_UNLINK that do not lock the objects as they pass over them (the _FULL versions still do, because the comparison function is peeking inside the object). There are no changes required to chan_sip to accomodate this, it will just be faster and cause less lock contention. ****** ADDITIONAL INFORMATION ****** Disclaimer is on file. Patch is relative to bug ASTERISK-3193221.
Comments:	By: jerjer (jerjer) 2005-01-03 14:49:58.000-0600 Just deploed these two patches... Working good, so far and seems to have solved a deadlock issue we were having with just cvs -head chan_sip.c By: syslod (syslod) 2005-01-03 21:53:16.000-0600 works great.  Fixed all deadlock issues with SIP. By: Mark Spencer (markster) 2005-01-03 22:39:04.000-0600 Wait, how could you implement a RENAME if you're not locking it when changing the name? By: Kevin P. Fleming (kpfleming) 2005-01-03 22:42:55.000-0600 My plan is to lock the container (or a portion of the container, in the case of a hashtable-based container), then do the rename. This means there is a small possibility of a user reading a partially-changed name, but since the names will change very infrequently that's probably something we can live with. By: Mark Spencer (markster) 2005-01-03 23:10:00.000-0600 I still think at least having a readlock is much better than no lock.  Saying something would potentially fail "infrequently" doesn't give me warm fuzzies. By: Kevin P. Fleming (kpfleming) 2005-01-03 23:23:46.000-0600 Well, let's reserve judgement on that until the actual implementation is available for review, if you don't mind :-) I still don't have everything quite working the way I wish it to, and I suspect it will be a few more days before it's even ready to post as an experimental change. By: Kevin P. Fleming (kpfleming) 2005-01-04 10:00:51.000-0600 Actually, with more time to think about it, I realize that ASTOBJ_RENAME cannot work if the object is currently contained in any containers, because those could be hash-based containers using the object's name, and we have no way to find out from any given object what containers it is linked into. So, that means that ASTOBJ_RENAME will only work if the object is not currently linked into any containers at all, and that removes the locking issue entirely. To rename an object (or to change any other field in the object used to index it in a container), you'll need to UNLINK it from any containers that are using that field to index it, then RENAME, then LINK it again. By: Mark Spencer (markster) 2005-01-04 10:38:59.000-0600 Okay, as soon as we find out why this removes the deadlock, i'm ready to merge it, but we have to diagnose the deadlock first! By: Kevin P. Fleming (kpfleming) 2005-01-04 11:20:37.000-0600 OK, I'm running on my test box with this patch reverted now, so I can try to duplicate the original problem. Anyone else who can do so please try to help out... if you can recreate the deadlock, then we need a gdb "thread apply all bt full" from the running Asterisk process. By: Mark Spencer (markster) 2005-01-07 00:09:40.000-0600 Added to CVS, thanks! By: Digium Subversion (svnbot) 2008-01-15 15:20:21.000-0600 Repository: asterisk Revision: 4700 U   trunk/include/asterisk/astobj.h ------------------------------------------------------------------------ r4700 | markster | 2008-01-15 15:20:21 -0600 (Tue, 15 Jan 2008) | 2 lines Include lock performance (bug ASTERISK-3171) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=4700