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.

Sign up for GitHub

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

Jump to bottom

Implement omrmmap_map_file() using mmap() on z/OS #6940

Merged
merged 3 commits into from
Apr 27, 2023
Merged

Implement omrmmap_map_file() using mmap() on z/OS #6940

merged 3 commits into from
Apr 27, 2023

Conversation

hangshao0
Copy link
Contributor

@hangshao0 hangshao0 commented Mar 30, 2023 •
edited
Loading

  1. Remove omrmmap.c and omrmmap.h from zos390 directory
  2. Implement omrmmap_map_file() using mmap() on z/OS
  3. Add OMRPORT_MMAP_FLAG_ZOS_READ_MAPFILE so that caller can still use
    the old implementation that read the file into the allocated memory
  4. The same as other platforms, use msync() to implement omrmmap_msync() on z/OS.

Signed-off-by: Hang Shao hangshao@ca.ibm.com

@hangshao0
Copy link
Contributor Author

@pshipton

@hangshao0 hangshao0 mentioned this pull request Mar 30, 2023
Merged
keithc-ca
keithc-ca suggested changes Mar 30, 2023
View reviewed changes
port/unix/omrmmap.c Outdated Show resolved Hide resolved
port/unix/omrmmap.c Outdated Show resolved Hide resolved
port/unix/omrmmap.c Outdated Show resolved Hide resolved
port/unix/omrmmap.c Outdated Show resolved Hide resolved
port/unix/omrmmap.c Outdated Show resolved Hide resolved
port/unix/omrmmap.c Outdated Show resolved Hide resolved
@hangshao0 hangshao0 force-pushed the master branch 3 times, most recently from 267eec9 to fedc8d9 Compare March 30, 2023 18:23
@pshipton
Copy link
Contributor

This seems an incompatible change with OpenJ9 due to OMRPORT_MMAP_FLAG_ZOS_READ_MAPFILE. Is it feasible to phase this in, rather than having to merge this concurrently with the OpenJ9 change that uses this flag? i.e. add OMRPORT_MMAP_FLAG_ZOS_READ_MAPFILE in another PR, and after it's merged introduce a new OpenJ9 PR to use it. Afterwards this can be merged.

@pshipton
Copy link
Contributor

Or perhaps just temporarily add OMRPORT_MMAP_FLAG_ZOS_READ_MAPFILE in OpenJ9 if it's not defined, with a value of zero, and pass it in the mmap call. Then eclipse-openj9/openj9#17073 can remove the definition after this is merged.

keithc-ca
keithc-ca reviewed Mar 30, 2023
View reviewed changes
port/unix/omrmmap.c Outdated Show resolved Hide resolved
port/unix/omrmmap.c Outdated Show resolved Hide resolved
port/unix/omrmmap.c Outdated Show resolved Hide resolved
port/unix/omrmmap.c Outdated Show resolved Hide resolved
@hangshao0
Copy link
Contributor Author

Or perhaps just temporarily add OMRPORT_MMAP_FLAG_ZOS_READ_MAPFILE in OpenJ9 if it's not defined, with a value of zero, and pass it in the mmap call. Then eclipse-openj9/openj9#17073 can remove the definition after this is merged.

This should work.

@hangshao0 hangshao0 mentioned this pull request Mar 31, 2023
Merged
hangshao0 added a commit to hangshao0/openj9 that referenced this pull request Mar 31, 2023
@hangshao0
Add temporary define of OMRPORT_MMAP_FLAG_ZOS_READ_MAPFILE
36209cb 
This change is made in order for the related OMR change 
eclipse-omr/omr#6940
to be merged without breaking the builds.

Signed-off-by: Hang Shao <hangshao@ca.ibm.com>
keithc-ca
keithc-ca reviewed Apr 3, 2023
View reviewed changes
port/unix/omrmmap.c Outdated Show resolved Hide resolved
port/unix/omrmmap.c Outdated Show resolved Hide resolved
port/unix/omrmmap.c Show resolved Hide resolved
port/unix/omrmmap.c Outdated Show resolved Hide resolved
port/unix/omrmmap.c Outdated Show resolved Hide resolved
port/unix/omrmmap.c Outdated Show resolved Hide resolved
port/unix/omrmmap.c Outdated Show resolved Hide resolved
port/unix/omrmmap.c Outdated Show resolved Hide resolved
@hangshao0 hangshao0 force-pushed the master branch 3 times, most recently from dff3de0 to d18dae0 Compare April 3, 2023 19:07
keithc-ca
keithc-ca reviewed Apr 6, 2023
View reviewed changes
port/unix/omrmmap.c Outdated Show resolved Hide resolved
@hangshao0 hangshao0 force-pushed the master branch 3 times, most recently from 11ae85f to 6f2c8b4 Compare April 6, 2023 20:14
keithc-ca
keithc-ca reviewed Apr 18, 2023
View reviewed changes
port/unix/omrmmap.c Outdated Show resolved Hide resolved
port/unix/omrmmap.c Outdated Show resolved Hide resolved
@hangshao0
Implement omrmmap_map_file() using mmap() on z/OS
f01a288 
1. The same as other platforms, use mmap() to implement
omrmmap_map_file()
2. Add OMRPORT_MMAP_FLAG_ZOS_READ_MAPFILE so that caller can still use
the old implementation that read the file into the allocated memory.

Signed-off-by: Hang Shao <hangshao@ca.ibm.com>
@hangshao0
Copy link
Contributor Author

