Block Virtual Threads for JVMTI Inspection

[EricYangIBM]

• Return the corresponding J9VMThread for a virtual thread from
  getVMThread.
• Prevent the virtual thread from (un)mounting using the inspectorCount
  approach.
• The (un)mountBegin/(un)mountEnd natives lock the mount/unmount section
  and prevent inspectors from inspecting during that time.
• For a virtual thread, even the underlying J9VMThread is prevented from
  terminating using the inspectorCount approach.
• Moved code from notifyJvmtiUnmountEnd to notifyJvmtiUnmountBegin for blocking
  before unmount. Also, VirtualThread.carrierThread is set to NULL in unmount.
  So, removing the VirtualThread from the linked list before the last unmount
  in notifyJvmtiUnmountBegin seems correct.
• Block mount after yield if a virtual thread is being inspected.
• In getVMThread:
  • targetThread will be NULL for a yielded virtual thread.
  • JVMTI_ERROR_NONE will be returned for a yielded virtual thread.
  • virtualThreadInspectorCount will be incremented for the yielded
    virtual thread.
  • If a virtual thread is in the process of mounting/unmounting, wait
    until it is done before inspecting.

Related: #15183. This impacts all JVMTI functions that rely on getVMThread and
releaseVMThread.

Original PR: #15690
Co-authored-by: Babneet Singh sbabneet@ca.ibm.com
Signed-off-by: Eric Yang eric.yang@ibm.com

[EricYangIBM]

@tajila

[hangshao0]

FYI @EricYangIBM I approved the original PR #15690

[tajila]

after discussion with Jack a few things:

