From 82523f19f6c048cffd729752f57656dbee5bbdc9 Mon Sep 17 00:00:00 2001 From: David Smith Date: Fri, 5 Oct 2012 12:52:43 -0500 Subject: [PATCH] (PR14637 partial fix) Improve stapdyn locking. * runtime/dyninst/runtime.h: Remove the 'stapdyn_big_dumb_lock' and make preempt_disable() and preempt_enable_no_resched() do nothing for dyninst. * runtime/dyninst/tls_data.c: Change the mutex in tls_data_container_t to a rwlock. (_stp_tls_free_per_thread_ptr): Write lock the container before remove the object from the list (and reduce the amount of time the container is locked). (_stp_tls_get_per_thread_ptr): Write lock the container before adding the object from the list (and reduce the amount of time the container is locked). * runtime/map.c (_stp_pmap_agg): Be sure to unlock the map in error conditions. * runtime/pmap-gen.c (KEYSYM(_stp_pmap_get)): Be sure to unlock the container in an error condition. --- runtime/dyninst/runtime.h | 14 ++----------- runtime/dyninst/tls_data.c | 42 +++++++++++++++++++++++--------------- runtime/map.c | 8 ++++++-- runtime/pmap-gen.c | 6 +++++- 4 files changed, 39 insertions(+), 31 deletions(-) diff --git a/runtime/dyninst/runtime.h b/runtime/dyninst/runtime.h index 3165f15bf..7108ed135 100644 --- a/runtime/dyninst/runtime.h +++ b/runtime/dyninst/runtime.h @@ -71,18 +71,8 @@ static inline int pseudo_atomic_cmpxchg(atomic_t *v, int oldval, int newval) } -static pthread_mutex_t stapdyn_big_dumb_lock = PTHREAD_MUTEX_INITIALIZER; - -static inline void preempt_disable(void) -{ - pthread_mutex_lock(&stapdyn_big_dumb_lock); -} - -static inline void preempt_enable_no_resched(void) -{ - pthread_mutex_unlock(&stapdyn_big_dumb_lock); -} - +#define preempt_disable() 0 +#define preempt_enable_no_resched() 0 #include "linux_defs.h" diff --git a/runtime/dyninst/tls_data.c b/runtime/dyninst/tls_data.c index 7f3be9dd2..2115d8ff1 100644 --- a/runtime/dyninst/tls_data.c +++ b/runtime/dyninst/tls_data.c @@ -20,7 +20,7 @@ struct tls_data_container_t { pthread_key_t key; /* key indexing TLS objects */ size_t size; /* allocated size of a new TLS object */ struct list_head head; /* list of tls_data_object_t structs */ - pthread_mutex_t lock; /* lock protecting list */ + pthread_rwlock_t lock; /* lock protecting list */ int (*init_function)(struct tls_data_object_t *); void (*free_function)(struct tls_data_object_t *); }; @@ -30,8 +30,19 @@ struct tls_data_object_t { struct tls_data_container_t *container; }; -#define TLS_DATA_CONTAINER_LOCK(con) pthread_mutex_lock(&(con)->lock) -#define TLS_DATA_CONTAINER_UNLOCK(con) pthread_mutex_unlock(&(con)->lock) +#ifdef STP_DEBUG_CONTAINER_LOCK +#define TLS_DATA_CONTAINER_INIT(con) pthread_rwlock_init(&(con)->lock, NULL) +#define TLS_DATA_CONTAINER_LOCK(con) {printf("%s:%d rd locking %p\n", __FUNCTION__, __LINE__, &(con)->lock); pthread_rwlock_rdlock(&(con)->lock);} +#define TLS_DATA_CONTAINER_WRLOCK(con) {printf("%s:%d wr locking %p\n", __FUNCTION__, __LINE__, &(con)->lock); pthread_rwlock_wrlock(&(con)->lock);} +#define TLS_DATA_CONTAINER_UNLOCK(con) {pthread_rwlock_unlock(&(con)->lock); printf("%s:%d unlocked %p\n", __FUNCTION__, __LINE__, &(con)->lock); } +#define TLS_DATA_CONTAINER_DESTROY(con) (void)pthread_rwlock_destroy(&(con)->lock) +#else +#define TLS_DATA_CONTAINER_INIT(con) pthread_rwlock_init(&(con)->lock, NULL) +#define TLS_DATA_CONTAINER_LOCK(con) pthread_rwlock_rdlock(&(con)->lock) +#define TLS_DATA_CONTAINER_WRLOCK(con) pthread_rwlock_wrlock(&(con)->lock) +#define TLS_DATA_CONTAINER_UNLOCK(con) pthread_rwlock_unlock(&(con)->lock) +#define TLS_DATA_CONTAINER_DESTROY(con) (void)pthread_rwlock_destroy(&(con)->lock) +#endif #define for_each_tls_data(obj, container) \ list_for_each_entry((obj), &(container)->head, list) @@ -48,8 +59,9 @@ static void _stp_tls_free_per_thread_ptr(void *addr) /* Remove this object from the container's list of objects */ if (container) { - pthread_mutex_lock(&container->lock); + TLS_DATA_CONTAINER_WRLOCK(container); list_del(&obj->list); + TLS_DATA_CONTAINER_UNLOCK(container); /* Give the code above us a chance to cleanup. */ if (container->free_function) @@ -60,9 +72,6 @@ static void _stp_tls_free_per_thread_ptr(void *addr) * the struct tls_data_object is the first thing in * its containing structure. */ free(obj); - - if (container) - pthread_mutex_unlock(&container->lock); } } @@ -98,15 +107,15 @@ _stp_tls_get_per_thread_ptr(struct tls_data_container_t *container) } /* Inform pthreads about this instance. */ - pthread_mutex_lock(&container->lock); + TLS_DATA_CONTAINER_WRLOCK(container); if ((rc = pthread_setspecific(container->key, obj)) == 0) { /* Add obj to container's list of objs (for * use in looping over all threads). */ list_add(&obj->list, &container->head); + TLS_DATA_CONTAINER_UNLOCK(container); } else { - _stp_error("Couldn't setspecific on tls key: %d\n", - rc); + TLS_DATA_CONTAINER_UNLOCK(container); /* Give the code above us a chance to cleanup. */ if (container->free_function) @@ -114,8 +123,9 @@ _stp_tls_get_per_thread_ptr(struct tls_data_container_t *container) free(obj); obj = NULL; + _stp_error("Couldn't setspecific on tls key: %d\n", + rc); } - pthread_mutex_unlock(&container->lock); } exit: return obj; @@ -139,14 +149,14 @@ _stp_tls_data_container_init(struct tls_data_container_t *container, int rc; INIT_LIST_HEAD(&container->head); - if ((rc = pthread_mutex_init(&container->lock, NULL)) != 0) { - _stp_error("Couldn't init tls mutex: %d\n", rc); + if ((rc = TLS_DATA_CONTAINER_INIT(container)) != 0) { + _stp_error("Couldn't init tls lock: %d\n", rc); return 1; } if ((rc = pthread_key_create(&container->key, &_stp_tls_free_per_thread_ptr)) != 0) { _stp_error("Couldn't create tls key: %d\n", rc); - (void)pthread_mutex_destroy(&container->lock); + TLS_DATA_CONTAINER_DESTROY(container); return 1; } @@ -161,7 +171,7 @@ _stp_tls_data_container_cleanup(struct tls_data_container_t *container) { struct tls_data_object_t *obj, *n; - TLS_DATA_CONTAINER_LOCK(container); + TLS_DATA_CONTAINER_WRLOCK(container); (void) pthread_key_delete(container->key); for_each_tls_data_safe(obj, n, container) { list_del(&obj->list); @@ -173,6 +183,6 @@ _stp_tls_data_container_cleanup(struct tls_data_container_t *container) free(obj); } TLS_DATA_CONTAINER_UNLOCK(container); - (void)pthread_mutex_destroy(&container->lock); + TLS_DATA_CONTAINER_DESTROY(container); } #endif /* _TLS_DATA_C_ */ diff --git a/runtime/map.c b/runtime/map.c index ddd8d279f..b0c7d43be 100644 --- a/runtime/map.c +++ b/runtime/map.c @@ -852,6 +852,7 @@ static MAP _stp_pmap_agg (PMAP pmap) #ifndef __KERNEL__ struct tls_data_object_t *obj; #endif + int quit = 0; agg = &pmap->agg; @@ -886,13 +887,16 @@ static MAP _stp_pmap_agg (PMAP pmap) _stp_add_agg(aptr, ptr); else { if (!_stp_new_agg(agg, ahead, ptr)) { - MAP_UNLOCK(m); - return NULL; + quit = 1; + agg = NULL; + break; } } } } MAP_UNLOCK(m); + if (quit) + break; } #ifndef __KERNEL__ TLS_DATA_CONTAINER_UNLOCK(&pmap->container); diff --git a/runtime/pmap-gen.c b/runtime/pmap-gen.c index 44f0cabb0..d99178055 100644 --- a/runtime/pmap-gen.c +++ b/runtime/pmap-gen.c @@ -1032,8 +1032,12 @@ static VALTYPE KEYSYM(_stp_pmap_get) (PMAP pmap, ALLKEYSD(key)) head = &map->hashes[hv]; #if NEED_MAP_LOCKS - if (!spin_trylock(&map->lock)) + if (!spin_trylock(&map->lock)) { +#ifndef __KERNEL__ + TLS_DATA_CONTAINER_UNLOCK(&pmap->container); +#endif return NULLRET; + } #endif hlist_for_each(e, head) { -- 2.43.5