From 681deba94ecf3d65b7f4bd9bda3c8a10c4b4c59b Mon Sep 17 00:00:00 2001 From: Ivan Maidanski Date: Thu, 7 Dec 2017 11:14:46 +0300 Subject: [PATCH] Minimize use of AO_ATTR_NO_SANITIZE_THREAD in atomic_ops_malloc/stack (Cherry-pick commits c058d9d, 110b0dc, 3aefd4e, 441415c from 'master'.) * src/atomic_ops_malloc.c (AO_malloc, AO_free): Remove TSan workaround (do not check AO_THREAD_SANITIZER macro). * src/atomic_ops_stack.c (store_before_cas): New static function (or defined as a macro). * src/atomic_ops_stack.c [!USE_ALMOST_LOCK_FREE] (load_before_cas): Likewiese. * src/atomic_ops_stack.c [AO_USE_ALMOST_LOCK_FREE] (AO_stack_push_explicit_aux_release): Remove AO_ATTR_NO_SANITIZE_THREAD. * src/atomic_ops_stack.c [!USE_ALMOST_LOCK_FREE && AO_HAVE_compare_double_and_swap_double] (AO_stack_push_release, AO_stack_pop_acquire): Likewise. * src/atomic_ops_stack.c [AO_USE_ALMOST_LOCK_FREE && AO_THREAD_SANITIZER] (AO_load_next): New static function (with AO_ATTR_NO_SANITIZE_THREAD attribute); add comments. * src/atomic_ops_stack.c [AO_USE_ALMOST_LOCK_FREE && !AO_THREAD_SANITIZER] (AO_load_next): Define to AO_load. * src/atomic_ops_stack.c [AO_USE_ALMOST_LOCK_FREE] (AO_stack_pop_explicit_aux_acquire): Replace AO_load(first_ptr) with AO_load_next(first_ptr). * src/atomic_ops_stack.c [AO_USE_ALMOST_LOCK_FREE] (AO_stack_push_explicit_aux_release): Replace *x=next with store_before_cas(x,next). * src/atomic_ops_stack.c [!USE_ALMOST_LOCK_FREE] (AO_stack_push_release): Likewise. * src/atomic_ops_stack.c [!USE_ALMOST_LOCK_FREE] (AO_stack_pop_acquire): Replace next=*cptr with load_before_cas((AO_t*)cptr). * src/atomic_ops_stack.c [!USE_ALMOST_LOCK_FREE && AO_HAVE_compare_and_swap_double] (AO_stack_pop_acquire): Reformat code. --- src/atomic_ops_malloc.c | 12 ++----- src/atomic_ops_stack.c | 73 +++++++++++++++++++++++++++++++++-------- 2 files changed, 61 insertions(+), 24 deletions(-) diff --git a/src/atomic_ops_malloc.c b/src/atomic_ops_malloc.c index acb00a0..f86585d 100644 --- a/src/atomic_ops_malloc.c +++ b/src/atomic_ops_malloc.c @@ -336,11 +336,7 @@ AO_malloc(size_t sz) add_chunk_as(chunk, log_sz); result = AO_stack_pop(AO_free_list+log_sz); } -# ifdef AO_THREAD_SANITIZER - AO_store(result, log_sz); -# else - *result = log_sz; -# endif + *result = log_sz; # ifdef AO_TRACE_MALLOC fprintf(stderr, "%p: AO_malloc(%lu) = %p\n", (void *)pthread_self(), (unsigned long)sz, (void *)(result + 1)); @@ -359,11 +355,7 @@ AO_free(void *p) return; base = (AO_t *)p - 1; -# ifdef AO_THREAD_SANITIZER - log_sz = (int)AO_load(base); -# else - log_sz = (int)(*base); -# endif + log_sz = (int)(*base); # ifdef AO_TRACE_MALLOC fprintf(stderr, "%p: AO_free(%p sz:%lu)\n", (void *)pthread_self(), p, log_sz > LOG_MAX_SIZE ? (unsigned)log_sz : 1UL << log_sz); diff --git a/src/atomic_ops_stack.c b/src/atomic_ops_stack.c index c364b67..11d385e 100644 --- a/src/atomic_ops_stack.c +++ b/src/atomic_ops_stack.c @@ -22,6 +22,18 @@ #define AO_REQUIRE_CAS #include "atomic_ops_stack.h" +/* This function call must be a part of a do-while loop with a CAS */ +/* designating the condition of the loop (see the use cases below). */ +#ifdef AO_THREAD_SANITIZER + AO_ATTR_NO_SANITIZE_THREAD + static void store_before_cas(AO_t *addr, AO_t value) + { + *addr = value; + } +#else +# define store_before_cas(addr, value) (void)(*(addr) = (value)) +#endif + #ifdef AO_USE_ALMOST_LOCK_FREE void AO_pause(int); /* defined in atomic_ops.c */ @@ -46,7 +58,6 @@ /* to be inserted. */ /* Both list headers and link fields contain "perturbed" pointers, i.e. */ /* pointers with extra bits "or"ed into the low order bits. */ -AO_ATTR_NO_SANITIZE_THREAD void AO_stack_push_explicit_aux_release(volatile AO_t *list, AO_t *x, AO_stack_aux *a) { @@ -94,7 +105,7 @@ void AO_stack_push_explicit_aux_release(volatile AO_t *list, AO_t *x, do { next = AO_load(list); - *x = next; /* data race is OK here */ + store_before_cas(x, next); } while (AO_EXPECT_FALSE(!AO_compare_and_swap_release(list, next, x_bits))); } @@ -115,6 +126,21 @@ void AO_stack_push_explicit_aux_release(volatile AO_t *list, AO_t *x, # define PRECHECK(a) #endif +/* This function is used before CAS in the below AO_stack_pop() and the */ +/* data race (reported by TSan) is OK because it results in a retry. */ +#ifdef AO_THREAD_SANITIZER + AO_ATTR_NO_SANITIZE_THREAD + static AO_t AO_load_next(volatile AO_t *first_ptr) + { + /* Assuming an architecture on which loads of word type are atomic. */ + /* AO_load cannot be used here because it cannot be instructed to */ + /* suppress the warning about the race. */ + return *first_ptr; + } +#else +# define AO_load_next AO_load +#endif + AO_t * AO_stack_pop_explicit_aux_acquire(volatile AO_t *list, AO_stack_aux * a) { @@ -169,7 +195,7 @@ AO_stack_pop_explicit_aux_acquire(volatile AO_t *list, AO_stack_aux * a) goto retry; } first_ptr = AO_REAL_NEXT_PTR(first); - next = AO_load(first_ptr); + next = AO_load_next(first_ptr); # if defined(__alpha__) && (__GNUC__ == 4) if (!AO_compare_and_swap_release(list, first, next)) # else @@ -197,6 +223,25 @@ AO_stack_pop_explicit_aux_acquire(volatile AO_t *list, AO_stack_aux * a) #else /* ! USE_ALMOST_LOCK_FREE */ +/* The functionality is the same as of AO_load_next but the atomicity */ +/* is not needed. The usage is similar to that of store_before_cas. */ +#ifdef AO_THREAD_SANITIZER + /* TODO: If compiled by Clang (as of clang-4.0) with -O3 flag, */ + /* no_sanitize attribute is ignored unless the argument is volatile. */ +# if defined(__clang__) +# define LOAD_BEFORE_CAS_VOLATILE volatile +# else +# define LOAD_BEFORE_CAS_VOLATILE /* empty */ +# endif + AO_ATTR_NO_SANITIZE_THREAD + static AO_t load_before_cas(const LOAD_BEFORE_CAS_VOLATILE AO_t *addr) + { + return *addr; + } +#else +# define load_before_cas(addr) (*(addr)) +#endif + /* Better names for fields in AO_stack_t */ #define ptr AO_val2 #define version AO_val1 @@ -209,14 +254,13 @@ AO_stack_pop_explicit_aux_acquire(volatile AO_t *list, AO_stack_aux * a) volatile /* non-static */ AO_t AO_noop_sink; #endif -AO_ATTR_NO_SANITIZE_THREAD void AO_stack_push_release(AO_stack_t *list, AO_t *element) { AO_t next; do { next = AO_load(&(list -> ptr)); - *element = next; /* data race is OK here */ + store_before_cas(element, next); } while (AO_EXPECT_FALSE(!AO_compare_and_swap_release(&(list -> ptr), next, (AO_t)element))); /* This uses a narrow CAS here, an old optimization suggested */ @@ -229,7 +273,6 @@ void AO_stack_push_release(AO_stack_t *list, AO_t *element) # endif } -AO_ATTR_NO_SANITIZE_THREAD AO_t *AO_stack_pop_acquire(AO_stack_t *list) { # if defined(__clang__) && !AO_CLANG_PREREQ(3, 5) @@ -246,8 +289,9 @@ AO_t *AO_stack_pop_acquire(AO_stack_t *list) /* Version must be loaded first. */ cversion = AO_load_acquire(&(list -> version)); cptr = (AO_t *)AO_load(&(list -> ptr)); - if (cptr == 0) return 0; - next = *cptr; /* data race is OK here */ + if (NULL == cptr) + return NULL; + next = load_before_cas((AO_t *)cptr); } while (AO_EXPECT_FALSE(!AO_compare_double_and_swap_double_release(list, cversion, (AO_t)cptr, cversion+1, (AO_t)next))); @@ -273,7 +317,7 @@ void AO_stack_push_release(AO_stack_t *list, AO_t *element) /* Again version must be loaded first, for different reason. */ version = AO_load_acquire(&(list -> version)); next_ptr = AO_load(&(list -> ptr)); - *element = next_ptr; + store_before_cas(element, next_ptr); } while (!AO_compare_and_swap_double_release( list, version, version+1, (AO_t) element)); @@ -288,14 +332,15 @@ AO_t *AO_stack_pop_acquire(AO_stack_t *list) do { cversion = AO_load_acquire(&(list -> version)); cptr = (AO_t *)AO_load(&(list -> ptr)); - if (cptr == 0) return 0; - next = *cptr; - } while (!AO_compare_double_and_swap_double_release - (list, cversion, (AO_t) cptr, cversion+1, next)); + if (NULL == cptr) + return NULL; + next = load_before_cas(cptr); + } while (!AO_compare_double_and_swap_double_release(list, + cversion, (AO_t)cptr, + cversion+1, next)); return cptr; } - #endif /* AO_HAVE_compare_and_swap_double */ #endif /* ! USE_ALMOST_LOCK_FREE */ -- 2.40.0