On Mon, 7 Sep 2020 23:07:13 GMT, Yasumasa Suenaga <[email protected]> wrote:
>> When these two methods (set_frame_pop/clear_frame_pop) are called in a >> handshake the requesting thread will have lock >> the JvmtiThreadState_lock. But the thread executing one of these in the >> handshake may not be the owner. >> So we only check that JvmtiThreadState_lock is locked. >> >> When verifying the callers to these methods I notice "clear_to_frame_pop" >> was unused, so instead of fixing it I remove >> it. >> Passes testing locally, still running T3 and T7. >> >> Now passed! > > Thanks for catching up this. Looks good. Hi David, > > When these two methods (set_frame_pop/clear_frame_pop) are called in a > > handshake the requesting thread will have lock > > the JvmtiThreadState_lock. But the thread executing one of these in the > > handshake may not be the owner. > > Ouch! We continue to get bitten by the fact the executor of a handshake > operation is not known a-priori. :( > We could add this. But I would prefer not doing it in this bug fix. > > So we only check that JvmtiThreadState_lock is locked. > > That avoids the problem but it is not really a sufficient check - we > need to know that it is the handshaker thread that owns the lock and > that it acquired it purely for the purpose of this handshake operation! > Right now we know that because I traced the code :) As I said I can make the requesting thread known to execution thread, but I would really prefer not doing that in this bug fix. > More importantly this issue shows that the locking code is in fact > incorrect. We are on very dangerous ground if we are going to allow > locks to be "proxied" in this fashion! This is akin to reintroducing > "sneaky locks"! A problem is that we use a safepoiting global lock to protect per thread resource. The way we get around this with safepoints is: "assert_locked_or_safepoint" (assert may be passed by a non-JavaThread or JavaThread in blocked/native without lock during a safepoint (safepoint + VM thread would fix that)) Which have this in it: // see if invoker of VM operation owns it VM_Operation* op = VMThread::vm_operation(); if (op != NULL && op->calling_thread() == lock->owner()) return; So we already have proxied locks and in other cases just avoiding them (by just checking if at safepoint). And as I said a few times now, I said I can make the requesting thread known to execution thread, but I would really prefer not doing that in this bug fix. > > If a handshake operation can be executed by either the target thread or > the handshaker thread, and locks are required, then they will have to be > acquired in the context of the handshake operation. If we can't do that > because of potential deadlocks in the handshake mechanism then I feel > that handshakes have a serious flaw that needs to be rectified. Perhaps > we need a way to force the operation to be executed by the handshaker so > that they can setup the right execution environment prior to the handshake? The problem is that it's a safepointing lock. Only executing the handshake by requester is an interesting idea, I'll investigate that. > > > When verifying the callers to these methods I notice "clear_to_frame_pop" > > was unused, so instead of fixing it I remove > > it. > > There is already a bug for that: > > https://bugs.openjdk.java.net/browse/JDK-8252816 > > so if this proceeds you should add that issue to the PR. Ok, I'll do that. Thanks for having a look, let me know howto proceed. > > Thanks, > David > ----- ------------- PR: https://git.openjdk.java.net/jdk/pull/60
