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.

Author: AZero13

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;
 }

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);