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
64 changes: 62 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,66 @@
// * 63-48 Fixed (16-bits, always zero)
//

// Default value if probing is not implemented for a certain platform: 128TB
#define DEFAULT_MAX_ADDRESS_BIT 47
// Minimum value returned, if probing fails: 64GB
#define MINIMUM_MAX_ADDRESS_BIT 36
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 could/should be typed static constants instead of defines.


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 (int i = DEFAULT_MAX_ADDRESS_BIT; i > MINIMUM_MAX_ADDRESS_BIT && max_address_bit == 0; --i) {
Copy link
Member

Choose a reason for hiding this comment

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

There's a bit of a mismatch between the type of 'i' and 'max_address_bit'. See also 'BitsPerSize_t - count_leading_zeros((size_t) result_addr) - 1;'. I think that changing max_address_bit to be an int, and getting rid of the size_t's here will fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it the other way around, and made 'i' of type size_t, since 'i' is the only int variable left and all the others are of type size_t.

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;
}
Copy link
Member

Choose a reason for hiding this comment

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

Is there a one-off error here? Taking i == 47 as an example. This means that you test the base_address == '10000000 00000000 00000000 00000000 00000000 00000000' (in bits). That is, you test that the address range with the 48th bit set (128T-256T) is usable. However, when 47 then is returned to the caller, it interprets it as if the 64T-128T range is usable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I verified your assumption by doing the following:
I returned the result of probe_valid_max_address_bit() + 1 from ZPlatformAddressOffsetBits(). I then executed a VM on a platform with only 3 pagetable levels, which has less than DEFAULT_MAX_ADDRESS_BIT bits available in the address space. The heap allocation succeeded, which means the current patch has a one off error.

However, I would fix this in ZPlatformAddressOffsetBits(). because I interpret the result of probe_valid_max_address_bit() as to be the highest bit index of an address which can be allocated. Which means that mmap the result of 1 << probe_valid_max_address_bit() should succeed (if the page is not already mapped). I think ZPlatformAddressOffsetBits() should add 1 to the result of probe_valid_max_address_bit().

Copy link
Contributor Author

@mychris mychris Sep 7, 2020

Choose a reason for hiding this comment

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

Sorry, the explanation of my verification was wrong. I did the following:

size_t ZPlatformAddressOffsetBits() {
const static size_t valid_max_address_bit = probe_valid_max_address_bit();
const size_t max_address_offset_bits = valid_max_address_bit - 3;
return max_address_offset_bits + 1;
}

and allocation of the Java heap succeeded.

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(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.

If we change this line to log_warning_p, then we get this error message in our hs_err files.

#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 ((uintptr_t) result_addr == base_addr) {
// address is valid
max_address_bit = i;
}
if (result_addr != MAP_FAILED) {
munmap(result_addr, page_size);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

If you swap the order of these if-statements you could add a break after 172.

This way the exit of the loop would look like the one you have after msync. You would also be able to get rid of the ' && max_address_bit == 0' check in the for statement.

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_bit = probe_valid_max_address_bit();
const size_t max_address_offset_bits = valid_max_address_bit - 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