Skip to content

Commit

Permalink
Remove fl_map and refactor FreeListPageResoure (#953)
Browse files Browse the repository at this point in the history
This PR removes the `shared_fl_map` field from `Map32`, and removes
`fl_map` and `fl_page_resource` fields from `Map64`. These global maps
were used to enumerate and update the starting address of spaces (stored
in `CommonFreeListPageResource` instances), for several different
purposes.

1. `Map32` can only decide the starting address of discontiguous spaces
after all contiguous spaces are placed, therefore it needs to modify the
start address of all discontiguous `FreeListPageResource` instances.
- `shared_fl_map` was used to locate `CommonFreeListPageResource`
instances by the space ordinal.
- With this PR, we use `Plan::for_each_space_mut` to enumerate spaces,
and use the newly added `Space::maybe_get_page_resource_mut` method to
get the page resource (if it has one) in order to update their starting
addresses.
2. `Map64` allocates `RawMemoryFreeList` in the beginning of each space
that uses `FreeListPageResource`. It needs to update the start address
of each space to take the address range occupied by `RawMemoryFreeList`
into account.
- `fl_page_resource` was used to locate `CommonFreeListPageResource`
instances by the space ordinal.
- `fl_map` was used to locate the underlying `RawMemoryFreeList`
instances of the corresponding `FreeListPageResource` by the space
ordinal.
- With this PR, we initialize the start address of the
`FreeListPageResource` immediately after a `RawMemoryFreeList` is
created, taking the limit of `RawMemoryFreeList` into account.
Therefore, it is no longer needed to update the starting address later.

Because those global maps are removed, several changes are made to
`VMMap` and its implementations `Map32` and `Map64`.

- The `VMMap::boot` and the `VMMap::bind_freelist` no longer serve any
purpose, and are removed.
- `Map64::finalize_static_space_map` now does nothing but setting
`finalized` to true.

The `CommonFreeListPageResource` data structure was introduced for the
sole purpose to be referenced by those global maps. This PR removes
`CommonFreeListPageResource` and `FreeListPageResourceInner`, and
relocates their fields.

- `common: CommonPageResource` is now a field of `FreeListPageResource`.
- The `free_list: Box<dyn FreeList>` and `start: Address` are moved into
`FreeListPageResourceSync` because they have always been accessed while
holding the mutex `FreeListPageResource::sync`.
- Note that the method `FreeListPageResource::release_pages` used to
call `free_list.size()` without holding the `sync` mutex, and
subsequently calls `free_list.free()` with the `sync` mutex. The
algorithm of `FreeList` guarantees `free()` will not race with `size()`.
But the `Mutex` forbids calling the read-only operation `free()` without
holding the lock because in general it is possible that a read-write
operations may race with read-only operations, too. For now, we extended
the range of the mutex to cover the whole function.

After the changes above, the `FreeListPageResource` no longer needs
`UnsafeCell`.

This PR is one step of the greater goal of removing unsafe operations
related to FreeList, PageResource and VMMap. See:
#853

Related PRs:
- #925: This PR is a subset of
that PR.
  • Loading branch information
wks authored May 10, 2024
1 parent 5f955bc commit 698a737
Show file tree
Hide file tree
Showing 19 changed files with 268 additions and 247 deletions.
17 changes: 14 additions & 3 deletions src/mmtk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ impl<VM: VMBinding> MMTK<VM> {
// So we do not save it in MMTK. This may change in the future.
let mut heap = HeapMeta::new();

let plan = crate::plan::create_plan(
let mut plan = crate::plan::create_plan(
*options.plan,
CreateGeneralPlanArgs {
vm_map: VM_MAP.as_ref(),
Expand All @@ -188,9 +188,20 @@ impl<VM: VMBinding> MMTK<VM> {
}

// TODO: This probably does not work if we have multiple MMTk instances.
VM_MAP.boot();
// This needs to be called after we create Plan. It needs to use HeapMeta, which is gradually built when we create spaces.
VM_MAP.finalize_static_space_map(heap.get_discontig_start(), heap.get_discontig_end());
VM_MAP.finalize_static_space_map(
heap.get_discontig_start(),
heap.get_discontig_end(),
&mut |start_address| {
plan.for_each_space_mut(&mut |space| {
// If the `VMMap` has a discontiguous memory range, we notify all discontiguous
// space that the starting address has been determined.
if let Some(pr) = space.maybe_get_page_resource_mut() {
pr.update_discontiguous_start(start_address);
}
})
},
);

if *options.transparent_hugepages {
MMAPPER.set_mmap_strategy(crate::util::memory::MmapStrategy::TransparentHugePages);
Expand Down
4 changes: 4 additions & 0 deletions src/policy/copyspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,10 @@ impl<VM: VMBinding> Space<VM> for CopySpace<VM> {
&self.pr
}

fn maybe_get_page_resource_mut(&mut self) -> Option<&mut dyn PageResource<VM>> {
Some(&mut self.pr)
}

fn common(&self) -> &CommonSpace<VM> {
&self.common
}
Expand Down
3 changes: 3 additions & 0 deletions src/policy/immix/immixspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,9 @@ impl<VM: VMBinding> Space<VM> for ImmixSpace<VM> {
fn get_page_resource(&self) -> &dyn PageResource<VM> {
&self.pr
}
fn maybe_get_page_resource_mut(&mut self) -> Option<&mut dyn PageResource<VM>> {
Some(&mut self.pr)
}
fn common(&self) -> &CommonSpace<VM> {
&self.common
}
Expand Down
3 changes: 3 additions & 0 deletions src/policy/immortalspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ impl<VM: VMBinding> Space<VM> for ImmortalSpace<VM> {
fn get_page_resource(&self) -> &dyn PageResource<VM> {
&self.pr
}
fn maybe_get_page_resource_mut(&mut self) -> Option<&mut dyn PageResource<VM>> {
Some(&mut self.pr)
}
fn common(&self) -> &CommonSpace<VM> {
&self.common
}
Expand Down
3 changes: 3 additions & 0 deletions src/policy/largeobjectspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,9 @@ impl<VM: VMBinding> Space<VM> for LargeObjectSpace<VM> {
fn get_page_resource(&self) -> &dyn PageResource<VM> {
&self.pr
}
fn maybe_get_page_resource_mut(&mut self) -> Option<&mut dyn PageResource<VM>> {
Some(&mut self.pr)
}

fn initialize_sft(&self, sft_map: &mut dyn crate::policy::sft_map::SFTMap) {
self.common().initialize_sft(self.as_sft(), sft_map)
Expand Down
3 changes: 3 additions & 0 deletions src/policy/lockfreeimmortalspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,9 @@ impl<VM: VMBinding> Space<VM> for LockFreeImmortalSpace<VM> {
fn get_page_resource(&self) -> &dyn PageResource<VM> {
unimplemented!()
}
fn maybe_get_page_resource_mut(&mut self) -> Option<&mut dyn PageResource<VM>> {
None
}
fn common(&self) -> &CommonSpace<VM> {
unimplemented!()
}
Expand Down
4 changes: 4 additions & 0 deletions src/policy/markcompactspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ impl<VM: VMBinding> Space<VM> for MarkCompactSpace<VM> {
&self.pr
}

fn maybe_get_page_resource_mut(&mut self) -> Option<&mut dyn PageResource<VM>> {
Some(&mut self.pr)
}

fn common(&self) -> &CommonSpace<VM> {
&self.common
}
Expand Down
4 changes: 4 additions & 0 deletions src/policy/marksweepspace/malloc_ms/global.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,10 @@ impl<VM: VMBinding> Space<VM> for MallocSpace<VM> {
unreachable!()
}

fn maybe_get_page_resource_mut(&mut self) -> Option<&mut dyn PageResource<VM>> {
None
}

fn common(&self) -> &CommonSpace<VM> {
unreachable!()
}
Expand Down
6 changes: 5 additions & 1 deletion src/policy/marksweepspace/native_ms/global.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::{
scheduler::{GCWorkScheduler, GCWorker},
util::{
copy::CopySemantics,
heap::FreeListPageResource,
heap::{FreeListPageResource, PageResource},
metadata::{self, side_metadata::SideMetadataSpec, MetadataSpec},
ObjectReference,
},
Expand Down Expand Up @@ -209,6 +209,10 @@ impl<VM: VMBinding> Space<VM> for MarkSweepSpace<VM> {
&self.pr
}

fn maybe_get_page_resource_mut(&mut self) -> Option<&mut dyn PageResource<VM>> {
Some(&mut self.pr)
}

fn initialize_sft(&self, sft_map: &mut dyn crate::policy::sft_map::SFTMap) {
self.common().initialize_sft(self.as_sft(), sft_map)
}
Expand Down
4 changes: 4 additions & 0 deletions src/policy/space.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ pub trait Space<VM: VMBinding>: 'static + SFT + Sync + Downcast {
fn as_sft(&self) -> &(dyn SFT + Sync + 'static);
fn get_page_resource(&self) -> &dyn PageResource<VM>;

/// Get a mutable reference to the underlying page resource, or `None` if the space does not
/// have a page resource.
fn maybe_get_page_resource_mut(&mut self) -> Option<&mut dyn PageResource<VM>>;

/// Initialize entires in SFT map for the space. This is called when the Space object
/// has a non-moving address, as we will use the address to set sft.
/// Currently after we create a boxed plan, spaces in the plan have a non-moving address.
Expand Down
3 changes: 3 additions & 0 deletions src/policy/vmspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ impl<VM: VMBinding> Space<VM> for VMSpace<VM> {
fn get_page_resource(&self) -> &dyn PageResource<VM> {
&self.pr
}
fn maybe_get_page_resource_mut(&mut self) -> Option<&mut dyn PageResource<VM>> {
Some(&mut self.pr)
}
fn common(&self) -> &CommonSpace<VM> {
&self.common
}
Expand Down
4 changes: 4 additions & 0 deletions src/util/heap/blockpageresource.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ impl<VM: VMBinding, B: Region> PageResource<VM> for BlockPageResource<VM, B> {
self.flpr.common_mut()
}

fn update_discontiguous_start(&mut self, start: Address) {
self.flpr.update_discontiguous_start(start)
}

fn alloc_pages(
&self,
space_descriptor: SpaceDescriptor,
Expand Down
Loading

0 comments on commit 698a737

Please sign in to comment.