Skip to content

Conversation

playX18
Copy link

@playX18 playX18 commented Feb 14, 2025

Added track.rs which provides simple utility functions to track memory allocated by MMTk using crabgrind crate. When crabgrind feature is not enabled all functions are no-ops.

@qinsoon
Copy link
Member

qinsoon commented Feb 24, 2025

Thanks for the PR. I will look into this this week.

Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR annotates memory allocated by MMTk for tools like valgrind memcheck so valgrind can reason about MMTk's memory.

The following is how the PR maps MMTk's memory terms to valgrind's terms:

MMTk valgrind
PageResource meta mem pool (with VALGRIND_CREATE_MEMPOOL[1])
allocated pages superblocks/chunks (with VALGRIND_MEMPOOL_ALLOC[1])
objects malloclike blocks (with VALGRIND_MALLOCLIKE_BLOCK[2])

This mapping looks reasonable.

However, one obvious caveat is that, valgrind expects us to annotate freed objects (following the malloc/free pattern) while MMTk as a GC usually does not know dead objects. This means when an object is actually dead, we cannot tell valgrind promptly -- MMTk only tells valgrind after the entire region (whatever the MMTk policy defines) is empty, then valgrind automatically marks the internal objects in the region as free. But this is not a show stopper. I believe even with this limitation, valgrind can still be helpful.

sft_map.notify_space_creation(s.as_sft());
s.initialize_sft(sft_map);
// after SFT is initialized, we can also initialize mempool tracking
s.get_page_resource().track();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that you use the address of PageResource to register as a mempool, and the address is only fixed here. If this is the case, just make this explicit in the comments.

if self.clear_log_bit_on_sweep {
VM::VMObjectModel::GLOBAL_LOG_BIT_SPEC.clear::<VM>(object, Ordering::SeqCst);
}
track_free(object.to_object_start::<VM>(), 0 /* TODO: Size */);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original code was accidentally removed?

trace!("alloc_slow");
self.acquire_block(size, align, offset, false)
let block = self.acquire_block(size, align, offset, false);
track_malloc(block, size, false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look right: the change above marks each allocation/object as 'malloc', but this marks the entire thread local buffer as 'malloc'.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you mark overlapping memory? E.g. marking the buffer first, then marking objects in it.

self.block_queue.add_global_array(array);
// Finish slow-allocation
self.commit_pages(reserved_pages, required_pages, tls);
track_mempool_alloc(self, first_block, required_pages * BYTES_IN_PAGE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates a chunk for valgrind that starts at first_block and have required_pages, which could be multiple blocks. But later track_mempool_free is called at block granularity, which means it frees a subset of what was allocated. Is this allowed for valgrind?

If not, we could just call 'track_mempool_alloc` for each block, and free each block separately.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also applies to other PageResource that allocates at page granularity.


impl<VM: VMBinding> PageResource<VM> for ExternalPageResource<VM> {
fn track(&self) {
/* cannot track external pages reliably? */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use panic or warn!.

}
self.commit_pages(reserved_pages, required_pages, tls);

track_mempool_alloc(self, rtn, conversions::pages_to_bytes(required_pages));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is never freed. Pages can be freed when the page resource releases the pages.

if tracking_enabled() {
let mut cursor = region.start();
while cursor < region.end() {
if let Some(object) = vo_bit::is_vo_bit_set_for_addr(cursor) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work reliably, depending on how we maintain VO bits (see the code below it). This is the issue that I explained: MMTk as a GC usually does not know what objects are dead, and only knows what is alive. We may not be able to reliably tell what objects are freed, we can only tell valgrind when the page is empty, and let valgrind to automatically free those objects.

#[cfg(feature = "crabgrind")]
{
use crabgrind::memcheck::Error;
let result = crabgrind::memcheck::is_defined(_start.to_mut_ptr(), _size);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may use this function for side metadata memory. Those memory is not annotated for valgrind. Will this work?

}
}

pub fn track_malloc(p: Address, size: usize, zero: bool) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The arguments passed as zero from different call sites are a bit random:

  • bump pointer: false
  • freelist and large object: true

Each space defines whether the memory needs to be zeroed. You could get the value from there, or just use true in this function (assuming they will be zeroed).

}
}

pub fn track_free(p: Address, size: usize) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

size can be removed. crabgrind::memcheck::alloc::free asks for rz (https://docs.rs/crabgrind/latest/crabgrind/memcheck/alloc/fn.free.html). Is it redzone size, rather than allocation size?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants