Expand omrvmemtest on Z/OS #4918

AlenBadel · 2020-03-10T15:04:07Z

Expand omrvmemtest on Z/OS

Expanding the current tests to add cases where OMRPORT_VMEM_PAGE_FLAG_PREFER_PAGEABLE may be used.

These test cases include:

Default large page when pageable is preferred
This test will return a pageable 1M default large page when available, and 1M fixed pages otherwise.
If no large pages are configured an invalid large page state will be set.
Default large page when pageable is preferred, and mode is OMRPORT_VMEM_MEMORY_MODE_EXECUTE
This test will return a pageable 1M default large page when available, otherwise an invalid large page state will be set.
Verify 1M large pages when pageable is preferred
This test will verify that 1M large pages are supported if there exists a pageable, or fixed 1M pages are available.
Verify 1M large pages when pageable is preferred, and mode is OMRPORT_VMEM_MEMORY_MODE_EXECUTE
This test will verify that 1M large pages are supported only if 1M pageable is configured.

For more info on OMRPORT_VMEM_PAGE_FLAG_PREFER_PAGEABLE, see Pull Request #4762.

Signed-off-by: AlenBadel [email protected]

AlenBadel · 2020-03-10T15:05:17Z

PR enables tests for functionality implemented here #4762

AlenBadel · 2020-03-10T15:06:23Z

Running some additional sanity on my end.

FYI @gita-omr

@babsingh Could you please review this?

gita-omr · 2020-03-10T15:45:30Z

Looks right on a high level, but I will let @babsingh review in detail. BTW: why do we have the same code in openj9?

AlenBadel · 2020-03-10T16:00:48Z

why do we have the same code in openj9?

I'm not entirely sure why the downstream project OpenJ9 has j9vmemtest, since this test can be used instead. That's certainly a discussion that can be had in OpenJ9.

mstoodle · 2020-03-10T17:47:40Z

Stupid question: I'm not sure I understand the "on Z/OS" part of this pull request. The changes don't appear to be in a z/OS specific file or under a z/OS specific macro. Isn't this really just adding some new scenarios to omrvmemtests ?

AlenBadel · 2020-03-10T17:52:48Z

Stupid question: I'm not sure I understand the "on Z/OS" part of this pull request. The changes don't appear to be in a z/OS specific file or under a z/OS specific macro. Isn't this really just adding some new scenarios to omrvmemtests ?

Each platform has it's own implementation of omrvmem_testFindValidPageSize_impl which run these tests. These tests are only added under the Z/OS implementation since the related change only affected Z/OS.

