On Tue, 8 Sep 2020 10:44:24 GMT, David Holmes <[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! > > Marked as reviewed by dholmes (Reviewer). Hi David, > Okay we can put in the current proposed fix to get the test working > again. Please file a new bug to fix the underlying locking problem. I created https://bugs.openjdk.java.net/browse/JDK-8252902 Please feel free to add sub-task, edit or change if I missed anything. Thanks, Robbin > > > > > 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 :) > > I'm sure you get my point though. :) > > > 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). > > Okay ... we know that the VMThread and safepoint VMops are special, and > always have been special, and people absolutely loathe the things we did > in supporting that special-ness and we (well you, Erik, Patricio, Dan) > have been working hard to get rid of a lot of that special code. So I > can't accept an argument that it is okay to take VMThread/VMop special > behaviour (e.g. lock proxying) and now spread it around to make other > special cases. > > > 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. > > Understood. > > I have a general concern with locking in relation to handshakes, that we > cannot actually get rid of all the special handling that pertained to > safepoints (safepoint-check-always/never, lock ranking) just because we > now use handshakes. We have the same kinds of deadlock concerns if a > handshake operation tries to take a lock and might block the thread. So > same problem as safepoints and locks but no supporting code to try and > help us with that. > > So if we cannot simply grab the necessary locks as part of the handshake > operation, then we need a way to ensure locking correctness prior to the > op - and the simplest seems to be that the handshaker does the necessary > locking and the handshake mechanism allows us to ensure the handshaker > executes the operation not the target. > > Thanks, > David > ----- ------------- PR: https://git.openjdk.java.net/jdk/pull/60
