Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dump process map on mmap failure and fix typos #1127

Merged
merged 4 commits into from
Apr 24, 2024

Conversation

k-sareen
Copy link
Collaborator

No description provided.

Comment on lines 150 to 151
#[cfg(target_os = "linux")]
info!("{}", get_process_memory_maps());
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. It is better if we make the call regardless of the current OS, but we may implement the function platform-specific. At this moment, we can make the function do nothing (or return empty string or "(unavailable)") for systems other than Linux.
  2. We should just print using eprint! or eprintln! (or do the printing inside get_process_memory_maps, or introduce a print_process_memory_maps). It will be very helpful to have the information when the release build fails. And if it fails, it doesn't matter whether we print more things to the console or less.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure

Copy link
Collaborator Author

@k-sareen k-sareen Apr 23, 2024

Choose a reason for hiding this comment

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

Ideally we want more information (what address was MMTk trying to mmap), but our current internal API for all this is quite wonky.

Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

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

I just realised in the case of OOM, we should let Collection::out_of_memory choose whether to print the mmap or silently recover from the error. But if other mmap errors occur, we can still just print the error.

@@ -147,6 +147,7 @@ pub fn handle_mmap_error<VM: VMBinding>(error: Error, tls: VMThread) -> ! {
match error.kind() {
// From Rust nightly 2021-05-12, we started to see Rust added this ErrorKind.
ErrorKind::OutOfMemory => {
eprintln!("{}", get_process_memory_maps());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, I just realised that the out-of-memory event is not always lethal because the VM binding can implement VM::VMCollection::out_of_memory. If the VM can recover from OOM (like OpenJDK), it is probably unnecessary to print the memory maps. But if it crashes (like when OpenJDK generates something like hs_err_pidXXXXX.log), the ability to print mmap would be helpful. I think if there is a call to VM::VMCollection::out_of_memory following this, we should move it to the default implementation of Collection::out_of_memory (because it panics anyway).

Copy link
Collaborator Author

@k-sareen k-sareen Apr 23, 2024

Choose a reason for hiding this comment

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

No this is different. This is an mmap error, not a simple OOM error. The simple OOM error is AllocationError::HeapOutOfMemory. As it says in the comment below the inserted line: "Expect the VM to abort immediately"

@@ -159,14 +160,18 @@ pub fn handle_mmap_error<VM: VMBinding>(error: Error, tls: VMThread) -> ! {
if let Some(os_errno) = error.raw_os_error() {
// If it is OOM, we invoke out_of_memory() through the VM interface.
if os_errno == libc::ENOMEM {
eprintln!("{}", get_process_memory_maps());
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is OOM, too. We may rely on the default impl of Collection::out_of_memory to print the mmap.

src/util/memory.rs Show resolved Hide resolved
@k-sareen k-sareen requested a review from wks April 24, 2024 01:54
Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

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

LGTM

@k-sareen k-sareen added this pull request to the merge queue Apr 24, 2024
Merged via the queue into mmtk:master with commit 1b9cfe4 Apr 24, 2024
23 checks passed
@k-sareen k-sareen deleted the fix/misc-typos-and-comment-formatting branch April 24, 2024 04:02
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