8268014: Build failure on SUSE Linux Enterprise Server 11.4 (s390x) due to 'SYS_get_mempolicy' was not declared #4277
Conversation
…ue to 'SYS_get_mempolicy' was not declared
|
👋 Welcome back jiefu! A progress list of the required criteria for merging this PR into |
|
/test |
|
@DamonFool you need to get approval to run the tests in tier1 for commits up until 9fc63d7 |
|
@DamonFool The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
|
@DamonFool This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 54 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Thanks @dholmes-ora . @martin Doerr, please let me know if the build failure can be fixed by this change. |
Okay. |
Ah, using __NR_get_mempolicy is suggested by @MBaesken in the JBS. But I'm not sure if the following change is safe enough since libnuma source code also handles the case when __NR_get_mempolicy isn't defined [1] . I would suggest keeping the same logic for that case. [1] https://github.com/numactl/numactl/blob/master/syscall.c#L29 |
Hi, checking for the existance of SYS_get_mempolicy AND __NR_get_mempolicy is better. Best regards, Matthias |
So are you fine with the latest change? |
|
I would prefer to only use what we need to fix the problem on the "old" system that it was observed rather than trying to generalize this for arbitrary/unknown old versions of Linux. Otherwise we can never know when it is okay to remove this code. David |
This sounds reasonable. |
|
Mailing list message from David Holmes on hotspot-runtime-dev: On 1/06/2021 9:25 pm, Jie Fu wrote:
I don't see that suggestion in the JBS. I do see a greatly simplified #if !defined(SYS_get_mempolicy) && defined(S390) If we know that __NR_get_mempolicy is defined on the system that had the Thanks, |
Updated. |
There was a problem hiding this comment.
This simple fix, as suggested by @MBaesken, is fine with me, as long as it actually fixes the problem.
Thanks,
David
hi, I checked with the larger fix you proposed yesterday. This fixed the night build. |
Thanks @MBaesken . |
Unfortunately, the latest (short) version fails with While __NR_get_mempolicy is on the machine in the system headers; it is not picked up, too bad. |
Thanks @MBaesken . Then, I would prefer the larger one, which had been tested last night. Or shall we only keep the fix for s390? |
There was a problem hiding this comment.
I would prefer to see something smaller and S390 specific as there is only a problem on that platform.
Also is the right check for S390 or s390x ? Different versions of this patch have used each of them. I have no idea what the classification of S390 machines is, so ni idea if these are in fact equivalent. ??
Thanks,
David
The S390 works nicely for both "flavors" it comes from our own autoconf/platform.m4 elif test "x$OPENJDK_$1_CPU" = xs390; then So this looks reliable. The other one #if !defined(SYS_get_mempolicy) && defined(S390) |
|
Right, |
I'm also a bit worrying about this. |
So it sounds really ugly. What do you think of this fix? |
|
Also fine with me. Should we return true or false when SYS_get_mempolicy is not defined? |
This is not the correct number. It is 269 on both s390 and s390x. (236 is The kernel definitions can be a bit difficult to navigate. The glibc sources contain generated lists of system call numbers in the |
No. |
|
Ok, your proposal is fine. Thanks! |
If this is true, there should be a bug in the libnuma source code [1]. [1] https://github.com/numactl/numactl/blob/master/syscall.c#L121 |
Thanks @TheRealMDoerr . |
Updated. |
| #ifdef SYS_get_mempolicy | ||
| int dummy = 0; | ||
| if (syscall(SYS_get_mempolicy, &dummy, NULL, 0, (void*)&dummy, 3) == -1) { | ||
| return false; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
If SYS_get_mempolicy is not defined then surely this must return false?
There was a problem hiding this comment.
If SYS_get_mempolicy is not defined then surely this must return false?
I don't think so.
No SYS_get_mempolicy symbol doesn't mean get_mempolicy syscall is unsupported.
It may still be invoked directly by its syscall number just like what the libnuma does [1].
The logic is:
- if defined(SYS_get_mempolicy) and SYS_get_mempolicy returns -1
then get_mempolicy is surely no supported
so numa_syscall_check() must return fasle - if not defined(SYS_get_mempolicy)
then get_mempolicy may be supported or not
so it can be wrong no matter what numa_syscall_check() returns.
However, for case 2), we can just let numa_syscall_check() return true.
This is fine because all the old JDKs (before jdk17) actually assume that get_mempolicy is always supported by default.
Another reason is that numa_syscall_check() is designed to do its best effort checks of numa related syscalls.
Even numa_syscall_check() return true, it still not guarantee that all numa required syscalls are supported since only get_mempolicy is checked currently.
But this is already enough for dockers with the default configuration.
Also, I think there should be SYS_get_mempolicy definition for almost all modern systems.
So the harmless false positive should be very very rare.
Thanks.
[1] https://github.com/numactl/numactl/blob/master/syscall.c#L121
|
Mailing list message from David Holmes on hotspot-runtime-dev: On 3/06/2021 12:51 pm, Jie Fu wrote:
Okay. So much trouble for so little code. Thanks, |
1 similar comment
|
Mailing list message from David Holmes on hotspot-runtime-dev: On 3/06/2021 12:51 pm, Jie Fu wrote:
Okay. So much trouble for so little code. Thanks, |
Thanks @dholmes-ora . Just enjoy the discussion with you all. |
|
/integrate |
openjdk
bot
added
integrated
|
@DamonFool Since your change was applied there have been 63 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit fbaebd4. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Thanks, I have filed an issue: numactl/numactl#110 |
Thumbs up. |
Hi all,
Please review the patch which fixes the build failure on SUSE Linux Enterprise Server 11.4 (s390x).
@dholmes-ora had reminded me of this bug in [1].
But I missed the fact that ZGC is currently only built on x64 and aarch64.
And I'm sorry for this breakage.
The sys_call numbers of get_mempolicy for different platforms are just copied from the libnuma source code [2].
Thanks.
Best regards,
Jie
[1] #4205 (comment)
[2] https://github.com/numactl/numactl/blob/master/syscall.c
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4277/head:pull/4277$ git checkout pull/4277Update a local copy of the PR:
$ git checkout pull/4277$ git pull https://git.openjdk.java.net/jdk pull/4277/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4277View PR using the GUI difftool:
$ git pr show -t 4277Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4277.diff