From e8e6b2e64c09edd93748a678ba85e0afbe1ab16a Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Mon, 26 Aug 2024 05:36:41 +0000 Subject: [PATCH 1/7] Fix find_last_non_zero_bit, and align metadata address before converting to data address. --- src/util/metadata/side_metadata/global.rs | 13 +- src/util/metadata/side_metadata/helpers.rs | 272 +++++++++++++++++++-- 2 files changed, 265 insertions(+), 20 deletions(-) diff --git a/src/util/metadata/side_metadata/global.rs b/src/util/metadata/side_metadata/global.rs index ad39a3d565..eaa0946002 100644 --- a/src/util/metadata/side_metadata/global.rs +++ b/src/util/metadata/side_metadata/global.rs @@ -991,6 +991,7 @@ impl SideMetadataSpec { /// /// This function uses non-atomic load for the side metadata. The user needs to make sure /// that there is no other thread that is mutating the side metadata. + #[allow(clippy::let_and_return)] pub unsafe fn find_prev_non_zero_value( &self, data_addr: Address, @@ -1000,7 +1001,15 @@ impl SideMetadataSpec { if self.uses_contiguous_side_metadata() { // Contiguous side metadata - self.find_prev_non_zero_value_fast::(data_addr, search_limit_bytes) + let result = self.find_prev_non_zero_value_fast::(data_addr, search_limit_bytes); + #[cfg(debug_assertions)] + { + // Double check if the implementation is correct + let result2 = + self.find_prev_non_zero_value_simple::(data_addr, search_limit_bytes); + assert_eq!(result, result2, "find_prev_non_zero_value_fast returned a diffrent result from the naive implementation."); + } + result } else { // TODO: We should be able to optimize further for this case. However, we need to be careful that the side metadata // is not contiguous, and we need to skip to the next chunk's side metadata when we search to a different chunk. @@ -1073,6 +1082,7 @@ impl SideMetadataSpec { BitByteRange::Bytes { start, end } => { match helpers::find_last_non_zero_bit_in_metadata_bytes(start, end) { helpers::FindMetaBitResult::Found { addr, bit } => { + let (addr, bit) = align_metadata_address(self, addr, bit); res = Some(contiguous_meta_address_to_address(self, addr, bit)); // Return true to abort the search. We found the bit. true @@ -1091,6 +1101,7 @@ impl SideMetadataSpec { match helpers::find_last_non_zero_bit_in_metadata_bits(addr, bit_start, bit_end) { helpers::FindMetaBitResult::Found { addr, bit } => { + let (addr, bit) = align_metadata_address(self, addr, bit); res = Some(contiguous_meta_address_to_address(self, addr, bit)); // Return true to abort the search. We found the bit. true diff --git a/src/util/metadata/side_metadata/helpers.rs b/src/util/metadata/side_metadata/helpers.rs index 9eb8823649..20e0b9bc4d 100644 --- a/src/util/metadata/side_metadata/helpers.rs +++ b/src/util/metadata/side_metadata/helpers.rs @@ -6,6 +6,7 @@ use crate::util::heap::layout::vm_layout::VMLayout; use crate::util::memory::MmapStrategy; #[cfg(target_pointer_width = "32")] use crate::util::metadata::side_metadata::address_to_chunked_meta_address; +use crate::util::metadata::MetadataValue; use crate::util::Address; use crate::MMAPPER; use std::io::Result; @@ -28,6 +29,7 @@ pub(super) fn address_to_contiguous_meta_address( } /// Performs reverse address translation from contiguous metadata bits to data addresses. +/// The input address and bit shift should be aligned. /// /// Arguments: /// * `metadata_spec`: The side metadata spec. It should be contiguous side metadata. @@ -38,6 +40,10 @@ pub(super) fn contiguous_meta_address_to_address( metadata_addr: Address, bit: u8, ) -> Address { + debug_assert_eq!( + align_metadata_address(metadata_spec, metadata_addr, bit), + (metadata_addr, bit) + ); let shift = (LOG_BITS_IN_BYTE as i32) - metadata_spec.log_num_of_bits as i32; let relative_meta_addr = metadata_addr - metadata_spec.get_absolute_offset(); @@ -58,6 +64,32 @@ pub(super) fn contiguous_meta_address_to_address( unsafe { Address::from_usize(data_addr) } } +/// Align an pair of a metadata address and a metadata bit offset to the start of this metadata value. +/// For example, when the metadata is 4 bits, it should only start at bit 0 or bit 4. +/// When the metadata is 16 bits, it should only start at bit 0, and its metadata address should be aligned to 2 bytes. +/// This is important, as [`contiguous_meta_address_to_address`] can only convert the start address of metadata to +/// the data address. +pub(super) fn align_metadata_address( + spec: &SideMetadataSpec, + metadata_addr: Address, + bit: u8, +) -> (Address, u8) { + if spec.log_num_of_bits >= LOG_BITS_IN_BYTE as usize { + ( + metadata_addr.align_down(1 << (spec.log_num_of_bits - LOG_BITS_IN_BYTE as usize)), + 0, + ) + } else { + ( + metadata_addr, + crate::util::conversions::raw_align_down( + bit as usize, + (1 << spec.log_num_of_bits) as usize, + ) as u8, + ) + } +} + /// Unmaps the specified metadata range, or panics. #[cfg(test)] pub(crate) fn ensure_munmap_metadata(start: Address, size: usize) { @@ -221,20 +253,18 @@ pub fn find_last_non_zero_bit_in_metadata_bytes( // Load and check a usize word let value = unsafe { cur.load::() }; if value != 0 { - // Find the exact non-zero byte within the usize using bitwise operations - let byte_offset = (value.trailing_zeros() / 8) as usize; - let byte_addr = cur + byte_offset; - let byte_value: u8 = ((value >> (byte_offset * 8)) & 0xFF) as u8; - let bit = find_last_non_zero_bit_in_u8(byte_value).unwrap(); + let bit = find_last_non_zero_bit(value, 0, usize::BITS as u8).unwrap(); + let byte_offset = bit >> LOG_BITS_IN_BYTE; + let bit_offset = bit - ((byte_offset) << LOG_BITS_IN_BYTE); return FindMetaBitResult::Found { - addr: byte_addr, - bit, + addr: cur + byte_offset as usize, + bit: bit_offset, }; } } else { // Load and check a byte let value = unsafe { cur.load::() }; - if let Some(bit) = find_last_non_zero_bit_in_u8(value) { + if let Some(bit) = find_last_non_zero_bit::(value, 0, 8) { return FindMetaBitResult::Found { addr: cur, bit }; } } @@ -252,22 +282,23 @@ pub fn find_last_non_zero_bit_in_metadata_bits( return FindMetaBitResult::UnmappedMetadata; } let byte = unsafe { addr.load::() }; - if let Some(bit) = find_last_non_zero_bit_in_u8(byte) { - if bit >= start_bit && bit < end_bit { - return FindMetaBitResult::Found { addr, bit }; - } + if let Some(bit) = find_last_non_zero_bit::(byte, start_bit, end_bit) { + return FindMetaBitResult::Found { addr, bit }; } FindMetaBitResult::NotFound } -fn find_last_non_zero_bit_in_u8(byte_value: u8) -> Option { - if byte_value != 0 { - let bit = byte_value.trailing_zeros(); - debug_assert!(bit < 8); - Some(bit as u8) - } else { - None +fn find_last_non_zero_bit(value: T, start: u8, end: u8) -> Option { + for cur_bit in (start..end).rev() { + assert!(cur_bit < T::BITS as u8); + if !value + .bitand(T::from_usize(1usize << cur_bit).unwrap()) + .is_zero() + { + return Some(cur_bit); + } } + None } pub fn scan_non_zero_bits_in_metadata_bytes( @@ -442,4 +473,207 @@ mod tests { test_round_trip_conversion(&spec, &TEST_ADDRESS_4KB_REGION); } + + #[test] + fn test_find_last_non_zero_bit_in_u8() { + use super::find_last_non_zero_bit; + let bit = find_last_non_zero_bit::(0b100101, 0, 1); + assert_eq!(bit, Some(0)); + + let bit = find_last_non_zero_bit::(0b100101, 0, 3); + assert_eq!(bit, Some(2)); + + let bit = find_last_non_zero_bit::(0b100101, 0, 8); + assert_eq!(bit, Some(5)); + } + + #[test] + fn test_align_metadata_address() { + let create_spec = |log_num_of_bits: usize| SideMetadataSpec { + name: "AlignMetadataBitTestSpec", + is_global: true, + offset: SideMetadataOffset::addr(GLOBAL_SIDE_METADATA_BASE_ADDRESS), + log_num_of_bits, + log_bytes_in_region: 3, + }; + + const ADDR_1000: Address = unsafe { Address::from_usize(0x1000) }; + const ADDR_1001: Address = unsafe { Address::from_usize(0x1001) }; + const ADDR_1002: Address = unsafe { Address::from_usize(0x1002) }; + const ADDR_1003: Address = unsafe { Address::from_usize(0x1003) }; + const ADDR_1004: Address = unsafe { Address::from_usize(0x1004) }; + const ADDR_1005: Address = unsafe { Address::from_usize(0x1005) }; + const ADDR_1006: Address = unsafe { Address::from_usize(0x1006) }; + const ADDR_1007: Address = unsafe { Address::from_usize(0x1007) }; + const ADDR_1008: Address = unsafe { Address::from_usize(0x1008) }; + const ADDR_1009: Address = unsafe { Address::from_usize(0x1009) }; + + let metadata_2bits = create_spec(1); + assert_eq!( + align_metadata_address(&metadata_2bits, ADDR_1000, 0), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_2bits, ADDR_1000, 1), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_2bits, ADDR_1000, 2), + (ADDR_1000, 2) + ); + assert_eq!( + align_metadata_address(&metadata_2bits, ADDR_1000, 3), + (ADDR_1000, 2) + ); + assert_eq!( + align_metadata_address(&metadata_2bits, ADDR_1000, 4), + (ADDR_1000, 4) + ); + assert_eq!( + align_metadata_address(&metadata_2bits, ADDR_1000, 5), + (ADDR_1000, 4) + ); + assert_eq!( + align_metadata_address(&metadata_2bits, ADDR_1000, 6), + (ADDR_1000, 6) + ); + assert_eq!( + align_metadata_address(&metadata_2bits, ADDR_1000, 7), + (ADDR_1000, 6) + ); + + let metadata_4bits = create_spec(2); + assert_eq!( + align_metadata_address(&metadata_4bits, ADDR_1000, 0), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_4bits, ADDR_1000, 1), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_4bits, ADDR_1000, 2), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_4bits, ADDR_1000, 3), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_4bits, ADDR_1000, 4), + (ADDR_1000, 4) + ); + assert_eq!( + align_metadata_address(&metadata_4bits, ADDR_1000, 5), + (ADDR_1000, 4) + ); + assert_eq!( + align_metadata_address(&metadata_4bits, ADDR_1000, 6), + (ADDR_1000, 4) + ); + assert_eq!( + align_metadata_address(&metadata_4bits, ADDR_1000, 7), + (ADDR_1000, 4) + ); + + let metadata_8bits = create_spec(3); + assert_eq!( + align_metadata_address(&metadata_8bits, ADDR_1000, 0), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_8bits, ADDR_1000, 1), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_8bits, ADDR_1000, 2), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_8bits, ADDR_1000, 3), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_8bits, ADDR_1000, 4), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_8bits, ADDR_1000, 5), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_8bits, ADDR_1000, 6), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_8bits, ADDR_1000, 7), + (ADDR_1000, 0) + ); + + let metadata_16bits = create_spec(4); + assert_eq!( + align_metadata_address(&metadata_16bits, ADDR_1000, 0), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_16bits, ADDR_1000, 1), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_16bits, ADDR_1000, 2), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_16bits, ADDR_1000, 3), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_16bits, ADDR_1000, 4), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_16bits, ADDR_1000, 5), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_16bits, ADDR_1000, 6), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_16bits, ADDR_1000, 7), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_16bits, ADDR_1001, 0), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_16bits, ADDR_1001, 1), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_16bits, ADDR_1001, 2), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_16bits, ADDR_1001, 3), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_16bits, ADDR_1001, 4), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_16bits, ADDR_1001, 5), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_16bits, ADDR_1001, 6), + (ADDR_1000, 0) + ); + assert_eq!( + align_metadata_address(&metadata_16bits, ADDR_1001, 7), + (ADDR_1000, 0) + ); + } } From f07c33b5fd83cc3a95042cae0cf1b0f565be750f Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Mon, 26 Aug 2024 06:14:08 +0000 Subject: [PATCH 2/7] Use leading_zeroes, which is considerably faster --- src/util/metadata/metadata_val_traits.rs | 18 ++++++++++ src/util/metadata/side_metadata/helpers.rs | 42 ++++++++++++---------- 2 files changed, 42 insertions(+), 18 deletions(-) diff --git a/src/util/metadata/metadata_val_traits.rs b/src/util/metadata/metadata_val_traits.rs index 2ac23736d1..a1b5282827 100644 --- a/src/util/metadata/metadata_val_traits.rs +++ b/src/util/metadata/metadata_val_traits.rs @@ -10,12 +10,30 @@ pub trait Bits { const BITS: u32; /// The size (in log2) of this atomic type in bits. const LOG2: u32; + + fn leading_zeros(self) -> u32; + fn trailing_zeros(self) -> u32; + fn leading_ones(self) -> u32; + fn trailing_ones(self) -> u32; } macro_rules! impl_bits_trait { ($t: ty) => { impl Bits for $t { const BITS: u32 = <$t>::BITS; const LOG2: u32 = Self::BITS.trailing_zeros(); + + fn leading_zeros(self) -> u32 { + <$t>::leading_zeros(self) + } + fn trailing_zeros(self) -> u32 { + <$t>::trailing_zeros(self) + } + fn leading_ones(self) -> u32 { + <$t>::leading_ones(self) + } + fn trailing_ones(self) -> u32 { + <$t>::trailing_ones(self) + } } }; } diff --git a/src/util/metadata/side_metadata/helpers.rs b/src/util/metadata/side_metadata/helpers.rs index 20e0b9bc4d..eeff08d393 100644 --- a/src/util/metadata/side_metadata/helpers.rs +++ b/src/util/metadata/side_metadata/helpers.rs @@ -6,7 +6,6 @@ use crate::util::heap::layout::vm_layout::VMLayout; use crate::util::memory::MmapStrategy; #[cfg(target_pointer_width = "32")] use crate::util::metadata::side_metadata::address_to_chunked_meta_address; -use crate::util::metadata::MetadataValue; use crate::util::Address; use crate::MMAPPER; use std::io::Result; @@ -253,7 +252,7 @@ pub fn find_last_non_zero_bit_in_metadata_bytes( // Load and check a usize word let value = unsafe { cur.load::() }; if value != 0 { - let bit = find_last_non_zero_bit(value, 0, usize::BITS as u8).unwrap(); + let bit = find_last_non_zero_bit_usize(value, 0, usize::BITS as u8).unwrap(); let byte_offset = bit >> LOG_BITS_IN_BYTE; let bit_offset = bit - ((byte_offset) << LOG_BITS_IN_BYTE); return FindMetaBitResult::Found { @@ -264,7 +263,7 @@ pub fn find_last_non_zero_bit_in_metadata_bytes( } else { // Load and check a byte let value = unsafe { cur.load::() }; - if let Some(bit) = find_last_non_zero_bit::(value, 0, 8) { + if let Some(bit) = find_last_non_zero_bit_u8(value, 0, 8) { return FindMetaBitResult::Found { addr: cur, bit }; } } @@ -282,23 +281,30 @@ pub fn find_last_non_zero_bit_in_metadata_bits( return FindMetaBitResult::UnmappedMetadata; } let byte = unsafe { addr.load::() }; - if let Some(bit) = find_last_non_zero_bit::(byte, start_bit, end_bit) { + if let Some(bit) = find_last_non_zero_bit_u8(byte, start_bit, end_bit) { return FindMetaBitResult::Found { addr, bit }; } FindMetaBitResult::NotFound } -fn find_last_non_zero_bit(value: T, start: u8, end: u8) -> Option { - for cur_bit in (start..end).rev() { - assert!(cur_bit < T::BITS as u8); - if !value - .bitand(T::from_usize(1usize << cur_bit).unwrap()) - .is_zero() - { - return Some(cur_bit); - } +fn find_last_non_zero_bit_u8(value: u8, start: u8, end: u8) -> Option { + let mask: u8 = ((1 << (end - start + 1)) - 1) << start; + let leading_zeroes = (value & mask).leading_zeros(); + if leading_zeroes >= u8::BITS { + None + } else { + Some(u8::BITS as u8 - leading_zeroes as u8) + } +} + +fn find_last_non_zero_bit_usize(value: usize, start: u8, end: u8) -> Option { + let mask: usize = ((1 << (end - start + 1)) - 1) << start; + let leading_zeroes = (value & mask).leading_zeros(); + if leading_zeroes >= usize::BITS { + None + } else { + Some(usize::BITS as u8 - leading_zeroes as u8) } - None } pub fn scan_non_zero_bits_in_metadata_bytes( @@ -476,14 +482,14 @@ mod tests { #[test] fn test_find_last_non_zero_bit_in_u8() { - use super::find_last_non_zero_bit; - let bit = find_last_non_zero_bit::(0b100101, 0, 1); + use super::find_last_non_zero_bit_u8; + let bit = find_last_non_zero_bit_u8(0b100101, 0, 1); assert_eq!(bit, Some(0)); - let bit = find_last_non_zero_bit::(0b100101, 0, 3); + let bit = find_last_non_zero_bit_u8(0b100101, 0, 3); assert_eq!(bit, Some(2)); - let bit = find_last_non_zero_bit::(0b100101, 0, 8); + let bit = find_last_non_zero_bit_u8(0b100101, 0, 8); assert_eq!(bit, Some(5)); } From 52c7604ae18ea5c3b5c8acb1aaf9438f98b4179b Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Mon, 26 Aug 2024 06:20:45 +0000 Subject: [PATCH 3/7] Only run some LOS tests with plans that use LOS --- .../mock_test_internal_ptr_large_object_multi_page.rs | 4 +++- .../mock_test_internal_ptr_large_object_same_page.rs | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/vm/tests/mock_tests/mock_test_internal_ptr_large_object_multi_page.rs b/src/vm/tests/mock_tests/mock_test_internal_ptr_large_object_multi_page.rs index 1855fcd54d..e330e3bb7c 100644 --- a/src/vm/tests/mock_tests/mock_test_internal_ptr_large_object_multi_page.rs +++ b/src/vm/tests/mock_tests/mock_test_internal_ptr_large_object_multi_page.rs @@ -1,6 +1,8 @@ -// GITHUB-CI: MMTK_PLAN=all +// GITHUB-CI: MMTK_PLAN=Immix,GenImmix,StickyImmix,MarkSweep,MarkCompact // GITHUB-CI: FEATURES=is_mmtk_object +// Only test this with plans that use LOS. NoGC does not use large object space. + use super::mock_test_prelude::*; use crate::util::*; diff --git a/src/vm/tests/mock_tests/mock_test_internal_ptr_large_object_same_page.rs b/src/vm/tests/mock_tests/mock_test_internal_ptr_large_object_same_page.rs index ab36b18249..f5f01511a4 100644 --- a/src/vm/tests/mock_tests/mock_test_internal_ptr_large_object_same_page.rs +++ b/src/vm/tests/mock_tests/mock_test_internal_ptr_large_object_same_page.rs @@ -1,6 +1,8 @@ -// GITHUB-CI: MMTK_PLAN=all +// GITHUB-CI: MMTK_PLAN=Immix,GenImmix,StickyImmix,MarkSweep,MarkCompact // GITHUB-CI: FEATURES=is_mmtk_object +// Only test this with plans that use LOS. NoGC does not use large object space. + use super::mock_test_prelude::*; use crate::AllocationSemantics; From 52de48d86dae8892163d36914bf186d0b3b110ba Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Tue, 27 Aug 2024 01:50:03 +0000 Subject: [PATCH 4/7] Fix some issues --- src/util/metadata/metadata_val_traits.rs | 18 ---------- src/util/metadata/side_metadata/helpers.rs | 42 ++++++++++++++-------- 2 files changed, 28 insertions(+), 32 deletions(-) diff --git a/src/util/metadata/metadata_val_traits.rs b/src/util/metadata/metadata_val_traits.rs index a1b5282827..2ac23736d1 100644 --- a/src/util/metadata/metadata_val_traits.rs +++ b/src/util/metadata/metadata_val_traits.rs @@ -10,30 +10,12 @@ pub trait Bits { const BITS: u32; /// The size (in log2) of this atomic type in bits. const LOG2: u32; - - fn leading_zeros(self) -> u32; - fn trailing_zeros(self) -> u32; - fn leading_ones(self) -> u32; - fn trailing_ones(self) -> u32; } macro_rules! impl_bits_trait { ($t: ty) => { impl Bits for $t { const BITS: u32 = <$t>::BITS; const LOG2: u32 = Self::BITS.trailing_zeros(); - - fn leading_zeros(self) -> u32 { - <$t>::leading_zeros(self) - } - fn trailing_zeros(self) -> u32 { - <$t>::trailing_zeros(self) - } - fn leading_ones(self) -> u32 { - <$t>::leading_ones(self) - } - fn trailing_ones(self) -> u32 { - <$t>::trailing_ones(self) - } } }; } diff --git a/src/util/metadata/side_metadata/helpers.rs b/src/util/metadata/side_metadata/helpers.rs index eeff08d393..7d411ff8d2 100644 --- a/src/util/metadata/side_metadata/helpers.rs +++ b/src/util/metadata/side_metadata/helpers.rs @@ -287,24 +287,35 @@ pub fn find_last_non_zero_bit_in_metadata_bits( FindMetaBitResult::NotFound } +macro_rules! impl_find_last_non_zero_bit { + ($int_ty: ty, $value: expr, $start: expr, $end: expr) => {{ + let mask = match (1 as $int_ty).checked_shl(($end - $start) as u32) { + Some(shl) => (shl - 1) << $start, + None => <$int_ty>::MAX << $start, + }; + let leading_zeroes = ($value & mask).leading_zeros(); + if leading_zeroes >= <$int_ty>::BITS { + None + } else { + Some(<$int_ty>::BITS as u8 - leading_zeroes as u8 - 1) + } + }}; +} + fn find_last_non_zero_bit_u8(value: u8, start: u8, end: u8) -> Option { - let mask: u8 = ((1 << (end - start + 1)) - 1) << start; - let leading_zeroes = (value & mask).leading_zeros(); - if leading_zeroes >= u8::BITS { - None - } else { - Some(u8::BITS as u8 - leading_zeroes as u8) - } + // TODO: Ideally we implement the function as a generic function. + // However Rust does not have a generic trait for integers yet. We can use + // third party traits or our own trait (such as MetadataValue), but it is cumbersome, + // and does not worth the efforts. + impl_find_last_non_zero_bit!(u8, value, start, end) } fn find_last_non_zero_bit_usize(value: usize, start: u8, end: u8) -> Option { - let mask: usize = ((1 << (end - start + 1)) - 1) << start; - let leading_zeroes = (value & mask).leading_zeros(); - if leading_zeroes >= usize::BITS { - None - } else { - Some(usize::BITS as u8 - leading_zeroes as u8) - } + // TODO: Ideally we implement the function as a generic function. + // However Rust does not have a generic trait for integers yet. We can use + // third party traits or our own trait (such as MetadataValue), but it is cumbersome, + // and does not worth the efforts. + impl_find_last_non_zero_bit!(usize, value, start, end) } pub fn scan_non_zero_bits_in_metadata_bytes( @@ -491,6 +502,9 @@ mod tests { let bit = find_last_non_zero_bit_u8(0b100101, 0, 8); assert_eq!(bit, Some(5)); + + let bit = find_last_non_zero_bit_u8(0b0, 0, 1); + assert_eq!(bit, None); } #[test] From 0c3b3e3c7a73a244f993ce5296892ae9667ae473 Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Wed, 28 Aug 2024 01:40:11 +0000 Subject: [PATCH 5/7] Fix an issue ni find_last_non_zero_bit_in_metadata_bytes that may cause us to check metadata value that is not in the given range. --- src/util/metadata/side_metadata/helpers.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/util/metadata/side_metadata/helpers.rs b/src/util/metadata/side_metadata/helpers.rs index 7d411ff8d2..a4c10b59ae 100644 --- a/src/util/metadata/side_metadata/helpers.rs +++ b/src/util/metadata/side_metadata/helpers.rs @@ -228,15 +228,21 @@ pub fn find_last_non_zero_bit_in_metadata_bytes( let mut mapped_chunk = Address::MAX; while cur > meta_start { // If we can check the whole word, set step to word size. Otherwise, the step is 1 (byte) and we check byte. - let step = if cur.is_aligned_to(BYTES_IN_ADDRESS) - && cur.align_down(BYTES_IN_ADDRESS) >= meta_start - { + let step = if cur.is_aligned_to(BYTES_IN_ADDRESS) && cur - BYTES_IN_ADDRESS >= meta_start { BYTES_IN_ADDRESS } else { 1 }; // Move to the address so we can load from it cur -= step; + // The value we check has to be in the range. + debug_assert!( + cur >= meta_start && cur < meta_end, + "Check metadata value at meta address {}, which is not in the range of [{}, {})", + cur, + meta_start, + meta_end + ); // If we are looking at an address that is not in a mapped chunk, we need to check if the chunk if mapped. if cur < mapped_chunk { From 10b7090af46433d5f7d49fd383a6e5060fa8bb05 Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Wed, 28 Aug 2024 01:44:09 +0000 Subject: [PATCH 6/7] Use generic type for find_last_non_zero_bit --- src/util/metadata/side_metadata/helpers.rs | 62 +++++++++------------- 1 file changed, 25 insertions(+), 37 deletions(-) diff --git a/src/util/metadata/side_metadata/helpers.rs b/src/util/metadata/side_metadata/helpers.rs index a4c10b59ae..0f0a442d9a 100644 --- a/src/util/metadata/side_metadata/helpers.rs +++ b/src/util/metadata/side_metadata/helpers.rs @@ -258,7 +258,7 @@ pub fn find_last_non_zero_bit_in_metadata_bytes( // Load and check a usize word let value = unsafe { cur.load::() }; if value != 0 { - let bit = find_last_non_zero_bit_usize(value, 0, usize::BITS as u8).unwrap(); + let bit = find_last_non_zero_bit::(value, 0, usize::BITS as u8).unwrap(); let byte_offset = bit >> LOG_BITS_IN_BYTE; let bit_offset = bit - ((byte_offset) << LOG_BITS_IN_BYTE); return FindMetaBitResult::Found { @@ -269,7 +269,7 @@ pub fn find_last_non_zero_bit_in_metadata_bytes( } else { // Load and check a byte let value = unsafe { cur.load::() }; - if let Some(bit) = find_last_non_zero_bit_u8(value, 0, 8) { + if let Some(bit) = find_last_non_zero_bit::(value, 0, 8) { return FindMetaBitResult::Found { addr: cur, bit }; } } @@ -287,41 +287,29 @@ pub fn find_last_non_zero_bit_in_metadata_bits( return FindMetaBitResult::UnmappedMetadata; } let byte = unsafe { addr.load::() }; - if let Some(bit) = find_last_non_zero_bit_u8(byte, start_bit, end_bit) { + if let Some(bit) = find_last_non_zero_bit::(byte, start_bit, end_bit) { return FindMetaBitResult::Found { addr, bit }; } FindMetaBitResult::NotFound } -macro_rules! impl_find_last_non_zero_bit { - ($int_ty: ty, $value: expr, $start: expr, $end: expr) => {{ - let mask = match (1 as $int_ty).checked_shl(($end - $start) as u32) { - Some(shl) => (shl - 1) << $start, - None => <$int_ty>::MAX << $start, - }; - let leading_zeroes = ($value & mask).leading_zeros(); - if leading_zeroes >= <$int_ty>::BITS { - None - } else { - Some(<$int_ty>::BITS as u8 - leading_zeroes as u8 - 1) - } - }}; -} - -fn find_last_non_zero_bit_u8(value: u8, start: u8, end: u8) -> Option { - // TODO: Ideally we implement the function as a generic function. - // However Rust does not have a generic trait for integers yet. We can use - // third party traits or our own trait (such as MetadataValue), but it is cumbersome, - // and does not worth the efforts. - impl_find_last_non_zero_bit!(u8, value, start, end) -} - -fn find_last_non_zero_bit_usize(value: usize, start: u8, end: u8) -> Option { - // TODO: Ideally we implement the function as a generic function. - // However Rust does not have a generic trait for integers yet. We can use - // third party traits or our own trait (such as MetadataValue), but it is cumbersome, - // and does not worth the efforts. - impl_find_last_non_zero_bit!(usize, value, start, end) +use num_traits::{CheckedShl, PrimInt}; +fn find_last_non_zero_bit(value: T, start: u8, end: u8) -> Option +where + T: PrimInt + CheckedShl, +{ + let mask = match T::one().checked_shl((end - start) as u32) { + Some(shl) => (shl - T::one()) << (start as u32), + None => T::max_value() << (start as u32), + }; + let masked = value & mask; + if masked.is_zero() { + None + } else { + let leading_zeroes = masked.leading_zeros(); + let total_bits = std::mem::size_of::() * u8::BITS as usize; + Some(total_bits as u8 - leading_zeroes as u8 - 1) + } } pub fn scan_non_zero_bits_in_metadata_bytes( @@ -499,17 +487,17 @@ mod tests { #[test] fn test_find_last_non_zero_bit_in_u8() { - use super::find_last_non_zero_bit_u8; - let bit = find_last_non_zero_bit_u8(0b100101, 0, 1); + use super::find_last_non_zero_bit; + let bit = find_last_non_zero_bit::(0b100101, 0, 1); assert_eq!(bit, Some(0)); - let bit = find_last_non_zero_bit_u8(0b100101, 0, 3); + let bit = find_last_non_zero_bit::(0b100101, 0, 3); assert_eq!(bit, Some(2)); - let bit = find_last_non_zero_bit_u8(0b100101, 0, 8); + let bit = find_last_non_zero_bit::(0b100101, 0, 8); assert_eq!(bit, Some(5)); - let bit = find_last_non_zero_bit_u8(0b0, 0, 1); + let bit = find_last_non_zero_bit::(0b0, 0, 1); assert_eq!(bit, None); } From 89b6e77b1be7af7264babaa313a82f9f59b0828e Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Wed, 28 Aug 2024 03:56:52 +0000 Subject: [PATCH 7/7] Change the end_addr in the simple impl --- src/util/metadata/side_metadata/global.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/util/metadata/side_metadata/global.rs b/src/util/metadata/side_metadata/global.rs index eaa0946002..bb6b98c326 100644 --- a/src/util/metadata/side_metadata/global.rs +++ b/src/util/metadata/side_metadata/global.rs @@ -1027,12 +1027,10 @@ impl SideMetadataSpec { let region_bytes = 1 << self.log_bytes_in_region; // Figure out the range that we need to search. let start_addr = data_addr.align_down(region_bytes); - let end_addr = data_addr - .saturating_sub(search_limit_bytes) - .align_down(region_bytes); + let end_addr = data_addr.saturating_sub(search_limit_bytes) + 1usize; let mut cursor = start_addr; - while cursor > end_addr { + while cursor >= end_addr { // We encounter an unmapped address. Just return None. if !cursor.is_mapped() { return None;