Add guarded virtual memory support for z/OS#8279
Conversation
2bc7269 to
e2cdcb2
Compare
|
Waiting for my test builds to finish before removing WIP tag. |
|
Almost all test jobs failed because they were unable to find needed jars. While they don't look related but will try running them locally to confirm. |
|
Don't see any relevant failures in my local tests. Sanity (minus native lib tests) bucket ran without failures. Doing another run of sanity bucket with native lib tests. I'll set the PR ready for review once my local tests finish. Opened an issue to investigate why previous build failed. |
e2cdcb2 to
2343772
Compare
|
My local sanity tests for functional code didn’t have any failures related to this PR. This is ready for review. @dmitripivkine, @joransiu / @r30shah |
| rc = omrremove_guard((void *)alignedAddress, numSegments); | ||
| } | ||
|
|
||
| if (0 == rc || ( 4 == rc && identifier->pageSize == FOUR_K)) { |
There was a problem hiding this comment.
- Would it be better to define return codes? As minimum we should add comment to explain what code 4 means.
- Please put brackets around logical conditions, do not rely on operations order. Also put
FOUR_Kat the left:
if ((0 == rc) || ((4 == rc) && (FOUR_K == Identifier->pageSize))) {
There was a problem hiding this comment.
I would not mention adding extra brackets to the code below to not pollute it too much, please review.
There was a problem hiding this comment.
Good point, I have updated the method comment with return code description
| } | ||
|
|
||
| if (0 == rc || ( 4 == rc && identifier->pageSize == FOUR_K)) { | ||
| ptr = (void*)alignedAddress; |
There was a problem hiding this comment.
please add space between void and star.
| /* Skip low memory allocation (malloc31) when OMRPORT_VMEM_MEMORY_MODE_GUARDED flag */ | ||
| /* is set to avoid MEMLIMIT issues. malloc31() cannot provide guarded memory and */ | ||
| /* counts against MEMLIMIT. When OMRPORT_VMEM_MEMORY_MODE_GUARDED is set, route all */ | ||
| /* allocations directly to above-the-bar allocation which avoids MEMLIMIT issues. */ |
There was a problem hiding this comment.
please use multiline comment format:
/*
* ...
* ...
*/
2b2a8b6 to
ee6aa1f
Compare
| @@ -231,12 +251,42 @@ void * | |||
| omrvmem_commit_memory(struct OMRPortLibrary *portLibrary, void *address, uintptr_t byteAmount, struct J9PortVmemIdentifier *identifier) | |||
| { | |||
| void *ptr = NULL; | |||
| #if defined(OMR_ENV_DATA64) | |||
| uintptr_t numSegments = 0; | |||
There was a problem hiding this comment.
There is no need to define this variable for full function scope. It can be define inside if statement close to use.
f955b57 to
5c7ef76
Compare
| intptr_t rc = -1; | ||
| uintptr_t alignedByteAmount = ROUND_UP_TO_POWEROF2(byteAmount, ONE_M); | ||
| uintptr_t alignedAddress = ROUND_DOWN_TO_POWEROF2((uintptr_t)address, ONE_M); | ||
| if (rangeIsValid(identifier, (void *)alignedAddress, alignedByteAmount)) { |
There was a problem hiding this comment.
I think this check is not necessary. We can rely on guarded allocation pattern: actual allocation for guarded memory happen to be done in 1m pages. So rounding up to 1m should be valid regardless requested size. And yes, we need comment about this here explaining why rounding to 1m is necessary. The same point is applicable to decommit function as well.
5c7ef76 to
d9d4d5b
Compare
| if (0 == result && OMR_ARE_ANY_BITS_SET(identifier->mode, OMRPORT_VMEM_MEMORY_MODE_GUARDED)) { | ||
| uintptr_t alignedByteAmount = ROUND_DOWN_TO_POWEROF2(byteAmount, ONE_M); | ||
| uintptr_t alignedAddress = ROUND_UP_TO_POWEROF2((uintptr_t)address, ONE_M); | ||
| if (rangeIsValid(identifier, (void *)alignedAddress, alignedByteAmount)) { |
There was a problem hiding this comment.
Same comment as for commit above
Add guarded virtual memory support to the z/OS vmem path so that object heap and sparse virtual memory reservations can use guarded storage for Balanced GC. This reduces MEMLIMIT pressure by keeping reserved regions guarded until they are committed for use. Introduce the OMRPORT_VMEM_MEMORY_MODE_GUARDED flag (currently supported only for Balanced GC) to indicate guarding of VLHGC heap and sparse virtual memory reservations. Guarded 1 MB pageable and 4 KB above-the-bar allocations are supported. Guard and unguard operations are implemented using IARV64 guarded storage support. Signed-off-by: Shubham Verma <shubhamv.sv@gmail.com>
d9d4d5b to
eb891af
Compare
| __asm(" IARV64 PLISTVER=MAX,MF=(L,SGETSTOR)":"DS"(wgetstor)); | ||
|
|
||
| segments = *numMBSegments; | ||
| wgetstor = ngetstor; |
There was a problem hiding this comment.
The ngetstor should be referencing the sgetstor you declared globally for this function.
| */ | ||
| void *omrallocate_1M_pageable_pages_guarded_above_bar(int *numMBSegments, int *userExtendedPrivateAreaMemoryType, const char *ttkn) { | ||
| long segments; | ||
| long origin; |
There was a problem hiding this comment.
We should initialize these variables with a default value.
|
|
||
| switch (useMemoryType) { | ||
| case ZOS64_VMEM_ABOVE_BAR_GENERAL: | ||
| break; |
There was a problem hiding this comment.
Since iarv64_rc is initialized to 0, won't we return a potentially garbage (uninitialized) value of origin?
| */ | ||
| void *omrallocate_4K_pages_guarded_in_userExtendedPrivateArea(int *numMBSegments, int *userExtendedPrivateAreaMemoryType, const char *ttkn) { | ||
| long segments; | ||
| long origin; |
There was a problem hiding this comment.
These variables should be initialized.
| __asm(" IARV64 PLISTVER=MAX,MF=(L,EGETSTOR)":"DS"(wgetstor)); | ||
|
|
||
| segments = *numMBSegments; | ||
| wgetstor = rgetstor; |
There was a problem hiding this comment.
rgetstor should be egetstor.
|
@dmitripivkine I have updated the PR based on your feedback. It's ready for another review. |
| @@ -534,7 +602,7 @@ reservePagesAboveBar(struct OMRPortLibrary *portLibrary, J9PortVmemIdentifier *i | |||
| uintptr_t userExtendedPrivateAreaMemoryType = ZOS64_VMEM_ABOVE_BAR_GENERAL; | |||
| uintptr_t userExtendedPrivateAreaMemoryMax = 0; | |||
|
|
|||
| if (OMRPORT_VMEM_ZOS_USE2TO32G_AREA == (OMRPORT_VMEM_ZOS_USE2TO32G_AREA & options)) { | |||
| if (OMR_ARE_ANY_BITS_SET(mode, OMRPORT_VMEM_MEMORY_MODE_GUARDED) || OMRPORT_VMEM_ZOS_USE2TO32G_AREA == (OMRPORT_VMEM_ZOS_USE2TO32G_AREA & options)) { | |||
There was a problem hiding this comment.
you can use OMR_ARE_ANY_BITS_SET for OMRPORT_VMEM_ZOS_USE2TO32G_AREA flag in options as well for consistency
| /* omrvmem_support_above_bar.s */ | ||
| #pragma linkage(omrallocate_4K_pages_in_userExtendedPrivateArea,OS_NOSTACK) | ||
| void * omrallocate_4K_pages_in_userExtendedPrivateArea(int numMBSegments, int userExtendedPrivateAreaMemoryType, const char * ttkn); | ||
|
|
||
| /* omrvmem_support_above_bar.s */ | ||
| #pragma linkage(omrallocate_4K_pages_guarded_in_userExtendedPrivateArea,OS_NOSTACK) | ||
| void * omrallocate_4K_pages_guarded_in_userExtendedPrivateArea(int numMBSegments, int userExtendedPrivateAreaMemoryType, const char * ttkn); |
There was a problem hiding this comment.
There is no need to have extra space after star (two occurrences). You can fix formatting in the previous pragma as well if you like.
| @@ -280,6 +328,26 @@ omrvmem_decommit_memory(struct OMRPortLibrary *portLibrary, void *address, uintp | |||
| case OMRPORT_VMEM_RESERVE_USED_J9ALLOCATE_4K_PAGES_ABOVE_BAR: /* FALLTHROUGH */ | |||
| case OMRPORT_VMEM_RESERVE_USED_MOSERVICES: | |||
| result = omrdiscard_data((void *)address, byteAmount >> ZOS_REAL_FRAME_SIZE_SHIFT); | |||
| if (0 == result && OMR_ARE_ANY_BITS_SET(identifier->mode, OMRPORT_VMEM_MEMORY_MODE_GUARDED)) { | |||
There was a problem hiding this comment.
please add extra brackets around logical conditions
| /* determine number of 1MB segments required */ | ||
| uintptr_t numSegments = alignedByteAmount / ONE_M; | ||
| result = omradd_guard((void *)address, numSegments); | ||
| if (4 == result && FOUR_K == identifier->pageSize) { |
There was a problem hiding this comment.
please add extra brackets around logical conditions
| uintptr_t numSegments = alignedByteAmount / ONE_M; | ||
| rc = omrremove_guard((void *)alignedAddress, numSegments); | ||
|
|
||
| if (0 == rc || (4 == rc && FOUR_K == identifier->pageSize)) { |
There was a problem hiding this comment.
please add extra brackets around logical conditions
Add guarded virtual memory support to the z/OS vmem path so that object heap and sparse virtual memory reservations can use guarded storage for Balanced GC. This reduces MEMLIMIT pressure by keeping reserved regions guarded until they are committed for use.
Introduce the OMRPORT_VMEM_MEMORY_MODE_GUARDED flag (currently supported only for Balanced GC) to indicate guarding of VLHGC heap and sparse virtual memory reservations. Guarded 1 MB pageable and 4 KB above-the-bar allocations are supported. Guard and unguard operations are implemented using IARV64 guarded storage support.
Signed-off-by: Shubham Verma shubhamv.sv@gmail.com