Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v4]
On Fri, 15 Dec 2023 06:22:39 GMT, Thomas Stuefe wrote:
>> Joachim Kern has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> followed the proposals
>
> src/hotspot/os/aix/os_aix.cpp line 1129:
>
>> 1127:
>> 1128: // get the library search path burned in to the executable file during
>> linking
>> 1129: // If the libpath cannot be retrieved return an empty path
>
> This is new. Is this complexity needed, if yes, why? Don't see a comment, may
> have missed it.
Also, why are we parsing xcoff32 headers in there? AIX OpenJDK will always be
64-bit. So, you can replace the whole xcoff32 section with assert( f_magic ==
U802TOCMAGIC, ..). The function becomes a lot simpler then.
> src/hotspot/os/aix/os_aix.cpp line 1132:
>
>> 1130: static const char* rtv_linkedin_libpath() {
>> 1131: static char buffer[4096];
>> 1132: static const char* libpath = 0;
>
> If your intent is to return an empty buffer if there is no contained libpath,
> I would just:
>
>
> static const char* libpath = "";
>
> then you can always just return libpath.
But looking at the using code, returning NULL in case there is no contained
libpath would be actually easier, see below.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427609926
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1427639138
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v4]
On Tue, 12 Dec 2023 14:05:48 GMT, Joachim Kern wrote:
>> On AIX, repeated calls to dlopen referring to the same shared library may
>> result in different, unique dl handles to be returned from libc. In that it
>> differs from typical libc implementations that cache dl handles.
>>
>> This causes problems in the JVM with code that assumes equality of handles.
>> One such problem is in the JVMTI agent handler. That problem was fixed with
>> a local fix to said handler
>> ([JDK-8315706](https://bugs.openjdk.org/browse/JDK-8315706)). However, this
>> fix causes follow-up problems since it assumes that the file name passed to
>> `os::dll_load()` is the file that has been opened. It prevents efficient,
>> os_aix.cpp-local workarounds for other AIX issues like the *.so/*.a duality.
>> See [JDK-8320005](https://bugs.openjdk.org/browse/JDK-8320005). As such, it
>> is a hack that causes other, more uglier hacks to follow (see discussion of
>> https://github.com/openjdk/jdk/pull/16604).
>>
>> We propose a different, cleaner way of handling this:
>>
>> - Handle this entirely inside the AIX versions of os::dll_load and
>> os::dll_unload.
>> - Cache dl handles; repeated opening of a library should return the cached
>> handle.
>> - Increase handle-local ref counter on open, Decrease it on close
>> - Make sure calls to os::dll_load are matched to os::dll_unload (See
>> [JDK-8320830](https://bugs.openjdk.org/browse/JDK-8320830)).
>>
>> This way we mimic dl handle equality as it is implemented on other
>> platforms, and this works for all callers of os::dll_load.
>
> Joachim Kern has updated the pull request incrementally with one additional
> commit since the last revision:
>
> followed the proposals
Is this libpath parsing code copied from the R3 kernel? If yes, pls make sure
there are no licensing issues.
src/hotspot/os/aix/os_aix.cpp line 206:
> 204: constexpr int max_handletable = 1024;
> 205: static int g_handletable_used = 0;
> 206: static struct handletableentry g_handletable[max_handletable] = {{0, 0,
> 0, 0}};
I would move all that new and clearly delineated dlopen stuff into an own file,
e.g. dlopen_aix.cpp or porting_aix.cpp (in porting_aix.cpp, we already have
wrappers for other functions). os_aix.cpp is already massive.
src/hotspot/os/aix/os_aix.cpp line 1129:
> 1127:
> 1128: // get the library search path burned in to the executable file during
> linking
> 1129: // If the libpath cannot be retrieved return an empty path
This is new. Is this complexity needed, if yes, why? Don't see a comment, may
have missed it.
src/hotspot/os/aix/os_aix.cpp line 1131:
> 1129: // If the libpath cannot be retrieved return an empty path
> 1130: static const char* rtv_linkedin_libpath() {
> 1131: static char buffer[4096];
This coding has some issues:
- a generic char buffer is not a good idea. Forces you to do casts all over the
place, and introduces alignment issues with unaligned char buffer. Which I
assume is the reason for all the separate memcpy-into-structures below. I would
just read into the structures directly.
- you need to check the return codes for fread to make sure you read the number
of bytes expected, lest you work with uninitialized memory and maybe to handle
sporadic EINTR.
- I don't get all the separate "SZ" macros. They must be equal to
sizeof(structure), right, otherwise you get buffer overruns or work with
uninitialized memory?
Proposal: add a local wrapper function like this:
template
static bool my_checked_fread(FILE* f, T* out) {
// read sizeof(T) from f.
// Check return code.
// Return bool if sizeof(T) bytes were read.
e.g. in a very trivial form:
int bytesread = fread(out, sizeof(T), 1, f);
return bytesread == sizeof(T);
}
and use it in your code like this:
struct xcoff64 the_xcoff64;
struct scn64 the_scn64;
struct ldr64 the_ldr64;
if (!my_checked_fread(f, _xcoff64)) {
assert?
}
...
if (!my_checked_fread(f, _ldr64) {
.. handle error
}
src/hotspot/os/aix/os_aix.cpp line 1132:
> 1130: static const char* rtv_linkedin_libpath() {
> 1131: static char buffer[4096];
> 1132: static const char* libpath = 0;
If your intent is to return an empty buffer if there is no contained libpath, I
would just:
static const char* libpath = "";
then you can always just return libpath.
src/hotspot/os/aix/os_aix.cpp line 1135:
> 1133:
> 1134: if (libpath)
> 1135: return libpath;
{ }
src/hotspot/os/aix/os_aix.cpp line 1137:
> 1135: return libpath;
> 1136:
> 1137: char pgmpath[32+1];
Will overflow if pid_t is 64bit. Give it a larger size; after all, you are
giving buffer 4K above, so you are not overly concerned with saving stack space.
src/hotspot/os/aix/os_aix.cpp line 1146:
> 1144: fread(buffer, 1, FILHSZ_64 + _AOUTHSZ_EXEC_64, f);
> 1145:
> 1146: if (((struct filehdr*)buffer)->f_magic == U802TOCMAGIC ) {
as stated above, I don't think this section is needed.
src/hotspot/os/aix/os_aix.cpp line 1170:
>
Re: RFR: 8321971: Improve the user name detection logic in perfMemory get_user_name_slow
On Thu, 14 Dec 2023 10:13:51 GMT, Jaikiran Pai wrote:
> Can I please get a review of this change which proposes to improve the code
> in `get_user_name_slow` function, which is used to identify the target JVM
> owner's user name? This addresses https://bugs.openjdk.org/browse/JDK-8321971.
>
> As noted in that JBS issue, in its current form, the nested loop ends up
> iterating over the directory contents of `hsperfdata_xxx` directory and then
> for each iteration it checks if the name of the entry matches the pid. This
> iteration shouldn't be needed and instead one could look for a file named
> `` within that directory.
>
> No new test has been added, given the nature of this change. Existing tier1,
> tier2, tier3 and svc_tools tests pass with this change on Linux, Windows and
> macosx.
Just some passing comments. I'm not familiar enough with the existing logic.
src/hotspot/os/posix/perfMemory_posix.cpp line 434:
> 432: // shared memory region for the given user name and vmid.
> 433: //
> 434: // the caller is expected to free the allocated memory.
Nit: comments with punctuation (e.g. terminating period) should be written as
sentences and start with a capital letter e.g.
// either do this
// Or this.
Thanks
src/hotspot/os/posix/perfMemory_posix.cpp line 606:
> 604: }
> 605: // skip over files that are not regular files.
> 606: if (!S_ISREG(statbuf.st_mode)) {
These appear to be able to be combined into a single if block.
src/hotspot/os/posix/perfMemory_posix.cpp line 612:
> 610: continue;
> 611: }
> 612: FREE_C_HEAP_ARRAY(char, filename);
If you move this to immediately after lstat then you don't need it in the
if-block
-
PR Review: https://git.openjdk.org/jdk/pull/17104#pullrequestreview-1783152470
PR Review Comment: https://git.openjdk.org/jdk/pull/17104#discussion_r1427568288
PR Review Comment: https://git.openjdk.org/jdk/pull/17104#discussion_r1427589543
PR Review Comment: https://git.openjdk.org/jdk/pull/17104#discussion_r1427590464
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v5]
On Thu, 14 Dec 2023 22:35:18 GMT, Serguei Spitsyn wrote:
>> src/java.base/share/classes/java/lang/VirtualThread.java line 1043:
>>
>>> 1041: notifyJvmtiDisableSuspend(true);
>>> 1042: try {
>>> 1043: // include the carrier thread state and name when
>>> mounted
>>
>> This one too, can you move the comment to before the
>> notifyJvmtiDisableSuspend.
>
> Moved both comments out of try blocks.
> What about this one (it seems we would wont to do the same) ? :
>
> notifyJvmtiDisableSuspend(true);
> try {
> // unpark carrier thread when pinned
> synchronized (carrierThreadAccessLock()) {
> Thread carrier = carrierThread;
> if (carrier != null && ((s = state()) == PINNED || s
> == TIMED_PINNED)) {
> U.unpark(carrier);
> }
> }
> } finally {
> notifyJvmtiDisableSuspend(false);
> }
Moved 3 comments out of try blocks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427386103
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v6]
> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1 > time frame. > It is fixing a deadlock issue between `VirtualThread` class critical sections > with the `interruptLock` (in methods: `unpark()`, `interrupt()`, > `getAndClearInterrupt()`, `threadState()`, `toString()`), > `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms. > The deadlocking scenario is well described by Patricio in a bug report > comment. > In simple words, a virtual thread should not be suspended during > 'interruptLock' critical sections. > > The fix is to record that a virtual thread is in a critical section > (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about > begin/end of critical section. > This bit is used in `HandshakeState::get_op_for_self()` to filter out any > `HandshakeOperation` if a target `JavaThread` is in a critical section. > > Some of new notifications with `notifyJvmtiSync()` method is on a performance > critical path. It is why this method has been intrincified. > > New test was developed by Patricio: > `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` > The test is very nice as it reliably in 100% reproduces the deadlock without > the fix. > The test is never failing with this fix. > > Testing: > - tested with newly added test: > `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` > - tested with mach5 tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: moved a couple of comments out of try blocks - Changes: - all: https://git.openjdk.org/jdk/pull/17011/files - new: https://git.openjdk.org/jdk/pull/17011/files/ad990422..917dc724 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17011=05 - incr: https://webrevs.openjdk.org/?repo=jdk=17011=04-05 Stats: 6 lines in 1 file changed: 3 ins; 3 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17011.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17011/head:pull/17011 PR: https://git.openjdk.org/jdk/pull/17011
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v5]
On Thu, 14 Dec 2023 19:50:00 GMT, Alan Bateman wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: moved notifyJvmtiDisableSuspend(true) out of try-block
>
> src/java.base/share/classes/java/lang/VirtualThread.java line 1043:
>
>> 1041: notifyJvmtiDisableSuspend(true);
>> 1042: try {
>> 1043: // include the carrier thread state and name when
>> mounted
>
> This one too, can you move the comment to before the
> notifyJvmtiDisableSuspend.
Moved both comments out of try blocks.
What about this one (it seems we would wont to do the same) ? :
notifyJvmtiDisableSuspend(true);
try {
// unpark carrier thread when pinned
synchronized (carrierThreadAccessLock()) {
Thread carrier = carrierThread;
if (carrier != null && ((s = state()) == PINNED || s ==
TIMED_PINNED)) {
U.unpark(carrier);
}
}
} finally {
notifyJvmtiDisableSuspend(false);
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427373522
Integrated: 8322065: Initial nroff manpage generation for JDK 23
On Thu, 14 Dec 2023 05:46:01 GMT, David Holmes wrote: > Updated the version to 23-ea and year to 2024. > > This initial generation also picks up the unpublished changes from: > > - [JDK-8302233](https://bugs.openjdk.org/browse/JDK-8302233) (keytool & > jarsigner) > - [JDK-8290702](https://bugs.openjdk.org/browse/JDK-8290702) (javadoc) (JDK > 23 backport) > > Thanks This pull request has now been integrated. Changeset: 692be577 Author:David Holmes URL: https://git.openjdk.org/jdk/commit/692be577385844bf00a01ff10e390e014191569f Stats: 193 lines in 27 files changed: 36 ins; 51 del; 106 mod 8322065: Initial nroff manpage generation for JDK 23 Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/17101
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v5]
On Thu, 14 Dec 2023 18:26:55 GMT, Serguei Spitsyn wrote:
>> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1
>> time frame.
>> It is fixing a deadlock issue between `VirtualThread` class critical
>> sections with the `interruptLock` (in methods: `unpark()`, `interrupt()`,
>> `getAndClearInterrupt()`, `threadState()`, `toString()`),
>> `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms.
>> The deadlocking scenario is well described by Patricio in a bug report
>> comment.
>> In simple words, a virtual thread should not be suspended during
>> 'interruptLock' critical sections.
>>
>> The fix is to record that a virtual thread is in a critical section
>> (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about
>> begin/end of critical section.
>> This bit is used in `HandshakeState::get_op_for_self()` to filter out any
>> `HandshakeOperation` if a target `JavaThread` is in a critical section.
>>
>> Some of new notifications with `notifyJvmtiSync()` method is on a
>> performance critical path. It is why this method has been intrincified.
>>
>> New test was developed by Patricio:
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock`
>> The test is very nice as it reliably in 100% reproduces the deadlock without
>> the fix.
>> The test is never failing with this fix.
>>
>> Testing:
>> - tested with newly added test:
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock`
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: moved notifyJvmtiDisableSuspend(true) out of try-block
src/java.base/share/classes/java/lang/VirtualThread.java line 918:
> 916: notifyJvmtiDisableSuspend(true);
> 917: try {
> 918: // if mounted then return state of carrier thread
Can you move this comment line to before the notifyJvmtiDisableSuspend(true)?
src/java.base/share/classes/java/lang/VirtualThread.java line 1043:
> 1041: notifyJvmtiDisableSuspend(true);
> 1042: try {
> 1043: // include the carrier thread state and name when
> mounted
This one too, can you move the comment to before the notifyJvmtiDisableSuspend.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427198296
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427198673
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 18:24:16 GMT, Serguei Spitsyn wrote: > Thank you, Alan. Fixed now. I believe, all your suggestions have been > addressed now. Thanks, it looks much better now. - PR Comment: https://git.openjdk.org/jdk/pull/17011#issuecomment-1856485757
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v4]
On Thu, 14 Dec 2023 18:03:00 GMT, Alan Bateman wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: 1) replace CriticalLock with DisableSuspend; 2) minor tweaks
>
> src/java.base/share/classes/java/lang/VirtualThread.java line 1042:
>
>> 1040: if (carrier != null) {
>> 1041: try {
>> 1042: notifyJvmtiDisableSuspend(true);
>
> this one too.
Thanks. All cases fixed now.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427095561
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 18:04:02 GMT, Alan Bateman wrote: > Okay with me. We'll need to move the notifyJvmtiDisableSuspend(true) to > before the try in all cases, I've pointed out the cases that we missed. Thank you, Alan. Fixed now. - PR Comment: https://git.openjdk.org/jdk/pull/17011#issuecomment-1856366484
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 12:16:34 GMT, Serguei Spitsyn wrote: >> src/hotspot/share/runtime/javaThread.hpp line 320: >> >>> 318: bool _is_in_VTMS_transition; // thread is >>> in virtual thread mount state transition >>> 319: bool _is_in_tmp_VTMS_transition; // thread is >>> in temporary virtual thread mount state transition >>> 320: bool _is_in_critical_section; // thread is >>> in a locking critical section >> >> might make sense to add a comment, that his variable Is changed/read only by >> current thread and no sync is needed. > > Good suggestion, thanks. Fixed now. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427099325
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v5]
> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1 > time frame. > It is fixing a deadlock issue between `VirtualThread` class critical sections > with the `interruptLock` (in methods: `unpark()`, `interrupt()`, > `getAndClearInterrupt()`, `threadState()`, `toString()`), > `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms. > The deadlocking scenario is well described by Patricio in a bug report > comment. > In simple words, a virtual thread should not be suspended during > 'interruptLock' critical sections. > > The fix is to record that a virtual thread is in a critical section > (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about > begin/end of critical section. > This bit is used in `HandshakeState::get_op_for_self()` to filter out any > `HandshakeOperation` if a target `JavaThread` is in a critical section. > > Some of new notifications with `notifyJvmtiSync()` method is on a performance > critical path. It is why this method has been intrincified. > > New test was developed by Patricio: > `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` > The test is very nice as it reliably in 100% reproduces the deadlock without > the fix. > The test is never failing with this fix. > > Testing: > - tested with newly added test: > `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` > - tested with mach5 tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: moved notifyJvmtiDisableSuspend(true) out of try-block - Changes: - all: https://git.openjdk.org/jdk/pull/17011/files - new: https://git.openjdk.org/jdk/pull/17011/files/4e5f6447..ad990422 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17011=04 - incr: https://webrevs.openjdk.org/?repo=jdk=17011=03-04 Stats: 10 lines in 1 file changed: 5 ins; 5 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17011.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17011/head:pull/17011 PR: https://git.openjdk.org/jdk/pull/17011
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v4]
On Thu, 14 Dec 2023 17:30:54 GMT, Serguei Spitsyn wrote:
>> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1
>> time frame.
>> It is fixing a deadlock issue between `VirtualThread` class critical
>> sections with the `interruptLock` (in methods: `unpark()`, `interrupt()`,
>> `getAndClearInterrupt()`, `threadState()`, `toString()`),
>> `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms.
>> The deadlocking scenario is well described by Patricio in a bug report
>> comment.
>> In simple words, a virtual thread should not be suspended during
>> 'interruptLock' critical sections.
>>
>> The fix is to record that a virtual thread is in a critical section
>> (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about
>> begin/end of critical section.
>> This bit is used in `HandshakeState::get_op_for_self()` to filter out any
>> `HandshakeOperation` if a target `JavaThread` is in a critical section.
>>
>> Some of new notifications with `notifyJvmtiSync()` method is on a
>> performance critical path. It is why this method has been intrincified.
>>
>> New test was developed by Patricio:
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock`
>> The test is very nice as it reliably in 100% reproduces the deadlock without
>> the fix.
>> The test is never failing with this fix.
>>
>> Testing:
>> - tested with newly added test:
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock`
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: 1) replace CriticalLock with DisableSuspend; 2) minor tweaks
src/java.base/share/classes/java/lang/VirtualThread.java line 746:
> 744: } else if ((s == PINNED) || (s == TIMED_PINNED)) {
> 745: try {
> 746: notifyJvmtiDisableSuspend(true);
Move to before the try.
src/java.base/share/classes/java/lang/VirtualThread.java line 853:
> 851: checkAccess();
> 852: try {
> 853: notifyJvmtiDisableSuspend(true);
This one also needs to be before try.
src/java.base/share/classes/java/lang/VirtualThread.java line 886:
> 884: if (oldValue) {
> 885: try {
> 886: notifyJvmtiDisableSuspend(true);
This one also needs to be before try.
src/java.base/share/classes/java/lang/VirtualThread.java line 917:
> 915: case RUNNING:
> 916: try {
> 917: notifyJvmtiDisableSuspend(true);
This one also needs to be before try.
src/java.base/share/classes/java/lang/VirtualThread.java line 1042:
> 1040: if (carrier != null) {
> 1041: try {
> 1042: notifyJvmtiDisableSuspend(true);
this one too.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427080057
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427080394
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427080484
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427080704
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427080811
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 12:19:43 GMT, Alan Bateman wrote: > Okay. What about the Leonid's suggestion to name it > `notifyJvmtiDisableSuspend()` ? Okay with me. We'll need to move the notifyJvmtiDisableSuspend(true) to before the try in all cases, I've pointed out the cases that we missed. - PR Comment: https://git.openjdk.org/jdk/pull/17011#issuecomment-1856339508
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 17:06:05 GMT, Alan Bateman wrote: >> Implemented this renaming suggestion. Let's wait if Alan ia okay with it. > >> Implemented this renaming suggestion. Let's wait if Alan ia okay with it. > > Are you planning to drop the changes to mount/unmount too? They shouldn't be > needed. > > notifyJvmtiCriticalLock(boolean) is okay for now but needs to be called > before the try, not in the block. We have changes coming that will require > moving these hooks to critical section enter/exit methods, so the naming will > be less important then. Yes, I've dropped changes in the mount/unmount methods. I've already done renaming to `notifyJvmtiDisableSuspend(boolean)`. Let's see if it is okay with you. It is not a problem to rename it back to `notifyJvmtiCriticalLock(boolean)`. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427032721
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v4]
> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1 > time frame. > It is fixing a deadlock issue between `VirtualThread` class critical sections > with the `interruptLock` (in methods: `unpark()`, `interrupt()`, > `getAndClearInterrupt()`, `threadState()`, `toString()`), > `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms. > The deadlocking scenario is well described by Patricio in a bug report > comment. > In simple words, a virtual thread should not be suspended during > 'interruptLock' critical sections. > > The fix is to record that a virtual thread is in a critical section > (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about > begin/end of critical section. > This bit is used in `HandshakeState::get_op_for_self()` to filter out any > `HandshakeOperation` if a target `JavaThread` is in a critical section. > > Some of new notifications with `notifyJvmtiSync()` method is on a performance > critical path. It is why this method has been intrincified. > > New test was developed by Patricio: > `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` > The test is very nice as it reliably in 100% reproduces the deadlock without > the fix. > The test is never failing with this fix. > > Testing: > - tested with newly added test: > `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` > - tested with mach5 tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: 1) replace CriticalLock with DisableSuspend; 2) minor tweaks - Changes: - all: https://git.openjdk.org/jdk/pull/17011/files - new: https://git.openjdk.org/jdk/pull/17011/files/18f1752e..4e5f6447 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17011=03 - incr: https://webrevs.openjdk.org/?repo=jdk=17011=02-03 Stats: 68 lines in 14 files changed: 9 ins; 10 del; 49 mod Patch: https://git.openjdk.org/jdk/pull/17011.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17011/head:pull/17011 PR: https://git.openjdk.org/jdk/pull/17011
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 16:57:25 GMT, Serguei Spitsyn wrote: > Implemented this renaming suggestion. Let's wait if Alan ia okay with it. Are you planning to drop the changes to mount/unmount too? They shouldn't be needed. notifyJvmtiCriticalLock(boolean) is okay for now but needs to be called before the try, not in the block. We have changes coming that will require moving these hooks to critical section enter/exit methods, so the naming will be less important then. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427000950
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 12:11:42 GMT, Serguei Spitsyn wrote: >> src/java.base/share/classes/java/lang/VirtualThread.java line 1164: >> >>> 1162: >>> 1163: @IntrinsicCandidate >>> 1164: private native void notifyJvmtiCriticalLock(boolean enter); >> >> The name is confusing to me, the CriticalLock looks like it is the section >> is critical and might be taken by a single thread only. Or it's just unclear >> what is critical here. >> However, the purpose is to disable suspend >> Wouldn't be 'notifyJvmtiSuspendLock notifyJvmtiDisableSuspend' better name >> here? >> or comment what critical means here. > > Okay, thanks. I like your name suggestion but let's check with Alan first. Implemented this renaming suggestion. Let's wait if Alan ia okay with it. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426990736
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v3]
On Tue, 5 Dec 2023 13:48:11 GMT, Thomas Stuefe wrote: >> Joachim Kern has updated the pull request incrementally with one additional >> commit since the last revision: >> >> encapsulate everything in os::Aix::dlopen > > Excellent, this is how I have pictured a good solution. Very nice. > > A number of remarks, but nothing fundamental. @tstuefe Sorry to tag you. Can you review the code. Once this code goes in I can push in my changes. We are targeting the fix for January. - PR Comment: https://git.openjdk.org/jdk/pull/16920#issuecomment-1856145815
Re: RFR: 8309271: A way to align already compiled methods with compiler directives [v15]
> Compiler Control (https://openjdk.org/jeps/165) provides method-context > dependent control of the JVM compilers (C1 and C2). The active directive > stack is built from the directive files passed with the > `-XX:CompilerDirectivesFile` diagnostic command-line option and the > Compiler.add_directives diagnostic command. It is also possible to clear all > directives or remove the top from the stack. > > A matching directive will be applied at method compilation time when such > compilation is started. If directives are added or changed, but compilation > does not start, then the state of compiled methods doesn't correspond to the > rules. This is not an error, and it happens in long running applications when > directives are added or removed after compilation of methods that could be > matched. For example, the user decides that C2 compilation needs to be > disabled for some method due to a compiler bug, issues such a directive but > this does not affect the application behavior. In such case, the target > application needs to be restarted, and such an operation can have high costs > and risks. Another goal is testing/debugging compilers. > > It would be convenient to optionally reconcile at least existing matching > nmethods to the current stack of compiler directives (so bypass inlined > methods). > > Natural way to eliminate the discrepancy between the result of compilation > and the broken rule is to discard the compilation result, i.e. > deoptimization. Prior to that we can try to re-compile the method letting > compile broker to perform it taking new directives stack into account. > Re-compilation helps to prevent hot methods from execution in the interpreter. > > A new flag `-r` has beed introduced for some directives related to compile > commands: `Compiler.add_directives`, `Compiler.remove_directives`, > `Compiler.clear_directives`. The default behavior has not changed (no flag). > If the new flag is present, the command scans already compiled methods and > puts methods that have any active non-default matching compiler directives to > re-compilation if possible, otherwise marks them for deoptimization. There is > currently no distinction which directives are found. In particular, this > means that if there are rules for inlining into some method, it will be > refreshed. On the other hand, if there are rules for a method and it was > inlined, top-level methods won't be refreshed, but this can be achieved by > having rules for them. > > In addition, a new diagnostic command `Compiler.replace_directives`, has been > added for ... Dmitry Chuyko has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 33 commits: - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - ... and 23 more: https://git.openjdk.org/jdk/compare/fde5b168...44d680cd - Changes: https://git.openjdk.org/jdk/pull/14111/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14111=14 Stats: 372 lines in 15 files changed: 339 ins; 3 del; 30 mod Patch: https://git.openjdk.org/jdk/pull/14111.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14111/head:pull/14111 PR: https://git.openjdk.org/jdk/pull/14111
Re: RFR: 8322065: Initial nroff manpage generation for JDK 23 [v2]
> Updated the version to 23-ea and year to 2024. > > This initial generation also picks up the unpublished changes from: > > - [JDK-8302233](https://bugs.openjdk.org/browse/JDK-8302233) (keytool & > jarsigner) > - [JDK-8290702](https://bugs.openjdk.org/browse/JDK-8290702) (javadoc) (JDK > 23 backport) > > Thanks David Holmes has updated the pull request incrementally with one additional commit since the last revision: Revert "8309981: Remove expired flags in JDK 23" This reverts commit 0324a90e936ae01e42ae099e7235156326cc318a. - Changes: - all: https://git.openjdk.org/jdk/pull/17101/files - new: https://git.openjdk.org/jdk/pull/17101/files/65a8c9ed..8b052141 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17101=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17101=00-01 Stats: 23 lines in 2 files changed: 10 ins; 11 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17101.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17101/head:pull/17101 PR: https://git.openjdk.org/jdk/pull/17101
Re: RFR: 8322065: Initial nroff manpage generation for JDK 23
On Thu, 14 Dec 2023 09:01:17 GMT, Alan Bateman wrote: > Initially I wondered if JDK-8309981 should be separated but include keeps > things in sync so I think okay. Thanks for the review @AlanBateman . Yeah I was in two minds there myself. I started fixing [JDK-8309981](https://bugs.openjdk.org/browse/JDK-8309981) only to discover that the start of release updates had not been done as part of the start of release, so I figured I may as well fix it all together given I'd generated all the updated files anyway. But I'm still a little unsure ... in fact I think I will remove it in the morning. - PR Comment: https://git.openjdk.org/jdk/pull/17101#issuecomment-1855761906
Re: RFR: 8322065: Initial nroff manpage generation for JDK 23
On Thu, 14 Dec 2023 09:17:05 GMT, Pavel Rappo wrote: > Thanks for doing this, David. I only note that the changes for JDK-8321384 > were published in [JDK-8308715](https://bugs.openjdk.org/browse/JDK-8308715), > which was integrated in the mainline before JDK 22 RDP 1. So they are already > present in the mainline. Ah I see. Thanks for correcting that, I will update the PR and JBS issue. And thanks for looking at this @pavelrappo . - PR Comment: https://git.openjdk.org/jdk/pull/17101#issuecomment-1855755042
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 12:06:41 GMT, Serguei Spitsyn wrote: > Carrier thread also can be suspended when executing the "critical code". Why > do you think it can't be a problem? Do you think the deadlocking scenario > described in the bug report is not possible? It's a different scenario. When mounting, the coordination of the interrupt status is done before the thread identity is changed. Similarly, when unmounting, the coordination is done after reverting the thread identity to the carrier. So if there is an agent randomly suspending threads when it shouldn't be an issue here. > > toggle_is_in_critical_section needs to detect reentrancy, it is otherwise > > too easy to refactor the Java code, e.g. call threadState while holding > > the interrupt lock. > > Is your concern a recursive `interruptLock` enter? I was also thinking if > this scenario is possible, so a counter can be used instead of boolean. Minimally an assert. A counter might be needed later. > Okay. What about the Leonid's suggestion to name it > `notifyJvmtiDisableSuspend()` ? We have changes in the works that require pinning during some critical sections so I think I prefer to use that terminology. We can move the notification to JVMTI to the enter/leave methods. - PR Comment: https://git.openjdk.org/jdk/pull/17011#issuecomment-1855748841
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Tue, 12 Dec 2023 23:54:43 GMT, Leonid Mesnik wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: (1) rename notifyJvmti method; (2) add try-final statements to >> VirtualThread methods > > src/hotspot/share/prims/jvm.cpp line 4013: > >> 4011: // Notification from VirtualThread about entering/exiting sync >> critical section. >> 4012: // Needed to avoid deadlocks with JVMTI suspend mechanism. >> 4013: JVM_ENTRY(void, JVM_VirtualThreadCriticalLock(JNIEnv* env, jobject >> vthread, jboolean enter)) > > the jobject vthread is not used. Can't be the method made static to reduce > the number of arguments? > It is the performance-critical code, I don't know if it is optimized by C2. Good question. In general, I'd like to keep this unified with the other `notiftJvmti` methods. Let me double check how it fits together. Also, I'm not sure how is going to impact the intrinsification. > src/hotspot/share/runtime/javaThread.hpp line 320: > >> 318: bool _is_in_VTMS_transition; // thread is >> in virtual thread mount state transition >> 319: bool _is_in_tmp_VTMS_transition; // thread is >> in temporary virtual thread mount state transition >> 320: bool _is_in_critical_section; // thread is >> in a locking critical section > > might make sense to add a comment, that his variable Is changed/read only by > current thread and no sync is needed. Good suggestion, thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426643218 PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426643663
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Tue, 12 Dec 2023 23:42:07 GMT, Leonid Mesnik wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: (1) rename notifyJvmti method; (2) add try-final statements to
>> VirtualThread methods
>
> src/java.base/share/classes/java/lang/VirtualThread.java line 1164:
>
>> 1162:
>> 1163: @IntrinsicCandidate
>> 1164: private native void notifyJvmtiCriticalLock(boolean enter);
>
> The name is confusing to me, the CriticalLock looks like it is the section is
> critical and might be taken by a single thread only. Or it's just unclear
> what is critical here.
> However, the purpose is to disable suspend
> Wouldn't be 'notifyJvmtiSuspendLock notifyJvmtiDisableSuspend' better name
> here?
> or comment what critical means here.
Okay, thanks. I like your name suggestion but let's check with Alan first.
> test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java
> line 30:
>
>> 28: * @requires vm.continuations
>> 29: * @library /testlibrary
>> 30: * @run main/othervm -Xint SuspendWithInterruptLock
>
> Doesn't it make sense to add a testcase without -Xint also? Just to give
> stress testing with compilation.
Thanks. I was also thinking about this. Will add a sub-test without -Xint.
> test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java
> line 36:
>
>> 34:
>> 35: public class SuspendWithInterruptLock {
>> 36: static boolean done;
>
> done is accessed from different threads, should be volatile.
Good suggestion, thanks.
> test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java
> line 54:
>
>> 52: Thread.yield();
>> 53: }
>> 54: done = true;
>
> I think it is better to use done to stop all threads and set it to true in
> the main thread after some time. So you could be sure that the yielder hadn't
> been completed before the suspender started. But it is just proposal.
Thank you. Will consider this.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426638981
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426635613
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426636196
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426637200
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Fri, 8 Dec 2023 11:54:40 GMT, Serguei Spitsyn wrote: >> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1 >> time frame. >> It is fixing a deadlock issue between `VirtualThread` class critical >> sections with the `interruptLock` (in methods: `unpark()`, `interrupt()`, >> `getAndClearInterrupt()`, `threadState()`, `toString()`), >> `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms. >> The deadlocking scenario is well described by Patricio in a bug report >> comment. >> In simple words, a virtual thread should not be suspended during >> 'interruptLock' critical sections. >> >> The fix is to record that a virtual thread is in a critical section >> (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about >> begin/end of critical section. >> This bit is used in `HandshakeState::get_op_for_self()` to filter out any >> `HandshakeOperation` if a target `JavaThread` is in a critical section. >> >> Some of new notifications with `notifyJvmtiSync()` method is on a >> performance critical path. It is why this method has been intrincified. >> >> New test was developed by Patricio: >> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` >> The test is very nice as it reliably in 100% reproduces the deadlock without >> the fix. >> The test is never failing with this fix. >> >> Testing: >> - tested with newly added test: >> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` >> - tested with mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: (1) rename notifyJvmti method; (2) add try-final statements to > VirtualThread methods @AlanBateman Thank you for reviewing an the comment. > It shouldn't be necessary to touch mount/unmount as the thread identity is > the carrier, not the virtual thread, when executing the "critical code". Carrier thread also can be suspended when executing the "critical code". Why do you think it can't be a problem? Do you think the deadlocking scenario described in the bug report is not possible? > toggle_is_in_critical_section needs to detect reentrancy, it is otherwise > too easy to refactor the Java code, e.g. call threadState while holding the > interrupt lock. Is your concern a recursive `interruptLock` enter? I was also thinking if this scenario is possible, so a counter can be used instead of boolean. > All the use-sides will need to use try-finally to more reliably revert the > critical section flag when rewinding. Right, thanks. It is already done. > The naming is very problematic, we'll need to replace with methods that are > clearly named enter and exit critical section. Ongoing work in this area to > support monitors has to introduce some temporary pinning so there will be > enter/exitCriticalSection methods, that's a better place for the JVMTI hooks. Okay. What about the Leonid's suggestion to name it `notifyJvmtiDisableSuspend()` ? - PR Comment: https://git.openjdk.org/jdk/pull/17011#issuecomment-1855730274
Re: RFR: 8322065: Initial nroff manpage generation for JDK 23
On Thu, 14 Dec 2023 05:46:01 GMT, David Holmes wrote: > Updated the version to 23-ea and year to 2024. > > This initial generation also picks up the unpublished changes from: > > - [JDK-8302233](https://bugs.openjdk.org/browse/JDK-8302233) (keytool & > jarsigner) > - [JDK-8290702](https://bugs.openjdk.org/browse/JDK-8290702) (javadoc) (JDK > 23 backport) > - [JDK-8321384](https://bugs.openjdk.org/browse/JDK-8321384) (javadoc) > > > In addition this includes the updates for > > - [JDK-8309981](https://bugs.openjdk.org/browse/8309981) Remove expired flags > in JDK 23 > > Thanks > Updated the version to 23-ea and year to 2024. > > This initial generation also picks up the unpublished changes from: > > * [JDK-8321384](https://bugs.openjdk.org/browse/JDK-8321384) (javadoc) Thanks for doing this, David. I only note that the changes for JDK-8321384 were published in [JDK-8308715](https://bugs.openjdk.org/browse/JDK-8308715), which was integrated in the mainline before JDK 22 RDP 1. So they are already present in the mainline. - PR Comment: https://git.openjdk.org/jdk/pull/17101#issuecomment-1855467435
Re: RFR: 8322065: Initial nroff manpage generation for JDK 23
On Thu, 14 Dec 2023 05:46:01 GMT, David Holmes wrote: > Updated the version to 23-ea and year to 2024. > > This initial generation also picks up the unpublished changes from: > > - [JDK-8302233](https://bugs.openjdk.org/browse/JDK-8302233) (keytool & > jarsigner) > - [JDK-8290702](https://bugs.openjdk.org/browse/JDK-8290702) (javadoc) (JDK > 23 backport) > - [JDK-8321384](https://bugs.openjdk.org/browse/JDK-8321384) (javadoc) > > > In addition this includes the updates for > > - [JDK-8309981](https://bugs.openjdk.org/browse/8309981) Remove expired flags > in JDK 23 > > Thanks Initially I wondered if JDK-8309981 should be separated but include keeps things in sync so I think okay. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17101#pullrequestreview-1781343785
