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

8252500: ZGC on aarch64: Unable to allocate heap for certain Linux kernel configurations #40

Closed
wants to merge 6 commits into from
65 changes: 63 additions & 2 deletions src/hotspot/cpu/aarch64/gc/z/zGlobals_aarch64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,14 @@
*/

#include "precompiled.hpp"
#include "gc/shared/gcLogPrecious.hpp"
#include "gc/z/zGlobals.hpp"
#include "runtime/globals.hpp"
#include "utilities/globalDefinitions.hpp"
#include "utilities/powerOfTwo.hpp"

#include <sys/mman.h>
Copy link
Member

Choose a reason for hiding this comment

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

Should this also be guarded by #ifdef LINUX?


//
// The heap can have three different layouts, depending on the max heap size.
//
Expand Down Expand Up @@ -135,9 +138,67 @@
// * 63-48 Fixed (16-bits, always zero)
//

// Default value if probing is not implemented for a certain platform: 128TB
static const size_t DEFAULT_MAX_ADDRESS_BIT = 47;
// Minimum value returned, if probing fails: 64GB
static const size_t MINIMUM_MAX_ADDRESS_BIT = 36;

static size_t probe_valid_max_address_bit() {
#ifdef LINUX
size_t max_address_bit = 0;
const size_t page_size = os::vm_page_size();
for (size_t i = DEFAULT_MAX_ADDRESS_BIT; i > MINIMUM_MAX_ADDRESS_BIT; --i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the loop condition be MINIMUM_MAX_ADDRESS_BIT inclusive instead of exclusive? Seems weird that it is exclusive, when we later check if max_address_bit < MINIMUM_MAX_ADDRESS_BIT return MINIMUM_MAX_ADDRESS_BIT, which seems like exactly what will happen if the loop would have advanced to MINIMUM_MAX_ADDRESS_BIT (and then finish the loop).

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how to comment a few lines below where you conditionally return MINIMUM_MAX_ADDRESS_BIT in this new cool GUI. Anyway, you could use MIN2 to return MIN2(MINIMUM_MAX_ADDRESS_BIT, max_address_bit) to make the intention more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not processing MINIMUM_MAX_ADDRESS_BIT in the loop, because this is the absolute minimum that should be returned. So, regardless of the probing result, we are going to return that value. Therefor, it is a waste of time to actually perform the probe.

The check max_address_bit < MINIMUM_MAX_ADDRESS_BIT is done, because after the probing (if it failed and we didn't find any valid page), right after the for loop, we do a single mmap and use the returned address to calculate the max_address_bit. So max_address_bit could then be lower than MINIMUM_MAX_ADDRESS_BIT.

For the return part, you are right, we could simplify it by using MAX2, since we are interested in the highest bit, and MINIMUM_MAX_ADDRESS_BIT is the absolute minimum we want to return.

I will push an update for the return part.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Incremental looks good.

const uintptr_t base_addr = ((uintptr_t) 1U) << i;
if (msync((void*)base_addr, page_size, MS_ASYNC) == 0) {
// msync suceeded, the address is valid, and maybe even already mapped.
max_address_bit = i;
break;
}
if (errno != ENOMEM) {
// Some error occured. This should never happen, but msync
// has some undefined behavior, hence ignore this bit.
#ifdef ASSERT
fatal("Received %s while probing the address space for the highest valid bit", os::errno_name(errno));
#else // ASSERT
log_warning_p(gc)("Received %s while probing the address space for the highest valid bit", os::errno_name(errno));
Copy link
Member

Choose a reason for hiding this comment

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

Should explicitly include runtime/os.hpp.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please turn the two %s into '%s', otherwise the output will look strange.

#endif // ASSERT
continue;
}
// Since msync failed with ENOMEM, the page might not be mapped.
// Try to map it, to see if the address is valid.
void* result_addr = mmap((void*) base_addr, page_size, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, -1, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make this "void* const result_addr = ...".

if (result_addr != MAP_FAILED) {
munmap(result_addr, page_size);
}
if ((uintptr_t) result_addr == base_addr) {
// address is valid
max_address_bit = i;
break;
}
}
if (max_address_bit == 0) {
// probing failed, allocate a very high page and take that bit as the maximum
uintptr_t high_addr = ((uintptr_t) 1U) << DEFAULT_MAX_ADDRESS_BIT;
void* result_addr = mmap((void*) high_addr, page_size, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, -1, 0);
Copy link
Contributor

@pliden pliden Sep 8, 2020

Choose a reason for hiding this comment

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

Same here, please make these two locals const.

if (result_addr != MAP_FAILED) {
max_address_bit = BitsPerSize_t - count_leading_zeros((size_t) result_addr) - 1;
Copy link
Member

Choose a reason for hiding this comment

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

It's not obvious to me that the '- 1' is correct here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's assume the probing failed (for whatever reason) and we are actually able to allocate DEFAULT_MAX_ADDRESS_BIT:
00000000_00000000_10000000_00000000_00000000_00000000_00000000_00000000 = 1U << DEFAULT_MAX_ADDRESS_BIT
Mmap succeeds and returns exactly that address. Now the line would evaluate to the following:
64 - 16 - 1 = 47 <=> DEFAULT_MAX_ADDRESS_BIT

Copy link
Member

Choose a reason for hiding this comment

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

I think this looks OK now that you moved the +1 to ZPlatformAddressOffsetBits().

munmap(result_addr, page_size);
}
}
log_info_p(gc, init)("Probing address space for the highest valid bit: %zu", max_address_bit);
Copy link
Member

Choose a reason for hiding this comment

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

I mentioned this during our pre-review, but I see that it wasn't updated: %zu should be using SIZE_FORMAT.

if (max_address_bit < MINIMUM_MAX_ADDRESS_BIT) {
return MINIMUM_MAX_ADDRESS_BIT;
}
return max_address_bit;
#else // LINUX
return DEFAULT_MAX_ADDRESS_BIT;
#endif // LINUX
}

size_t ZPlatformAddressOffsetBits() {
const size_t min_address_offset_bits = 42; // 4TB
const size_t max_address_offset_bits = 44; // 16TB
const static size_t valid_max_address_offset_bits = probe_valid_max_address_bit() + 1;
const size_t max_address_offset_bits = valid_max_address_offset_bits - 3;
const size_t min_address_offset_bits = max_address_offset_bits - 2;
const size_t address_offset = round_up_power_of_2(MaxHeapSize * ZVirtualToPhysicalRatio);
const size_t address_offset_bits = log2_intptr(address_offset);
return clamp(address_offset_bits, min_address_offset_bits, max_address_offset_bits);
Expand Down