[Asterisk-code-review] lock: Improve performance of DEBUG THREADS. (asterisk[15])
Joshua Colp
asteriskteam at digium.com
Mon Oct 1 08:32:27 CDT 2018
Joshua Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/10317 )
Change subject: lock: Improve performance of DEBUG_THREADS.
......................................................................
lock: Improve performance of DEBUG_THREADS.
Add a volatile flag to lock tracking structures so we only need to use
the global lock when first initializing tracking.
Additionally add support for DEBUG_THREADS_LOOSE_ABI. This is used by
astobj2.c to eliminate storage for tracking fields when DEBUG_THREADS is
not defined.
Change-Id: Iabd650908901843e9fff47ef1c539f0e1b8cb13b
---
M include/asterisk/astobj2.h
M include/asterisk/lock.h
M main/astobj2.c
M main/lock.c
4 files changed, 81 insertions(+), 90 deletions(-)
Approvals:
Richard Mudgett: Looks good to me, but someone else must approve
George Joseph: Looks good to me, approved
Joshua Colp: Approved for Submit
diff --git a/include/asterisk/astobj2.h b/include/asterisk/astobj2.h
index 0e442db..8e3a105 100644
--- a/include/asterisk/astobj2.h
+++ b/include/asterisk/astobj2.h
@@ -752,6 +752,9 @@
* lock address, this allows you to correlate against
* object address, to match objects to reported locks.
*
+ * \warning AO2 lock objects do not include tracking fields when
+ * DEBUG_THREADS is not enabled.
+ *
* \since 1.6.1
*/
void *ao2_object_get_lockaddr(void *obj);
diff --git a/include/asterisk/lock.h b/include/asterisk/lock.h
index 5b6817f..28091f9 100644
--- a/include/asterisk/lock.h
+++ b/include/asterisk/lock.h
@@ -92,11 +92,11 @@
#define AST_LOCK_TRACK_INIT_VALUE { { NULL }, { 0 }, 0, { NULL }, { 0 }, PTHREAD_MUTEX_INIT_VALUE }
#endif
-#define AST_MUTEX_INIT_VALUE { PTHREAD_MUTEX_INIT_VALUE, NULL, 1 }
-#define AST_MUTEX_INIT_VALUE_NOTRACKING { PTHREAD_MUTEX_INIT_VALUE, NULL, 0 }
+#define AST_MUTEX_INIT_VALUE { PTHREAD_MUTEX_INIT_VALUE, NULL, {1, 0} }
+#define AST_MUTEX_INIT_VALUE_NOTRACKING { PTHREAD_MUTEX_INIT_VALUE, NULL, {0, 0} }
-#define AST_RWLOCK_INIT_VALUE { __AST_RWLOCK_INIT_VALUE, NULL, 1 }
-#define AST_RWLOCK_INIT_VALUE_NOTRACKING { __AST_RWLOCK_INIT_VALUE, NULL, 0 }
+#define AST_RWLOCK_INIT_VALUE { __AST_RWLOCK_INIT_VALUE, NULL, {1, 0} }
+#define AST_RWLOCK_INIT_VALUE_NOTRACKING { __AST_RWLOCK_INIT_VALUE, NULL, {0, 0} }
#define AST_MAX_REENTRANCY 10
@@ -120,6 +120,13 @@
pthread_mutex_t reentr_mutex;
};
+struct ast_lock_track_flags {
+ /*! non-zero if lock tracking is enabled */
+ unsigned int tracking:1;
+ /*! non-zero if track is setup */
+ volatile unsigned int setup:1;
+};
+
/*! \brief Structure for mutex and tracking information.
*
* We have tracking information in this structure regardless of DEBUG_THREADS being enabled.
@@ -127,9 +134,18 @@
*/
struct ast_mutex_info {
pthread_mutex_t mutex;
- /*! Track which thread holds this mutex */
+#if !defined(DEBUG_THREADS) && !defined(DEBUG_THREADS_LOOSE_ABI)
+ /*!
+ * These fields are renamed to ensure they are never used when
+ * DEBUG_THREADS is not defined.
+ */
+ struct ast_lock_track *_track;
+ struct ast_lock_track_flags _flags;
+#elif defined(DEBUG_THREADS)
+ /*! Track which thread holds this mutex. */
struct ast_lock_track *track;
- unsigned int tracking:1;
+ struct ast_lock_track_flags flags;
+#endif
};
/*! \brief Structure for rwlock and tracking information.
@@ -139,9 +155,18 @@
*/
struct ast_rwlock_info {
pthread_rwlock_t lock;
+#if !defined(DEBUG_THREADS) && !defined(DEBUG_THREADS_LOOSE_ABI)
+ /*!
+ * These fields are renamed to ensure they are never used when
+ * DEBUG_THREADS is not defined.
+ */
+ struct ast_lock_track *_track;
+ struct ast_lock_track_flags _flags;
+#elif defined(DEBUG_THREADS)
/*! Track which thread holds this lock */
struct ast_lock_track *track;
- unsigned int tracking:1;
+ struct ast_lock_track_flags flags;
+#endif
};
typedef struct ast_mutex_info ast_mutex_t;
diff --git a/main/astobj2.c b/main/astobj2.c
index 91ebc1e..d9d8a0c 100644
--- a/main/astobj2.c
+++ b/main/astobj2.c
@@ -25,6 +25,10 @@
<support_level>core</support_level>
***/
+/* This reduces the size of lock structures within astobj2 objects when
+ * DEBUG_THREADS is not defined. */
+#define DEBUG_THREADS_LOOSE_ABI
+
#include "asterisk.h"
#include "asterisk/_private.h"
diff --git a/main/lock.c b/main/lock.c
index 9c1d383..77ca0ea 100644
--- a/main/lock.c
+++ b/main/lock.c
@@ -72,21 +72,16 @@
#ifdef DEBUG_THREADS
AST_MUTEX_DEFINE_STATIC(reentrancy_lock);
-static inline struct ast_lock_track *ast_get_reentrancy(struct ast_lock_track **plt)
+static inline struct ast_lock_track *ast_get_reentrancy(struct ast_lock_track **plt,
+ struct ast_lock_track_flags *flags, int no_setup)
{
pthread_mutexattr_t reentr_attr;
struct ast_lock_track *lt;
- /* It's a bit painful to lock a global mutex for every access to the
- * reentrancy structure, but it's necessary to ensure that we don't
- * double-allocate the structure or double-initialize the reentr_mutex.
- *
- * If you'd like to replace this with a double-checked lock, be sure to
- * properly volatile-ize everything to avoid optimizer bugs.
- *
- * We also have to use the underlying pthread calls for manipulating
- * the mutex, because this is called from the Asterisk mutex code.
- */
+ if (!flags->tracking || flags->setup) {
+ return *plt;
+ }
+
pthread_mutex_lock(&reentrancy_lock.mutex);
if (*plt) {
@@ -94,6 +89,11 @@
return *plt;
}
+ if (no_setup) {
+ pthread_mutex_unlock(&reentrancy_lock.mutex);
+ return NULL;
+ }
+
lt = *plt = ast_std_calloc(1, sizeof(*lt));
if (!lt) {
fprintf(stderr, "%s: Failed to allocate lock tracking\n", __func__);
@@ -110,6 +110,7 @@
pthread_mutex_init(<->reentr_mutex, &reentr_attr);
pthread_mutexattr_destroy(&reentr_attr);
+ flags->setup = 1;
pthread_mutex_unlock(&reentrancy_lock.mutex);
return lt;
}
@@ -148,7 +149,8 @@
#endif /* AST_MUTEX_INIT_W_CONSTRUCTORS */
t->track = NULL;
- t->tracking = tracking;
+ t->flags.tracking = tracking;
+ t->flags.setup = 0;
#endif /* DEBUG_THREADS */
pthread_mutexattr_init(&attr);
@@ -165,8 +167,8 @@
int res;
#ifdef DEBUG_THREADS
- struct ast_lock_track *lt = t->track;
- int canlog = t->tracking && strcmp(filename, "logger.c");
+ struct ast_lock_track *lt = ast_get_reentrancy(&t->track, &t->flags, 1);
+ int canlog = t->flags.tracking && strcmp(filename, "logger.c");
#if defined(AST_MUTEX_INIT_W_CONSTRUCTORS) && defined(CAN_COMPARE_MUTEX_TO_INIT_VALUE)
if ((t->mutex) == ((pthread_mutex_t) PTHREAD_MUTEX_INITIALIZER)) {
@@ -243,16 +245,12 @@
int res;
#ifdef DEBUG_THREADS
- struct ast_lock_track *lt = NULL;
- int canlog = t->tracking && strcmp(filename, "logger.c");
+ struct ast_lock_track *lt = ast_get_reentrancy(&t->track, &t->flags, 0);
+ int canlog = t->flags.tracking && strcmp(filename, "logger.c");
#ifdef HAVE_BKTR
struct ast_bt *bt = NULL;
#endif
- if (t->tracking) {
- lt = ast_get_reentrancy(&t->track);
- }
-
if (lt) {
#ifdef HAVE_BKTR
struct ast_bt tmp;
@@ -367,16 +365,12 @@
int res;
#ifdef DEBUG_THREADS
- struct ast_lock_track *lt = NULL;
- int canlog = t->tracking && strcmp(filename, "logger.c");
+ struct ast_lock_track *lt = ast_get_reentrancy(&t->track, &t->flags, 0);
+ int canlog = t->flags.tracking && strcmp(filename, "logger.c");
#ifdef HAVE_BKTR
struct ast_bt *bt = NULL;
#endif
- if (t->tracking) {
- lt = ast_get_reentrancy(&t->track);
- }
-
if (lt) {
#ifdef HAVE_BKTR
struct ast_bt tmp;
@@ -432,7 +426,7 @@
#ifdef DEBUG_THREADS
struct ast_lock_track *lt = NULL;
- int canlog = t->tracking && strcmp(filename, "logger.c");
+ int canlog = t->flags.tracking && strcmp(filename, "logger.c");
#ifdef HAVE_BKTR
struct ast_bt *bt = NULL;
#endif
@@ -446,10 +440,7 @@
}
#endif /* AST_MUTEX_INIT_W_CONSTRUCTORS */
- if (t->tracking) {
- lt = ast_get_reentrancy(&t->track);
- }
-
+ lt = ast_get_reentrancy(&t->track, &t->flags, 0);
if (lt) {
ast_reentrancy_lock(lt);
if (lt->reentrancy && (lt->thread_id[ROFFSET] != pthread_self())) {
@@ -561,7 +552,7 @@
#ifdef DEBUG_THREADS
struct ast_lock_track *lt = NULL;
struct ast_lock_track lt_orig;
- int canlog = t->tracking && strcmp(filename, "logger.c");
+ int canlog = t->flags.tracking && strcmp(filename, "logger.c");
#if defined(AST_MUTEX_INIT_W_CONSTRUCTORS) && defined(CAN_COMPARE_MUTEX_TO_INIT_VALUE)
if ((t->mutex) == ((pthread_mutex_t) PTHREAD_MUTEX_INITIALIZER)) {
@@ -572,10 +563,7 @@
}
#endif /* AST_MUTEX_INIT_W_CONSTRUCTORS */
- if (t->tracking) {
- lt = ast_get_reentrancy(&t->track);
- }
-
+ lt = ast_get_reentrancy(&t->track, &t->flags, 0);
if (lt) {
ast_reentrancy_lock(lt);
if (lt->reentrancy && (lt->thread_id[ROFFSET] != pthread_self())) {
@@ -629,7 +617,7 @@
#ifdef DEBUG_THREADS
struct ast_lock_track *lt = NULL;
struct ast_lock_track lt_orig;
- int canlog = t->tracking && strcmp(filename, "logger.c");
+ int canlog = t->flags.tracking && strcmp(filename, "logger.c");
#if defined(AST_MUTEX_INIT_W_CONSTRUCTORS) && defined(CAN_COMPARE_MUTEX_TO_INIT_VALUE)
if ((t->mutex) == ((pthread_mutex_t) PTHREAD_MUTEX_INITIALIZER)) {
@@ -640,10 +628,7 @@
}
#endif /* AST_MUTEX_INIT_W_CONSTRUCTORS */
- if (t->tracking) {
- lt = ast_get_reentrancy(&t->track);
- }
-
+ lt = ast_get_reentrancy(&t->track, &t->flags, 0);
if (lt) {
ast_reentrancy_lock(lt);
if (lt->reentrancy && (lt->thread_id[ROFFSET] != pthread_self())) {
@@ -706,7 +691,8 @@
#endif /* AST_MUTEX_INIT_W_CONSTRUCTORS */
t->track = NULL;
- t->tracking = tracking;
+ t->flags.tracking = tracking;
+ t->flags.setup = 0;
#endif /* DEBUG_THREADS */
pthread_rwlockattr_init(&attr);
@@ -724,8 +710,8 @@
int res;
#ifdef DEBUG_THREADS
- struct ast_lock_track *lt = t->track;
- int canlog = t->tracking && strcmp(filename, "logger.c");
+ struct ast_lock_track *lt = ast_get_reentrancy(&t->track, &t->flags, 1);
+ int canlog = t->flags.tracking && strcmp(filename, "logger.c");
#if defined(AST_MUTEX_INIT_W_CONSTRUCTORS) && defined(CAN_COMPARE_MUTEX_TO_INIT_VALUE)
if (t->lock == ((pthread_rwlock_t) __AST_RWLOCK_INIT_VALUE)) {
@@ -772,7 +758,7 @@
#ifdef DEBUG_THREADS
struct ast_lock_track *lt = NULL;
- int canlog = t->tracking && strcmp(filename, "logger.c");
+ int canlog = t->flags.tracking && strcmp(filename, "logger.c");
#ifdef HAVE_BKTR
struct ast_bt *bt = NULL;
#endif
@@ -787,10 +773,7 @@
}
#endif /* AST_MUTEX_INIT_W_CONSTRUCTORS */
- if (t->tracking) {
- lt = ast_get_reentrancy(&t->track);
- }
-
+ lt = ast_get_reentrancy(&t->track, &t->flags, 0);
if (lt) {
ast_reentrancy_lock(lt);
if (lt->reentrancy) {
@@ -851,16 +834,12 @@
int res;
#ifdef DEBUG_THREADS
- struct ast_lock_track *lt = NULL;
- int canlog = t->tracking && strcmp(filename, "logger.c");
+ struct ast_lock_track *lt = ast_get_reentrancy(&t->track, &t->flags, 0);
+ int canlog = t->flags.tracking && strcmp(filename, "logger.c");
#ifdef HAVE_BKTR
struct ast_bt *bt = NULL;
#endif
- if (t->tracking) {
- lt = ast_get_reentrancy(&t->track);
- }
-
if (lt) {
#ifdef HAVE_BKTR
struct ast_bt tmp;
@@ -960,16 +939,12 @@
int res;
#ifdef DEBUG_THREADS
- struct ast_lock_track *lt = NULL;
- int canlog = t->tracking && strcmp(filename, "logger.c");
+ struct ast_lock_track *lt = ast_get_reentrancy(&t->track, &t->flags, 0);
+ int canlog = t->flags.tracking && strcmp(filename, "logger.c");
#ifdef HAVE_BKTR
struct ast_bt *bt = NULL;
#endif
- if (t->tracking) {
- lt = ast_get_reentrancy(&t->track);
- }
-
if (lt) {
#ifdef HAVE_BKTR
struct ast_bt tmp;
@@ -1069,16 +1044,12 @@
int res;
#ifdef DEBUG_THREADS
- struct ast_lock_track *lt = NULL;
- int canlog = t->tracking && strcmp(filename, "logger.c");
+ struct ast_lock_track *lt = ast_get_reentrancy(&t->track, &t->flags, 0);
+ int canlog = t->flags.tracking && strcmp(filename, "logger.c");
#ifdef HAVE_BKTR
struct ast_bt *bt = NULL;
#endif
- if (t->tracking) {
- lt = ast_get_reentrancy(&t->track);
- }
-
if (lt) {
#ifdef HAVE_BKTR
struct ast_bt tmp;
@@ -1162,16 +1133,12 @@
int res;
#ifdef DEBUG_THREADS
- struct ast_lock_track *lt = NULL;
- int canlog = t->tracking && strcmp(filename, "logger.c");
+ struct ast_lock_track *lt = ast_get_reentrancy(&t->track, &t->flags, 0);
+ int canlog = t->flags.tracking && strcmp(filename, "logger.c");
#ifdef HAVE_BKTR
struct ast_bt *bt = NULL;
#endif
- if (t->tracking) {
- lt = ast_get_reentrancy(&t->track);
- }
-
if (lt) {
#ifdef HAVE_BKTR
struct ast_bt tmp;
@@ -1254,15 +1221,11 @@
int res;
#ifdef DEBUG_THREADS
- struct ast_lock_track *lt = NULL;
+ struct ast_lock_track *lt = ast_get_reentrancy(&t->track, &t->flags, 0);
#ifdef HAVE_BKTR
struct ast_bt *bt = NULL;
#endif
- if (t->tracking) {
- lt = ast_get_reentrancy(&t->track);
- }
-
if (lt) {
#ifdef HAVE_BKTR
struct ast_bt tmp;
@@ -1313,15 +1276,11 @@
int res;
#ifdef DEBUG_THREADS
- struct ast_lock_track *lt = NULL;
+ struct ast_lock_track *lt = ast_get_reentrancy(&t->track, &t->flags, 0);
#ifdef HAVE_BKTR
struct ast_bt *bt = NULL;
#endif
- if (t->tracking) {
- lt = ast_get_reentrancy(&t->track);
- }
-
if (lt) {
#ifdef HAVE_BKTR
struct ast_bt tmp;
--
To view, visit https://gerrit.asterisk.org/10317
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: merged
Gerrit-Change-Id: Iabd650908901843e9fff47ef1c539f0e1b8cb13b
Gerrit-Change-Number: 10317
Gerrit-PatchSet: 2
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2 (1000185)
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20181001/82891928/attachment-0001.html>
More information about the asterisk-code-review
mailing list