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

8260262: Use common code in function unmap_shared() in perfMemory_posix.cpp #4995

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -1023,18 +1023,23 @@ static char* mmap_create_shared(size_t size) {
return mapAddress;
}

// release a named shared memory region
// release a named shared memory region that was mmap-ed.
//
static void unmap_shared(char* addr, size_t bytes) {
#if defined(_AIX)
// Do not rely on os::reserve_memory/os::release_memory to use mmap.
// Use os::reserve_memory/os::release_memory for PerfDisableSharedMem=1, mmap/munmap for PerfDisableSharedMem=0
if (::munmap(addr, bytes) == -1) {
warning("perfmemory: munmap failed (%d)\n", errno);
int res;
if (MemTracker::tracking_level() > NMT_minimal) {
// Note: Tracker contains a ThreadCritical.
Tracker tkr(Tracker::release);
res = ::munmap(addr, bytes);
if (res == 0) {
tkr.record((address)addr, bytes);
}
} else {
res = ::munmap(addr, bytes);
}
if (res != 0) {
log_info(os)("os::release_memory failed (" PTR_FORMAT ", " SIZE_FORMAT ")", p2i(addr), bytes);
}
#else
os::release_memory(addr, bytes);
Copy link
Contributor

@coleenp coleenp Aug 9, 2021

Choose a reason for hiding this comment

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

Why doesn't AIX call munmap with os::release_memory() and then do this NMT tracking?

Copy link
Member

@tstuefe tstuefe Aug 9, 2021

Choose a reason for hiding this comment

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

I'm not sure I understand what you are proposing.

The old code was incorrect since it used raw ::mmap() to map perf memory, but used os::release_memory() to release it. This was asymmetric and assumed the underlying implementation for os::reserve/release... uses mmap, which is not always true. The patch corrects this by coupling raw ::mmap with raw ::munmap, and do NMT tracking accordingly.

Arguably, one could factor out mmap+NMT and munmap+NMT in os_posix.cpp. We probably have a number of places where this would be needed. E.g. polling pages and such. But I guess that was out of scope for this issue.

Copy link
Contributor

@coleenp coleenp Aug 9, 2021

Choose a reason for hiding this comment

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

I see, reading the code more ... The code to create the perf memory if PerfDisableSharedMemory=0 used mmap directly so it makes sense to use munmap directly too. This is definitely better.

Aside: This definition is the same on all the posix platforms so could be moved. Not with this RFE but definitely a trivial one.
bool os::pd_unmap_memory(char* addr, size_t bytes) {

Copy link
Member

@tstuefe tstuefe Aug 9, 2021

Choose a reason for hiding this comment

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

I agree.

#endif
}

// create the PerfData memory region in shared memory.