From 8b53a1459cee7ea56a7eca1ecf717c3b65d89a21 Mon Sep 17 00:00:00 2001 From: Rose <83477269+AtariDreams@users.noreply.github.com> Date: Mon, 25 Dec 2023 12:53:14 -0500 Subject: [PATCH] atomically read page_bitmaps page_bitmaps does not need to be volatile since it is going to be treated as atomic. In fact, as long as each variable is treated as atomic, we do not need volatile at all, as that means it is modified by outside means, aka hardware, etc. The atomic macros already ensure optimizations that could cause incorrect values to be used do not happen. Finally, the first loop is redundant as they are iterated over again in the loop below. --- src/allocator.c | 65 +++++++++++++++++++----------------------- src/shims/atomic_sfb.h | 6 ++-- 2 files changed, 32 insertions(+), 39 deletions(-) diff --git a/src/allocator.c b/src/allocator.c index 66284090f..1dd9d4484 100644 --- a/src/allocator.c +++ b/src/allocator.c @@ -211,7 +211,7 @@ DISPATCH_ALWAYS_INLINE_NDEBUG DISPATCH_CONST static bool bitmap_is_full(bitmap_t bits) { - return (bits == BITMAP_ALL_ONES); + return atomic_load_explicit(bits, memory_order_relaxed) == BITMAP_ALL_ONES); } #define NO_BITS_WERE_UNSET (UINT_MAX) @@ -220,7 +220,7 @@ bitmap_is_full(bitmap_t bits) // allowed to be set. DISPATCH_ALWAYS_INLINE_NDEBUG static unsigned int -bitmap_set_first_unset_bit_upto_index(volatile bitmap_t *bitmap, +bitmap_set_first_unset_bit_upto_index(bitmap_t *bitmap, unsigned int max_index) { // No barriers needed in acquire path: the just-allocated @@ -233,7 +233,7 @@ bitmap_set_first_unset_bit_upto_index(volatile bitmap_t *bitmap, DISPATCH_ALWAYS_INLINE static unsigned int -bitmap_set_first_unset_bit(volatile bitmap_t *bitmap) +bitmap_set_first_unset_bit(bitmap_t *bitmap) { return bitmap_set_first_unset_bit_upto_index(bitmap, UINT_MAX); } @@ -244,7 +244,7 @@ bitmap_set_first_unset_bit(volatile bitmap_t *bitmap) // Return true if this bit was the last in the bitmap, and it is now all zeroes DISPATCH_ALWAYS_INLINE_NDEBUG static bool -bitmap_clear_bit(volatile bitmap_t *bitmap, unsigned int index, +bitmap_clear_bit(bitmap_t *bitmap, unsigned int index, bool exclusively) { #if DISPATCH_DEBUG @@ -267,8 +267,8 @@ bitmap_clear_bit(volatile bitmap_t *bitmap, unsigned int index, DISPATCH_ALWAYS_INLINE_NDEBUG static void -mark_bitmap_as_full_if_still_full(volatile bitmap_t *supermap, - unsigned int bitmap_index, volatile bitmap_t *bitmap) +mark_bitmap_as_full_if_still_full(bitmap_t *supermap, + unsigned int bitmap_index, bitmap_t *bitmap) { #if DISPATCH_DEBUG dispatch_assert(bitmap_index < BITMAPS_PER_SUPERMAP); @@ -321,12 +321,12 @@ alloc_continuation_from_magazine(struct dispatch_magazine_s *magazine) unsigned int s, b, index; for (s = 0; s < SUPERMAPS_PER_MAGAZINE; s++) { - volatile bitmap_t *supermap = supermap_address(magazine, s); + bitmap_t *supermap = supermap_address(magazine, s); if (bitmap_is_full(*supermap)) { continue; } for (b = 0; b < BITMAPS_PER_SUPERMAP; b++) { - volatile bitmap_t *bitmap = bitmap_address(magazine, s, b); + bitmap_t *bitmap = bitmap_address(magazine, s, b); index = bitmap_set_first_unset_bit(bitmap); if (index != NO_BITS_WERE_UNSET) { set_last_found_page( @@ -530,44 +530,37 @@ _dispatch_alloc_maybe_madvise_page(dispatch_continuation_t c) // page can't be madvised; maybe it contains non-continuations return; } - // Are all the continuations in this page unallocated? - volatile bitmap_t *page_bitmaps; + + bitmap_t *page_bitmaps; get_maps_and_indices_for_continuation((dispatch_continuation_t)page, NULL, - NULL, (bitmap_t **)&page_bitmaps, NULL); + NULL, &page_bitmaps, NULL); unsigned int i; + + // Try to take ownership of them all, and if it fails, unlock the ones we locked for (i = 0; i < BITMAPS_PER_PAGE; i++) { - if (page_bitmaps[i] != 0) { - return; - } - } - // They are all unallocated, so we could madvise the page. Try to - // take ownership of them all. - int last_locked = 0; - do { - if (!os_atomic_cmpxchg(&page_bitmaps[last_locked], BITMAP_C(0), + if (!os_atomic_cmpxchg(&page_bitmaps[i], BITMAP_C(0), BITMAP_ALL_ONES, relaxed)) { // We didn't get one; since there is a cont allocated in // the page, we can't madvise. Give up and unlock all. - goto unlock; + break; } - } while (++last_locked < (signed)BITMAPS_PER_PAGE); + } + + if (i >= BITMAPS_PER_PAGE) { #if DISPATCH_DEBUG - //fprintf(stderr, "%s: madvised page %p for cont %p (next = %p), " - // "[%u+1]=%u bitmaps at %p\n", __func__, page, c, c->do_next, - // last_locked-1, BITMAPS_PER_PAGE, &page_bitmaps[0]); - // Scribble to expose use-after-free bugs - // madvise (syscall) flushes these stores - memset(page, DISPATCH_ALLOCATOR_SCRIBBLE, DISPATCH_ALLOCATOR_PAGE_SIZE); + // fprintf(stderr, "%s: madvised page %p for cont %p (next = %p), " + // "[%u+1]=%u bitmaps at %p\n", __func__, page, c, c->do_next, + // last_locked-1, BITMAPS_PER_PAGE, &page_bitmaps[0]); + // Scribble to expose use-after-free bugs + // madvise (syscall) flushes these stores + memset(page, DISPATCH_ALLOCATOR_SCRIBBLE, DISPATCH_ALLOCATOR_PAGE_SIZE); #endif - (void)dispatch_assume_zero(madvise(page, DISPATCH_ALLOCATOR_PAGE_SIZE, - MADV_FREE)); - -unlock: - while (last_locked > 1) { - page_bitmaps[--last_locked] = BITMAP_C(0); + // madvise the page + (void)dispatch_assume_zero(madvise(page, DISPATCH_ALLOCATOR_PAGE_SIZE, + MADV_FREE)); } - if (last_locked) { - os_atomic_store(&page_bitmaps[0], BITMAP_C(0), relaxed); + while (i) { + os_atomic_store(&page_bitmaps[--i], BITMAP_C(0), relaxed); } return; } diff --git a/src/shims/atomic_sfb.h b/src/shims/atomic_sfb.h index a87def054..b42a3a651 100644 --- a/src/shims/atomic_sfb.h +++ b/src/shims/atomic_sfb.h @@ -32,7 +32,7 @@ // Returns UINT_MAX if all the bits in p were already set. DISPATCH_ALWAYS_INLINE static inline unsigned int -os_atomic_set_first_bit(volatile unsigned long *p, unsigned int max) +os_atomic_set_first_bit(unsigned long *p, unsigned int max) { unsigned long val, bit; if (max > (sizeof(val) * 8)) { @@ -82,7 +82,7 @@ os_atomic_set_first_bit(volatile unsigned long *p, unsigned int max) DISPATCH_ALWAYS_INLINE static inline unsigned int -os_atomic_set_first_bit(volatile unsigned long *p, unsigned int max_index) +os_atomic_set_first_bit(unsigned long *p, unsigned int max) { unsigned int index; unsigned long b, b_masked; @@ -94,7 +94,7 @@ os_atomic_set_first_bit(volatile unsigned long *p, unsigned int max_index) os_atomic_rmw_loop_give_up(return UINT_MAX); } index--; - if (unlikely(index > max_index)) { + if (unlikely(index > max)) { os_atomic_rmw_loop_give_up(return UINT_MAX); } b_masked = b | (1UL << index);