#elif defined(J9ZOS390)
static int
omrvmem_testFindValidPageSize_impl(struct OMRPortLibrary *portLibrary, const char *testName)
{
...

babsingh · 2020-03-10T20:20:17Z

@babsingh Could you please review this?

Please refer to eclipse-openj9/openj9#8791 (review) for the review since this pull request has near identical code to eclipse-openj9/openj9#8791.

mstoodle · 2020-03-10T20:51:49Z

I understand this code exists in OMR and OpenJ9. It seems like this change is being reviewed at OpenJ9, so I'm going to close this PR until everything is worked out over there.

When there's a change to be made and reviewed at OMR, we will welcome it.

AlenBadel · 2020-03-16T15:51:21Z

OpenJ9 uses omrporttest, and it's been decided to rely on the OMR port test implementation. @mstoodle I suggest that this PR be re-opened as these changes are required to verify #4762.

mstoodle · 2020-03-16T16:00:26Z

Expecting the review and testing to happen in OMR then. Thanks @AlenBadel .

AlenBadel · 2020-03-16T22:28:45Z

@fjeremic Could you kindly trigger a run sanity on Z/OS? Thanks!

fjeremic · 2020-03-16T23:00:07Z

@genie-omr build zos

AlenBadel · 2020-03-17T15:02:22Z

@fjeremic Would there be any further testing that could be triggered via PR?

fjeremic · 2020-03-17T21:14:30Z

@fjeremic Would there be any further testing that could be triggered via PR?

Yeah. The test is common so we have to test all platforms. Then it's off to someone with port library experience for review. My suggestion is one of @rwy0717 or @youngar.

@genie-omr build all

AlenBadel · 2020-03-17T22:16:15Z

Yeah. The test is common so we have to test all platforms. Then it's off to someone with port library experience for review. My suggestion is one of @rwy0717 or @youngar.

I was looking for further sanity that can be launched via PR. I was able to find that internally, and can say that everything looks clear on my end.

It must be brought up that there are a lot of references to OpenJ9 in this file. As each test is named after an OpenJ9 paging option variant.
@mstoodle Would you like to see an issue raised to track the removal of these?

fjeremic · 2020-03-17T22:19:02Z

@genie-omr build all

mstoodle · 2020-03-18T13:43:14Z

Thanks, @AlenBadel , please raise an issue (though I expect it doesn't need to be tackled with urgency).

AlenBadel · 2020-03-18T14:37:31Z

@babsingh Could you please pass through the code one last time and give me a final pass? Thanks!

AlenBadel · 2020-03-18T14:49:15Z

Thanks, @AlenBadel , please raise an issue (though I expect it doesn't need to be tackled with urgency).

Created Issue: #4947

babsingh

Code lgtm. But, the documentation can be improved. A reader may not understand the reference to OMRPORT_VMEM_PAGE_FLAG_PREFER_PAGEABLE. In the commit message, you can ...

summarize the new feature and four new testcases; and
add a link to your new feature for more details Add capability to give preference to pageable large pages on Z/OS #4762.

AlenBadel · 2020-03-18T16:45:36Z

Thanks for the review Babneet. I agree with your suggestion and will be amending feature related functionality to the commit.

Just to note as I make further changes that #4918 (comment) all builds succeeded.

Expanding the current tests to add cases where OMRPORT_VMEM_PAGE_FLAG_PREFER_PAGEABLE may be used. These test cases include: 1) Default large page when pageable is preferred This test will return a pageable 1M default large page when available, and 1M fixed pages otherwise. If no large pages are configured an invalid large page state will be set. 2) Default large page when pageable is preferred, and mode is OMRPORT_VMEM_MEMORY_MODE_EXECUTE This test will return a pageable 1M default large page when available, otherwise an invalid large page state will be set. 3) Verify 1M large pages when pageable is preferred This test will verify that 1M large pages are supported if there exists a pageable, or fixed 1M pages are available. 4) Verify 1M large pages when pageable is preferred, and mode is OMRPORT_VMEM_MEMORY_MODE_EXECUTE This test will verify that 1M large pages are supported only if 1M pageable is configured. For more info on OMRPORT_VMEM_PAGE_FLAG_PREFER_PAGEABLE, see Pull Request eclipse-omr#4762. Signed-off-by: AlenBadel <[email protected]>

AlenBadel · 2020-03-18T17:18:12Z

@mstoodle Please advise if you would like further reviewing.

mstoodle · 2020-03-19T22:17:05Z

@genie-omr build all

mstoodle · 2020-03-19T23:11:47Z

It looks fine to me, but this is not an area I consider myself an expert in.

@rwy0717 or @youngar or @charliegracie would one of you please be the committer here?

mstoodle

Changes look ok to me, but would prefer a committer close to this component take the committer role here.

AlenBadel · 2020-03-24T00:55:27Z

@rwy0717 or @youngar or @charliegracie would one of you please be the committer here?

I would appreciate if someone could take a quick look at this, and ultimately get it merged.

rwy7 · 2020-03-24T19:46:37Z

Normally I ask that, for new tests, authors use gtest assertions and finer grained test cases, rather than reporting through the older test logging API. However, since this test makes use of helpers built on top of these logger APIs, I think this integrates nicely into the suite.

Since I don't see any comments that need addressing, I'm going to go ahead and merge this.

Thanks for the new test :)

AlenBadel requested review from charliegracie and mstoodle as code owners March 10, 2020 15:04

fjeremic added comp:port comp:test labels Mar 10, 2020

fjeremic mentioned this pull request Mar 10, 2020