1. if (firstMount) { should be removed, need to check the inspector counts all the time

2. There is a potential race condition between notifymountStart and notifymountEnd (same applies for notifyunmountStart and notifyunmountEnd) as the stackswap, virtualThread.carrier fields changes occur between those points. Its effectively an unsafe range where an inspector cannot reliably inspect the thread. We propose the following changes:
   For VirtualThread:

• on entry to notifymount/unmountStart set vthreadinspector to -1 if the current count is 0
  • this will require acquire/relesing the monitor
• on entry to notifymount/unmountStart vthread should wait on lock if the current count is > 0
  • same as what we are doing now
• on entry to notifymount/unmountEnd set vthreadinspector to zero and notifyAll
  • will also require enter/release the lock

For inspector:

• in getVMThread,
  • if count is <0 wait on lock
  • if count is >= 0 increment count
    • just like we are currently doing
• in releaseVMThread,
  • decrement inspector count
    • if count goes to 0 notify all
    • just like we are currently doing

[fengxue-IS]

Per discussion with @tajila we have come up with a new way of blocking vthread mount/unmount to avoid any timing issue between threadstate updates and thread stack swap in continuation:

notifyJVMTIMountBegin() {
    omrthread_monitor_enter(vm->liveVirtualThreadListMutex);
    while (inspectorCount != 0) {
        omrthread_monitor_wait(vm->liveVirtualThreadListMutex);
    }
    inspectorCount = -1;
    omrthread_monitor_exit(vm->liveVirtualThreadListMutex);
}

notifyJVMTIMountEnd() {
    omrthread_monitor_enter(vm->liveVirtualThreadListMutex);
    inspectorCount = 0;
    omrthread_monitor_notify_all(vm->liveVirtualThreadListMutex);
    omrthread_monitor_exit(vm->liveVirtualThreadListMutex);
}

same logic applies to unmountBegin/End

for getVMThread & releaseVMThread

getVMThread {
    if (currentThread->threadObject != threadObject) {
        omrthread_monitor_enter(vm->liveVirtualThreadListMutex);
        while (inspectorCount == -1) {
            omrthread_monitor_wait(vm->liveVirtualThreadListMutex);
        }
        inspectorCount += 1;
        omrthread_monitor_exit(vm->liveVirtualThreadListMutex);
    }
}

releaseVMThread {
    if (currentThread->threadObject != threadObject) {
        omrthread_monitor_enter(vm->liveVirtualThreadListMutex);
        assert(inspectorCount > 0)
        inspectorCount -= 1;
        omrthread_monitor_notify_all(vm->liveVirtualThreadListMutex);
        omrthread_monitor_exit(vm->liveVirtualThreadListMutex);
    }
}

[tajila]

@gacholio ^^

[gacholio]

At a glance this seems reasonable - I'll have to look in a bit more detail. I also had comments in the old PR which may or may not still apply - will do a full review.

[tajila]

I see a lot of releaseVMThread being called with NULL thread - why is this? Won't it leave the counter unbalanced?

I think this still applies. not sure why NULL is being passed, @fengxue-IS do you know why?

This same code needs to go in haltThreadForInspection, I think, so the comments in the general code should not reference JVMTI.

I dont quite understand this one. Most of the code that calls haltThreadForInspection is done in JVMTI which will call get/releaseVMThread before hand. The other cases are related to acquiring stack traces and threadstate both of which is doable without updating inspector counts since its not "active" inspection, its more like "get me the state at this exact point in time".

[EricYangIBM]

I see a lot of releaseVMThread being called with NULL thread - why is this? Won't it leave the counter unbalanced?

Two cases are because virtual threads are not supported with the operation (ENSURE_JTHREAD_NOT_VIRTUAL(currentThread, thread, JVMTI_ERROR_UNSUPPORTED_OPERATION); is called above), another case is in java version < 19

[gacholio]

I dont quite understand this one. Most of the code that calls haltThreadForInspection is done in JVMTI which will call get/releaseVMThread before hand. The other cases are related to acquiring stack traces and threadstate both of which is doable without updating inspector counts since its not "active" inspection, its more like "get me the state at this exact point in time".

I'm not sure why you believe this - the obvious case of Thread.getStackTrace() clearly must halt the target thread before walking it's stack.

[gacholio]

Two cases are because virtual threads are not supported with the operation (ENSURE_JTHREAD_NOT_VIRTUAL(currentThread, thread, JVMTI_ERROR_UNSUPPORT

This is unnecessarily confusing for no gain - why not simply follow the pattern?

[tajila]

Two cases are because virtual threads are not supported with the operation (ENSURE_JTHREAD_NOT_VIRTUAL(currentThread, thread, JVMTI_ERROR_UNSUPPORTED_OPERATION); is called above), another case is in java version < 19

If ENSURE_JTHREAD_NOT_VIRTUAL is called then we should be abe to pass thread since is guaranteed to not be a virtual thread?

[EricYangIBM]

If ENSURE_JTHREAD_NOT_VIRTUAL is called then we should be abe to pass thread since is guaranteed to not be a virtual thread?

Agreed

[gacholio]

If ENSURE_JTHREAD_NOT_VIRTUAL is called then we should be abe to pass thread since is guaranteed to not be a virtual thread?

Right - the code has to work for all types of threads (for the calls that don't exclude virtual).

[tajila]

I'm not sure why you believe this - the obvious case of Thread.getStackTrace() clearly must halt the target thread before walking it's stack.

What I mean when getting a stack trace, we are not acquiring the stack while the thread is running, it must be halted (this is what I meant by point in time). So youll never observe the vthread related changes between the start and end of the operation since the thread cannot change. For get/releaseVMThread however, the thread is still active within that window, hence why it needs more safe guards.

[gacholio]

Please let me know when you've implemented the new solution and I'll do the review.

[gacholio]

The Thread code does not use JVMTI - it calls halt directly.

The stack trace code will presumably need to be updated (to get the stack of unmounted continuations) at some point, but it will still be halting in the same fashion and must prevent mount/unmount or any other change to the java stack being inspected.

[fengxue-IS]

The stack trace code will presumably need to be updated (to get the stack of unmounted continuations) at some point, but it will still be halting in the same fashion and must prevent mount/unmount or any other change to the java stack being inspected.

getStackTrace on a non-virtual thread should work as expected without any changes
for VirtualThread, it calls asyncGetStackTrace in https://github.com/ibmruntimes/openj9-openjdk-jdk19/blob/openj9/src/java.base/share/classes/java/lang/VirtualThread.java#L854-L864

if the virtual thread is mounted, then haltThreadForInspection should prevent any operation including unmount
if it is not mounted, then it uses tryGetStackTrace to obtain the stack which already sets SUSPENDED flag on the virtual thread object preventing it from being scheduled to mount.

[gacholio]

I suppose the stack swaps always occur while holding VM access, so the normal halt would indeed prevent that from happening.

[EricYangIBM]

The new approach has been implemented @gacholio

[gacholio]

Please squash the commits before testing.

[EricYangIBM]

Squashed

[gacholio]

jenkins test sanity.functional plinux jdk19

[gacholio]

jenkins compile win jdk8

[gacholio]

@fengxue-IS Have all of your concerns been addressed?

[fengxue-IS]

lgtm

[pshipton]

fyi plinux is down atm (and xlinux, mac).

[tajila]

jenkins test sanity amac jdk19

[pshipton]

fyi plinux is down atm (and xlinux, mac)

mac includes amac (All nodes of label ‘[ci.role.build&&hw.arch.aarch64&&sw.os.mac](https://openj9-jenkins.osuosl.org/label/ci.role.build&&hw.arch.aarch64&&sw.os.mac/)’ are offline)

[tajila]

jenkins test sanity win jdk19

[EricYangIBM]

Tests passed, also ran https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/13985/