Summary:	ASTERISK-03106: [PATCH] change ASTOBJ APIs
Reporter:	Kevin P. Fleming (kpfleming)	Labels:
Date Opened:	2004-12-27 16:32:08.000-0600	Date Closed:	2008-01-15 15:18:33.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) astobj_api_change_rev1.diff.txt ( 1) astobj_api_change_rev2.diff.txt ( 2) astobj_api_change_rev3.diff.txt ( 3) astobj_api_change_rev4.diff.txt
Description:	Mark, here is the patch based on our IRC discussion this morning. It does the following (and I did sneak in some other API changes, hopefully you will agree): 1) All ASTOBJ locks are now managed via macros, rather than direct ast_mutex_lock/unlock calls. The macros are named to support reader and writer locks, but currently use mutexes (just as the rest of Asterisk does). This paves the way for future upgrade to rwlocks, if it can be done properly. 2) ASTOBJ_REF is now a "function-like" macro, that returns either a pointer to the object or NULL, depending on whether it was able to increment the reference count or not. Yes, it is possible for ast_mutex_lock to fail; in that case, ASTOBJ_REF returns NULL. This way, your pointer variable to an ASTOBJ is never populated until you _know_ you have a valid reference to it. 3) ASTOBJ_UNREF has been optimized, and sends a hint to the compiler to help branch prediction. 4) ASTOBJ_MARK/UNMARK now lock the object, as the operation they are performing is not atomic. 5) ASTOBJ_DESTROY has been optimized, and no longer takes the object's lock unless the refcount is greater than zero and it needs to set the DELME flag. Accessing the object's refcount is an atomic opertion on all modern platforms, so it does not need to be lock protected here. 6) ASTOBJ_INIT now sets the object's refcount to 1 (the caller has a reference). Many) ASTOBJ_CONTAINER macros no longer have "iterator" and related arguments; instead, they declare them in their internal blocks when needed. 7) ASTOBJ_CONTAINER_TRAVERSE no longer locks child objects before passing them to the "eval" block; if the object needs to be locked, the "eval" block must do it. In addition, the traversal is now safe against free'ing of the child object, or LINK'ing it to another list. (For those who are curious, this extra safety produces _zero_ additional object code at -O2 or above). 8) ASTOBJ_CONTAINER_FIND_FULL is now a function-like macro, and returns a REF'd pointer to the found child, or NULL. 9) ASTOBJ_CONTAINER_DESTROYALL got some optimizations. 10) ASTOBJ_CONTAINER_UNLINK_FULL and FIND_UNLINK_FULL merged into a single macro, and significantly optimized. 11) ASTOBJ_CONTAINER_PRUNE_MARKED got some optimizations. 12) ASTOBJ_CONTAINER_LINK_FULL no longer REF's the incoming object; the container's reference to the object is uncounted (the only counted references are those held by the users of the macros, not the macros themselves). 13) ASTOBJ_CONTAINER_RELEASE renamed to ASTOBJ_CONTAINER_DESTROY for consistency. With these changes, doing TRAVERSE operations requires only a single lock (as it did before the ASTOBJ conversion). As part of the patch, chan_sip has been updated to take into account all of these API changes, including additional explicit locking in TRAVERSE operations. ****** ADDITIONAL INFORMATION ****** Disclaimer is on file.
Comments:	By: Kevin P. Fleming (kpfleming) 2004-12-27 16:42:02.000-0600 rev2 has some fixes: 1) ASTOBJ_CONTAINER_FIND_UNLINK_FULL did not REF the returned object; it does now 2) chan_sip's build_peer function did some funny stuff with the refcount; the above change eliminated the need for any funniness 3) Fixed a compiler warning in do_monitor in chan_sip By: Mark Spencer (markster) 2004-12-28 03:06:43.000-0600 Kevin, just a few things: Why was the lock field renamed to _lock? Why is there an if in ASTOBJ_REF, shouldn't the write lock always succeed?  Why do we have a local variable ref rather than simply calling (object); at the end? Does the builtin_expect usage predict the refcount to be exactly 1, or just that the first argument is the condition and the second is whether the condition is expected to be true or false? Thanks! By: Mark Spencer (markster) 2004-12-28 03:08:19.000-0600 Also, do you think that AST_UNREF(a) should set a=NULL; at the end?  In principle there would never be any situation in which we would access the pointer after AST_UNREF(a). By: Kevin P. Fleming (kpfleming) 2004-12-28 08:38:51.000-0600 I renamed the lock field to keep users from using ast_mutex_lock directly on ASTOBJ instances. No, the write lock might not always succeed. For example, it's remotely possible that the object will have been destroyed in between the time you are passed a pointer to it and the time you try to take the lock. The reason for "ref" instead of calling "(object)" was to be sure I could return NULL in the case of lock failure; there has to be _some_ local variable within the macro to remember success or failure and return NULL or (object); I figured it was easiest to just copy object into another variable. Your second thought about builtin_expect is on target; it returns the value of the first expression, and the second expression is the "likely" value of the first one. Since it is being used for a boolean condition, anything other than zero is treated identically. I thought about making UNREF set the object pointer to NULL, and that's probably not a bad idea at all. I'll post a new rev with that change. By: Kevin P. Fleming (kpfleming) 2004-12-28 09:24:46.000-0600 rev3, two changes: 1) ASTOBJ_UNREF now sets the object pointer to NULL 2) an additional set of locking macros for 'object as container' vs. 'object as object'... if we ever change to a new locking model for the list itself (vs. the object's fields), this will allow an easy split Once we get agreement that these changes are going to go in, I'll write up doxygen docs for all this stuff and try to brain-dump all the bits and pieces we've talked about for these macros :-) By: Mark Spencer (markster) 2004-12-28 11:45:42.000-0600 ASTOBJ_WRLOCK is simply a pointer to ast_mutex_lock which cannot return anything but success (it blocks on a failure to obtain the lock obviously), therefore ASTOBJ_WRLOCK can never fail.   Further, if you are ASTOBJ_REF'ing an object, it cannot by definition be in a position which would allow it to be deleted -- and if it were deleted, the lock would be being called on bad memory. I believe, therefore, that AST_WRLOCK can never fail, and if there were a condition in which it could fail, that condition must be fixed, not the WRLOCK checked for whether it succeeded or failed. Other than that, I think we're basically there.  Lets get the change in first, then open a separate one for the documentation. By: Kevin P. Fleming (kpfleming) 2004-12-28 11:49:38.000-0600 Well, on Linux at least, ast_mutex_lock calls pthread_mutex_lock. According to the man page for pthread_mutex_lock, it can fail. However, you are correct, it is only under very bizarre circumstances that it could occur. I will remove the lock-failure checking for now; we can always add it later if we find a need. By: Mark Spencer (markster) 2004-12-28 18:31:45.000-0600 Added to CVS, thank you for your contribution! By: Digium Subversion (svnbot) 2008-01-15 15:18:33.000-0600 Repository: asterisk Revision: 4578 U   trunk/channels/chan_sip.c U   trunk/include/asterisk/astobj.h ------------------------------------------------------------------------ r4578 | markster | 2008-01-15 15:18:33 -0600 (Tue, 15 Jan 2008) | 2 lines Merge kpflemings ASTOBJ improvements (bug ASTERISK-3106) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=4578