Expand j9vmemtest on Z/OS eclipse-openj9/openj9#8791

Closed

mstoodle closed this Mar 10, 2020

AlenBadel mentioned this pull request Mar 12, 2020

Supporting Hotspot Large Page Options eclipse-openj9/openj9#8671

Open

mstoodle reopened this Mar 16, 2020

AlenBadel force-pushed the omrvmem branch 2 times, most recently from a81fc20 to b8acc82 Compare March 16, 2020 22:27

AlenBadel force-pushed the omrvmem branch from 6aa97d8 to 0f45c88 Compare March 17, 2020 22:08

babsingh approved these changes Mar 18, 2020

View reviewed changes

AlenBadel force-pushed the omrvmem branch from 0f45c88 to 87418fa Compare March 18, 2020 16:58

mstoodle approved these changes Mar 19, 2020

View reviewed changes

rwy7 approved these changes Mar 24, 2020

View reviewed changes

rwy7 self-assigned this Mar 24, 2020

rwy7 merged commit ea80e8c into eclipse-omr:master Mar 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand omrvmemtest on Z/OS #4918

Expand omrvmemtest on Z/OS #4918

AlenBadel commented Mar 10, 2020 •

edited

Loading

AlenBadel commented Mar 10, 2020 •

edited

Loading

AlenBadel commented Mar 10, 2020 •

edited

Loading

gita-omr commented Mar 10, 2020

AlenBadel commented Mar 10, 2020

mstoodle commented Mar 10, 2020

AlenBadel commented Mar 10, 2020

babsingh commented Mar 10, 2020

mstoodle commented Mar 10, 2020

AlenBadel commented Mar 16, 2020

mstoodle commented Mar 16, 2020

AlenBadel commented Mar 16, 2020

fjeremic commented Mar 16, 2020

AlenBadel commented Mar 17, 2020

fjeremic commented Mar 17, 2020

AlenBadel commented Mar 17, 2020

fjeremic commented Mar 17, 2020

mstoodle commented Mar 18, 2020

AlenBadel commented Mar 18, 2020

AlenBadel commented Mar 18, 2020

babsingh left a comment

AlenBadel commented Mar 18, 2020

AlenBadel commented Mar 18, 2020

mstoodle commented Mar 19, 2020

mstoodle commented Mar 19, 2020

mstoodle left a comment

AlenBadel commented Mar 24, 2020

rwy7 commented Mar 24, 2020

Expand omrvmemtest on Z/OS #4918

Expand omrvmemtest on Z/OS #4918

Conversation

AlenBadel commented Mar 10, 2020 • edited Loading

AlenBadel commented Mar 10, 2020 • edited Loading

AlenBadel commented Mar 10, 2020 • edited Loading

gita-omr commented Mar 10, 2020

AlenBadel commented Mar 10, 2020

mstoodle commented Mar 10, 2020

AlenBadel commented Mar 10, 2020

babsingh commented Mar 10, 2020

mstoodle commented Mar 10, 2020

AlenBadel commented Mar 16, 2020

mstoodle commented Mar 16, 2020

AlenBadel commented Mar 16, 2020

fjeremic commented Mar 16, 2020

AlenBadel commented Mar 17, 2020

fjeremic commented Mar 17, 2020

AlenBadel commented Mar 17, 2020

fjeremic commented Mar 17, 2020

mstoodle commented Mar 18, 2020

AlenBadel commented Mar 18, 2020

AlenBadel commented Mar 18, 2020

babsingh left a comment

Choose a reason for hiding this comment

AlenBadel commented Mar 18, 2020

AlenBadel commented Mar 18, 2020

mstoodle commented Mar 19, 2020

mstoodle commented Mar 19, 2020

mstoodle left a comment

Choose a reason for hiding this comment

AlenBadel commented Mar 24, 2020

rwy7 commented Mar 24, 2020

AlenBadel commented Mar 10, 2020 •

edited

Loading

AlenBadel commented Mar 10, 2020 •

edited

Loading

AlenBadel commented Mar 10, 2020 •

edited

Loading