Lock mark sweep block list during release

Cargo.toml

@@ -125,6 +125,9 @@ immix_smaller_block = []
 # Zero the unmarked lines after a GC cycle in immix. This helps debug untraced objects.
 immix_zero_on_release = []
 
+# Run sanity checks for BlockList in mark sweep.
+ms_block_list_sanity = []
+
 # Run sanity GC
 sanity = []
 # Run analysis
@@ -138,7 +141,7 @@ nogc_no_zeroing = ["nogc_lock_free"]
 single_worker = []
 
 # To run expensive comprehensive runtime checks, such as checking duplicate edges
-extreme_assertions = []
+extreme_assertions = ["ms_block_list_sanity"]
 
 # Enable multiple spaces for NoGC, each allocator maps to an individual ImmortalSpace.
 nogc_multi_space = []

src/policy/marksweepspace/native_ms/block.rs

@@ -136,35 +136,35 @@ impl Block {
             .is_ok()
     }
 
-    pub fn load_prev_block(&self) -> Option<Block> {
+    pub(in crate::policy::marksweepspace::native_ms) fn load_prev_block(&self) -> Option<Block> {
         let prev = unsafe { Block::PREV_BLOCK_TABLE.load::<usize>(self.start()) };
         NonZeroUsize::new(prev).map(Block)
     }
 
-    pub fn load_next_block(&self) -> Option<Block> {
+    pub(in crate::policy::marksweepspace::native_ms) fn load_next_block(&self) -> Option<Block> {
         let next = unsafe { Block::NEXT_BLOCK_TABLE.load::<usize>(self.start()) };
         NonZeroUsize::new(next).map(Block)
     }
 
-    pub fn store_next_block(&self, next: Block) {
+    pub(in crate::policy::marksweepspace::native_ms) fn store_next_block(&self, next: Block) {
         unsafe {
             Block::NEXT_BLOCK_TABLE.store::<usize>(self.start(), next.start().as_usize());
         }
     }
 
-    pub fn clear_next_block(&self) {
+    pub(in crate::policy::marksweepspace::native_ms) fn clear_next_block(&self) {
         unsafe {
             Block::NEXT_BLOCK_TABLE.store::<usize>(self.start(), 0);
         }
     }
 
-    pub fn store_prev_block(&self, prev: Block) {
+    pub(in crate::policy::marksweepspace::native_ms) fn store_prev_block(&self, prev: Block) {
         unsafe {
             Block::PREV_BLOCK_TABLE.store::<usize>(self.start(), prev.start().as_usize());
         }
     }
 
-    pub fn clear_prev_block(&self) {
+    pub(in crate::policy::marksweepspace::native_ms) fn clear_prev_block(&self) {
         unsafe {
             Block::PREV_BLOCK_TABLE.store::<usize>(self.start(), 0);
         }

src/policy/marksweepspace/native_ms/block_list.rs

@@ -4,19 +4,23 @@ use crate::util::linear_scan::Region;
 use crate::vm::VMBinding;
 use std::sync::atomic::AtomicBool;
 use std::sync::atomic::Ordering;
+#[cfg(feature = "ms_block_list_sanity")]
+use std::sync::Mutex;
 
 /// List of blocks owned by the allocator
 #[repr(C)]
 pub struct BlockList {
-    pub first: Option<Block>,
-    pub last: Option<Block>,
-    pub size: usize,
-    pub lock: AtomicBool,
+    first: Option<Block>,
+    last: Option<Block>,
+    size: usize,
+    lock: AtomicBool,
+    #[cfg(feature = "ms_block_list_sanity")]
+    sanity_list: Mutex<Vec<Block>>,
 }
 
 impl std::fmt::Debug for BlockList {
     fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
-        write!(f, "BlockList {:?}", self.iter().collect::<Vec<Block>>())
+        write!(f, "{:?}", self.iter().collect::<Vec<Block>>())
     }
 }
 
@@ -27,16 +31,45 @@ impl BlockList {
             last: None,
             size,
             lock: AtomicBool::new(false),
+            #[cfg(feature = "ms_block_list_sanity")]
+            sanity_list: Mutex::new(vec![]),
+        }
+    }
+
+    #[cfg(feature = "ms_block_list_sanity")]
+    fn verify_block_list(&self, sanity_list: &mut Vec<Block>) {
+        if !sanity_list
+            .iter()
+            .cloned()
+            .eq(BlockListIterator { cursor: self.first })
+        {
+            eprintln!("Sanity block list: {:?}", sanity_list);
+            eprintln!("First {:?}", sanity_list.first());
+            eprintln!("Actual block list: {:?}", self);
+            eprintln!("First {:?}", self.first);
+            eprintln!("Block list {:?}", self as *const _);
+            panic!("Incorrect block list");
         }
     }
 
     /// List has no blocks
+    #[allow(clippy::let_and_return)]
     pub fn is_empty(&self) -> bool {
-        self.first.is_none()
+        let ret = self.first.is_none();
+
+        #[cfg(feature = "ms_block_list_sanity")]
+        {
+            let mut sanity_list = self.sanity_list.lock().unwrap();
+            self.verify_block_list(&mut sanity_list);
+            assert_eq!(sanity_list.is_empty(), ret);
+        }
+
+        ret
     }
 
     /// Remove a block from the list
     pub fn remove(&mut self, block: Block) {
+        trace!("Blocklist {:?}: Remove {:?}", self as *const _, block);
         match (block.load_prev_block(), block.load_next_block()) {
             (None, None) => {
                 self.first = None;
@@ -45,23 +78,40 @@ impl BlockList {
             (None, Some(next)) => {
                 next.clear_prev_block();
                 self.first = Some(next);
-                next.store_block_list(self);
+                // next.store_block_list(self);
+                debug_assert_eq!(next.load_block_list(), self as *mut _);
             }
             (Some(prev), None) => {
                 prev.clear_next_block();
                 self.last = Some(prev);
-                prev.store_block_list(self);
+                // prev.store_block_list(self);
+                debug_assert_eq!(prev.load_block_list(), self as *mut _);
             }
             (Some(prev), Some(next)) => {
                 prev.store_next_block(next);
                 next.store_prev_block(prev);
             }
         }
+
+        #[cfg(feature = "ms_block_list_sanity")]
+        {
+            let mut sanity_list = self.sanity_list.lock().unwrap();
+            if let Some((index, _)) = sanity_list
+                .iter()
+                .enumerate()
+                .find(|&(_, &val)| val == block)
+            {
+                sanity_list.remove(index);
+            } else {
+                panic!("Cannot find {:?} in the block list", block);
+            }
+            self.verify_block_list(&mut sanity_list);
+        }
     }
 
     /// Pop the first block in the list
     pub fn pop(&mut self) -> Option<Block> {
-        if let Some(head) = self.first {
+        let ret = if let Some(head) = self.first {
             if let Some(next) = head.load_next_block() {
                 self.first = Some(next);
                 next.clear_prev_block();
@@ -75,11 +125,27 @@ impl BlockList {
             Some(head)
         } else {
             None
+        };
+
+        #[cfg(feature = "ms_block_list_sanity")]
+        {
+            let mut sanity_list = self.sanity_list.lock().unwrap();
+            let sanity_ret = if sanity_list.is_empty() {
+                None
+            } else {
+                Some(sanity_list.remove(0)) // pop first
+            };
+            self.verify_block_list(&mut sanity_list);
+            assert_eq!(sanity_ret, ret);
         }
+
+        trace!("Blocklist {:?}: Pop = {:?}", self as *const _, ret);
+        ret
     }
 
     /// Push block to the front of the list
     pub fn push(&mut self, block: Block) {
+        trace!("Blocklist {:?}: Push {:?}", self as *const _, block);
         if self.is_empty() {
             block.clear_next_block();
             block.clear_prev_block();
@@ -93,10 +159,33 @@ impl BlockList {
             self.first = Some(block);
         }
         block.store_block_list(self);
+
+        #[cfg(feature = "ms_block_list_sanity")]
+        {
+            let mut sanity_list = self.sanity_list.lock().unwrap();
+            sanity_list.insert(0, block); // push front
+            self.verify_block_list(&mut sanity_list);
+        }
     }
 
     /// Moves all the blocks of `other` into `self`, leaving `other` empty.
     pub fn append(&mut self, other: &mut BlockList) {
+        trace!(
+            "Blocklist {:?}: Append Blocklist {:?}",
+            self as *const _,
+            other as *const _
+        );
+        #[cfg(feature = "ms_block_list_sanity")]
+        {
+            // Check before merging
+            let mut sanity_list = self.sanity_list.lock().unwrap();
+            self.verify_block_list(&mut sanity_list);
+            let mut sanity_list_other = other.sanity_list.lock().unwrap();
+            other.verify_block_list(&mut sanity_list_other);
+        }
+        #[cfg(feature = "ms_block_list_sanity")]
+        let mut sanity_list_in_other = other.sanity_list.lock().unwrap().clone();
+
         debug_assert_eq!(self.size, other.size);
         if !other.is_empty() {
             debug_assert!(
@@ -128,12 +217,26 @@ impl BlockList {
             }
             other.reset();
         }
+
+        #[cfg(feature = "ms_block_list_sanity")]
+        {
+            let mut sanity_list = self.sanity_list.lock().unwrap();
+            sanity_list.append(&mut sanity_list_in_other);
+            self.verify_block_list(&mut sanity_list);
+        }
     }
 
     /// Remove all blocks
     fn reset(&mut self) {
+        trace!("Blocklist {:?}: Reset", self as *const _);
         self.first = None;
         self.last = None;
+
+        #[cfg(feature = "ms_block_list_sanity")]
+        {
+            let mut sanity_list = self.sanity_list.lock().unwrap();
+            sanity_list.clear();
+        }
     }
 
     /// Lock the list. The MiMalloc allocator mostly uses thread-local block lists, and those operations on the list
@@ -152,10 +255,12 @@ impl BlockList {
                 .compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst)
                 .is_ok();
         }
+        trace!("Blocklist {:?}: locked", self as *const _);
     }
 
     /// Unlock list. See the comments on the lock method.
     pub fn unlock(&mut self) {
+        trace!("Blocklist {:?}: unlock", self as *const _);
         self.lock.store(false, Ordering::SeqCst);
     }
 
@@ -172,6 +277,25 @@ impl BlockList {
             }
         }
     }
+
+    /// Get the size of this block list.
+    #[allow(clippy::let_and_return)]
+    pub fn size(&self) -> usize {
+        let ret = self.size;
+
+        #[cfg(feature = "ms_block_list_sanity")]
+        {
+            let mut sanity_list = self.sanity_list.lock().unwrap();
+            self.verify_block_list(&mut sanity_list);
+        }
+
+        ret
+    }
+
+    /// Get the first block in the list.
+    pub fn first(&self) -> Option<Block> {
+        self.first
+    }
 }
 
 pub struct BlockListIterator {

src/policy/marksweepspace/native_ms/global.rs

@@ -65,15 +65,29 @@ impl AbandonedBlockLists {
     fn move_consumed_to_unswept(&mut self) {
         let mut i = 0;
         while i < MI_BIN_FULL {
+            // This is executed during release. We also execute SweepChunk which will access blocks and block lists during release.
+            // We have to acquire locks before we modify these block lists.
+            self.consumed[i].lock();
+            self.unswept[i].lock();
+
             if !self.consumed[i].is_empty() {
                 self.unswept[i].append(&mut self.consumed[i]);
             }
+
+            self.unswept[i].unlock();
+            self.consumed[i].unlock();
             i += 1;
         }
     }
 
     fn sweep<VM: VMBinding>(&mut self, space: &MarkSweepSpace<VM>) {
         for i in 0..MI_BIN_FULL {
+            // This is executed during release. We also execute SweepChunk which will access blocks and block lists during release.
+            // We have to acquire locks before we modify these block lists.
+            self.available[i].lock();
+            self.consumed[i].lock();
+            self.unswept[i].lock();
+
             self.available[i].sweep_blocks(space);
             self.consumed[i].sweep_blocks(space);
             self.unswept[i].sweep_blocks(space);
@@ -86,6 +100,10 @@ impl AbandonedBlockLists {
                     self.consumed[i].push(block);
                 }
             }
+
+            self.unswept[i].unlock();
+            self.consumed[i].unlock();
+            self.available[i].unlock();
         }
     }
 }

src/util/alloc/free_list_allocator.rs

@@ -218,10 +218,10 @@ impl<VM: VMBinding> FreeListAllocator<VM> {
         debug_assert!(bin <= MAX_BIN);
 
         let available = &mut available_blocks[bin];
-        debug_assert!(available.size >= size);
+        debug_assert!(available.size() >= size);
 
         if !available.is_empty() {
-            let mut cursor = available.first;
+            let mut cursor = available.first();
 
             while let Some(block) = cursor {
                 if block.has_free_cells() {
@@ -230,7 +230,7 @@ impl<VM: VMBinding> FreeListAllocator<VM> {
                 available.pop();
                 consumed_blocks.get_mut(bin).unwrap().push(block);
 
-                cursor = available.first;
+                cursor = available.first();
             }
         }
 
@@ -303,7 +303,7 @@ impl<VM: VMBinding> FreeListAllocator<VM> {
 
                 crate::policy::marksweepspace::native_ms::BlockAcquireResult::Fresh(block) => {
                     self.add_to_available_blocks(bin, block, stress_test);
-                    self.init_block(block, self.available_blocks[bin].size);
+                    self.init_block(block, self.available_blocks[bin].size());
 
                     return Some(block);
                 }