All the existing review comments are addressed.

@babsingh
Copy link
Contributor

jenkins build all

@babsingh
Copy link
Contributor

jenkins build zos

4 similar comments
@babsingh
Copy link
Contributor

jenkins build zos

@AdamBrousseau
Copy link
Contributor

jenkins build zos

@AdamBrousseau
Copy link
Contributor

jenkins build zos

@AdamBrousseau
Copy link
Contributor

jenkins build zos

@babsingh
Copy link
Contributor

Waiting for the git issue to be resolved in the zOS PR builds: https://github.ibm.com/runtimes/infrastructure/issues/7935.

@AdamBrousseau
Copy link
Contributor

jenkins build zos

@babsingh
Copy link
Contributor

https://ci.eclipse.org/omr/job/PullRequest-zos_390-64/4077/consoleFull

@hangshao0 There is a failure on zOS:

16:31:28  15: [----------] 15 tests from PortMmapTest
16:31:28  15: omrmmap_test5: launchChildProcess: Failed to start process '/u/user1/workspace/Build/build/omrporttest -child_omrmmap_test5_1'
16:31:28  15: 	portableErrno = 0 portableErrMsg = 
16:31:28  15: /u/user1/workspace/Build/fvtest/porttest/omrmmapTest.cpp line  694: omrmmap_test5 launchChildProcess() failed for pid1!
16:31:28  15: 
16:31:28  15: 		LastErrorNumber: 0
16:31:28  15: 		LastErrorMessage: 
16:31:28  15: 
16:31:28  15: /u/user1/workspace/Build/fvtest/porttest/testHelpers.cpp:109: Failure
16:31:28  15: Value of: 0 == numberFailedTestsInComponent
16:31:28  15:   Actual: false
16:31:28  15: Expected: true
16:31:28  15: Test failed!
16:31:28  15: [  FAILED  ] PortMmapTest.mmap_test5 (1 ms)

@hangshao0
Copy link
Contributor Author

About the failures in #6940 (comment), the test was never run on z/OS before as omrmmap_capabilities() did not return OMRPORT_MMAP_CAPABILITY_WRITE
https://github.com/eclipse/omr/blob/c766c4155b02214afd2db4abf5db93a36017e01c/port/zos390/omrmmap.c#L141

In a previous z/OS build:

15:58:28  15: [----------] 15 tests from PortMmapTest
15:58:28  15: OMRPORT_MMAP_CAPABILITY_WRITE not supported on this platform, bypassing test

The failure in the PR build is the first time we are running this sub-test on z/OS.

@babsingh
Copy link
Contributor

ok, please keep us updated on how the failure will be addressed:

  • update relevant code if it is valid test failure; or
  • exclude the test if the test is incompatible on zOS.

@hangshao0
Copy link
Contributor Author

I need to enable verbose output of the portlib test to see where it failed. @AdamBrousseau Do you know how I can launch omr build on z/OS internally ? I tried one here, but the vmfarm build failed immediately.

@hangshao0
Copy link
Contributor Author

exclude the test if the test is incompatible on zOS.

I have excluded PortMmapTest.mmap_test5 on z/OS. As the child processes cannot be created on z/OS in thetest. Other portlib test rely on child processes are already excluded on z/OS, like PortFileTest2 tests here. I will add a comment in #6542 to mention PortMmapTest.mmap_test5 there.

@hangshao0 hangshao0 mentioned this pull request Apr 26, 2023
Open
@hangshao0
Copy link
Contributor Author

@babsingh You can re-try the z/OS build.

@babsingh
Copy link
Contributor

Before I launch the builds, can you please include the justification for excluding the test from #6940 (comment) and Related: #6542 in the commit message which excludes the test?

@hangshao0
Copy link
Contributor Author

Updated the commit message.

@hangshao0
Exclude PortMmapTest.mmap_test5 on z/OS
4588ae1 
This test relies on child processes to be launched. launchChildProcess()
cannot launch child process in portlib test on z/OS. Other portlib tests
using child process are already excluded, so exclude mmap_test5 as well
on z/OS.

Related to eclipse-omr#6542 

Signed-off-by: Hang Shao <hangshao@ca.ibm.com>
@babsingh
Copy link
Contributor

jenkins build all

@babsingh
Copy link
Contributor

https://dev.azure.com/eclipse-omr/ea4519db-b27e-4d19-a971-f01491f803e3/_apis/build/builds/7450/logs/49

The OSX failure is a known failure #6968 which was probably introduced by #6867.

@babsingh
Copy link
Contributor

https://ci.eclipse.org/omr/job/PullRequest-linux_riscv64_cross/1094/console

Known failure #6665 observed in the Linux riscv64 build; restarting this build.

jenkins build riscv

@babsingh babsingh merged commit a5d1394 into eclipse-omr:master Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pshipton pshipton pshipton approved these changes

@babsingh babsingh babsingh approved these changes

@keithc-ca keithc-ca keithc-ca approved these changes

@charliegracie charliegracie Awaiting requested review from charliegracie

@youngar youngar Awaiting requested review from youngar

@mstoodle mstoodle Awaiting requested review from mstoodle mstoodle is a code owner

@0xdaryl 0xdaryl Awaiting requested review from 0xdaryl 0xdaryl is a code owner

Assignees
No one assigned
Labels
arch:z comp:core comp:port
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hangshao0 @pshipton @babsingh @AdamBrousseau @keithc-ca