[Asterisk-code-review] lock: Reduce storage related to DEBUG THREADS. (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/10293 )

Change subject: lock: Reduce storage related to DEBUG_THREADS.
......................................................................


Patch Set 1: Code-Review-1

(1 comment)

https://gerrit.asterisk.org/#/c/10293/1/include/asterisk/lock.h
File include/asterisk/lock.h:

https://gerrit.asterisk.org/#/c/10293/1/include/asterisk/lock.h@130
PS1, Line 130: struct ast_mutex_info {
             : 	pthread_mutex_t mutex;
             : 	/*! Track which thread holds this mutex */
             : 	struct ast_lock_track *tracker;
             : };
I'm not in favor of this patch as it just about precludes the most needed improvement to DEBUG_THREADS:  It needs to be made much faster.  It currently competes with valgrind for sluggishness. :)

I think that a patch to improve the performance can be done by declaring the structs something like this:

I am pretty sure this does not break ABI.

struct ast_lock_tracking_flags {
	/*! non-zero if lock tracking is enabled */
	unsigned int tracking:1;
	/*! non-zero if track is setup */
	unsigned int setup:1;
};

struct ast_mutex_info {
	pthread_mutex_t mutex;
	/*! Track which thread holds this mutex */
	struct ast_lock_track *track;
	struct ast_lock_tracking_flags flags;
};

struct ast_rwlock_info {
	pthread_rwlock_t lock;
	/*! Track which thread holds this lock */
	struct ast_lock_track *track;
	struct ast_lock_tracking_flags flags;
};

Then in ast_get_reentrancy():

We take the advise of the comment and do a double-check.

We can test setup for non-zero and immediately return the track pointer.  If it is zero then lock the reentrancy lock and check setup again.  If it is non-zero then unlock and return track.  Otherwise, we are the first to setup the lock tracking struct.  We then proceed to setup the track pointer and then set the setup flag.  We have to use this order to prevent an incomplete track pointer set from being used since setting a pointer is not atomic.

Obviously the parameters to ast_get_reentrancy() would need alteration to provide the needed access for mutex and rwlocks. :)



-- 
To view, visit https://gerrit.asterisk.org/10293
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iabd650908901843e9fff47ef1c539f0e1b8cb13b
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 1
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2 (1000185)
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 19:04:51 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180927/23c69796/attachment.html>