RFR: Add JVMTI raw monitor lock/unlock mechanisms

Arthur Eubanks aeubanks at google.com
Mon Apr 29 21:58:00 UTC 2019 

I don't see the comment change in src/hotspot/share/prims/jvmtiEnv.cpp.
Other than that, LGTM. (no need for another webrev)

On Mon, Apr 29, 2019 at 2:47 PM Jean Christophe Beyler <jcbeyler at google.com>
wrote:

> Hi Man & Arthur,
>
> New webrev is here:
> http://cr.openjdk.java.net/~jcbeyler/jvmti/webrev.05/
>
> Let me know what you both think :)
> Jc
>
>
> -------------------------------------
>
> On Mon, Apr 29, 2019 at 2:18 PM Man Cao <manc at google.com> wrote:
>
>> webrev.04 looks good to me.
>> Minor comment is that I'd add TODO or comment about
>> pthread_mutex_lock/unlock should be removed from PerObjectCallback()
>> and Java_JvmtiTaggerLoopRunner_iterateOverTags()
>> in libAbstractNativeLoop.cpp, once we have handled the lock creation race.
>> No need for a webrev for this.
>>
>>
> Done :)
>
>
>> For the lock creation race in JvmtiTagMap::JvmtiTagMap(), I think there
>> is a valid C++ data race in double-checked locking
>> in JvmtiTagMap::tag_map_for().
>> Ideally accesses to JvmtiEnvBase::_tag_map should use
>> OrderAccess::load_acquire() and OrderAccess::release_store(), and we should
>> make TSAN aware of these fence semantics, then TSAN would not complain
>> about the lock creation race.
>> I can try sending serviceability-dev@ a change for OrderAccess.
>>
>
> For what it's worth, even if you fix that, you still will get a race from
> TSAN since tsan does not care about this particular race; it cares about
> the lock creation in the instance itself. You would need to fix that and
> not the setting of this tag map.
> Good news is that you can use the jvmtiTaggertest to see if you are right
> and succeed :)
>
>
>>
>> Lastly for Arthur's comment:
>> > test/hotspot/jtreg/tsan/RawRacyNativeLoopTest.java:
>> > Does "lock" have to be static?
>> ThreadLocals are typically private static fields by convention:
>> https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ThreadLocal.html
>> .
>>
>> -Man
>>
>>
>> On Mon, Apr 29, 2019 at 1:10 PM Arthur Eubanks <aeubanks at google.com>
>> wrote:
>>
>>> src/hotspot/share/prims/jvm.cpp: is the cast to (void *) necessary?
>>> -  return new Mutex(Mutex::native, "JVM_RawMonitorCreate");
>>> +  void *mon = (void*) new Mutex(Mutex::native, "JVM_RawMonitorCreate");
>>> +
>>> +  TSAN_RAW_LOCK_CREATE(mon);
>>> +  return mon;
>>>
>>> src/hotspot/share/prims/jvmtiEnv.cpp: the comment shouldn't be there, or
>>> it should be updated
>>> +  // A wait is modeled in Tsan as a simple release-acquire pair.
>>> +  // The matching acquire annotation is below.
>>> +  TSAN_RAW_LOCK_ACQUIRED(rmonitor);
>>> +
>>>    switch (r) {
>>>    case ObjectMonitor::OM_INTERRUPTED:
>>>      return JVMTI_ERROR_INTERRUPT;
>>>    case ObjectMonitor::OM_ILLEGAL_MONITOR_STATE:
>>>      return JVMTI_ERROR_NOT_MONITOR_OWNER;
>>>
>>> src/hotspot/share/prims/jvmtiTagMap.cpp:
>>> It seems wrong to be mixing in Tsan specific stuff with MutexLocker. It
>>> makes the separation of Tsan-specific features and the rest of the JVM less
>>> clear. I would prefer something like
>>> #if INCLUDE_TSAN
>>> TsanMutexScope tsanMutexScope(lock());
>>> #endif
>>> as opposed to replacing MutexLocker with TsanVisibleMutexLocker.
>>> TsanVisibleMutexLocker uses multiple inheritance? ew. What about having
>>> the two superclasses as two member variables instead?
>>>
>>> I don't understand "good thing" in the comments below:
>>> +  // This means that this lock is created under a mutex (good thing)
>>> but then,
>>> +  // subsequent uses do have a lock to protect it (good thing because
>>> not
>>> +  // needed in this case), however TSAN sees it as being needed because:
>>> +  //  - Another thread can come and get the newly created JvmtiTagMap
>>> without a
>>> +  //  lock and acquire the lock.
>>> +  //  - This provokes a race for TSAN on the lock itself, though there
>>> is no
>>> +  //  real issue.
>>>
>>> test/hotspot/jtreg/tsan/RawRacyNativeLoopTest.java:
>>> Does "lock" have to be static?
>>>
>>> On Fri, Apr 26, 2019 at 2:43 PM Jean Christophe Beyler <
>>> jcbeyler at google.com> wrote:
>>>
>>>> Seems like I forgot to say two things:
>>>>
>>>> 1) First here is the link to the newest webrev:
>>>> http://cr.openjdk.java.net/~jcbeyler/jvmti/webrev.04/
>>>> 2) JTREG does not really allow two different native files, so as far as
>>>> I can tell all other examples do what I've done here for -agentlib and
>>>> System.loadlibrary
>>>>
>>>> Thanks,
>>>> Jc
>>>>
>>>> On Fri, Apr 26, 2019 at 9:50 AM Jean Christophe Beyler <
>>>> jcbeyler at google.com> wrote:
>>>>
>>>>> Hi Man,
>>>>>
>>>>> New webrev is here:
>>>>>
>>>>> On Thu, Apr 25, 2019 at 10:41 PM Man Cao <manc at google.com> wrote:
>>>>>
>>>>>> It seems there are a few missing instrumentation in jvmtiTagMap.cpp:
>>>>>> 1. There should be a call to TSAN_RAW_LOCK_CREATE(&_lock) in
>>>>>> JvmtiTagMap::JvmtiTagMap(JvmtiEnv* env), and a call to
>>>>>> TSAN_RAW_LOCK_DESTROY(&_lock) in JvmtiTagMap::~JvmtiTagMap().
>>>>>>
>>>>>
>>>>> So funny thing I can't do that actually. When I do, TSAN complains
>>>>> about a race between the create and the lock itself. I don't know why it
>>>>> didn't show up in our internal testing but I've added a note to it. I'm
>>>>> going to run some tests on our version to see if I'm right that it just
>>>>> didn't show up due to how we were using it (enough time between the create
>>>>> and the iterate to not show up). But my study of the code this morning
>>>>> seems to lead to : yes TSAN sees it as a race but it is not really one.
>>>>>
>>>>>
>>>>>> 2. TsanVisibleMutexLocker should replace MutexLocker in
>>>>>> JvmtiTagMap::get_objects_with_tags().
>>>>>>
>>>>>
>>>>> Done, there was another one as well that now I did the same to. Same
>>>>> as above, I think we just did the minimal that we saw, but not the full set.
>>>>>
>>>>> (Note: I think I'm missing other tag callbacks that should be checked
>>>>> for races, I'll check the code and see in a second phase; probably with a
>>>>> more extensive study of the create/lock tsan race)
>>>>>
>>>>>
>>>>>> Will adding these fix the issue with "sum" in the test?
>>>>>>
>>>>>
>>>>> No they don't, I still have the same issue, the problem is that sum is
>>>>> changed without a lock in user code, and I fail to see how to do a "fence"
>>>>> for tsan from user to JVM and back. Except if we have a solution right now,
>>>>> I'd rather figure this out in a subsequent webrev as this webrev is already
>>>>> growing quite a bit, if that's ok :-)
>>>>>
>>>>>
>>>>>>
>>>>>> For the new tests, there is "-agentlib:AbstractNativeLoop" on the
>>>>>> command line, and they will do System.loadLibrary("AbstractNativeLoop")
>>>>>> again in class initializer. I'm not sure if this could cause any unexpected
>>>>>> behavior, but it is probably better to avoid doing this. Could we put
>>>>>> JVMTI-related code in a new file?
>>>>>>
>>>>>> In RawRacyNativeLoopTest, I think there is a race on the "private
>>>>>> long lock" field itself. Two threads will write different values to the
>>>>>> same field, so TSAN will likely report a race on this field regardless of
>>>>>> what happens in JVMTI code.
>>>>>>
>>>>>
>>>>> Done, added a threadlocal instead to make it really localized. This is
>>>>> yet another reason I'd like our race detection in our tests to check the
>>>>> signatures but we need symbolization first (hint hint).
>>>>> Also tested and it now fails with the right race.
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>> Minor comments:
>>>>>> I think a better name for TsanSimulatedMutexLocker could be
>>>>>> TsanMutexScope, because TsanSimulatedMutexLocker has nothing to do with the
>>>>>> MutexLocker class. Also, this class could be a subclase of StackObj.
>>>>>>
>>>>>
>>>>> Done, I think the StackObj would have been caught when running this in
>>>>> fastdebug, which we have not yet set up to be done by default in our
>>>>> testing...
>>>>>
>>>>>
>>>>>> I'd keep our internal comment about "Simulates barrier
>>>>>> synchronization on safepoint" in doit() functions in jvmtiTagMap.cpp. I
>>>>>> think they are quite useful to explain why we need the TSAN instrumentation.
>>>>>>
>>>>>
>>>>> Done.
>>>>>
>>>>>
>>>>>> In libAbstractNativeLoop.cpp, why "include <string.h>" but I don't
>>>>>> see any string functions?
>>>>>>
>>>>>
>>>>> memset is defined there :)
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> -Man
>>>>>>
>>>>>>
>>>>>> On Thu, Apr 25, 2019 at 3:01 PM Jean Christophe Beyler <
>>>>>> jcbeyler at google.com> wrote:
>>>>>>
>>>>>>> Done on all points I believe Man.
>>>>>>>
>>>>>>> I added the JvmtiTagMap change and added a new test for that as
>>>>>>> well. The test was a bit annoying due to the fact that it was difficult to
>>>>>>> explain that the "sum" parameter was safe to be updated before/after the
>>>>>>> heap iteration. Currently, I've had to put a mutex around it all, which
>>>>>>> seems like overkill but it was the only way Tsan would not complain.
>>>>>>>
>>>>>>> I'm unclear if that is right, it seems that it is due to the fact
>>>>>>> that it does not see the VM operation as a fork/join operation perhaps? I
>>>>>>> believe this is systemic to any JVMTI operation that has a VM operation
>>>>>>> style callback so we might have more work there.
>>>>>>>
>>>>>>> Webrev: http://cr.openjdk.java.net/~jcbeyler/jvmti/webrev.02/
>>>>>>>
>>>>>>> Btw, @Arthur: the // INCLUDE_TSAN is actually questionable... it
>>>>>>> seems half the #endif do it and half don't so I'm not sure what is the
>>>>>>> right approach.
>>>>>>>
>>>>>>> Let me know what you think,
>>>>>>> Jc
>>>>>>>
>>>>>>> On Wed, Apr 24, 2019 at 4:58 PM Man Cao <manc at google.com> wrote:
>>>>>>>
>>>>>>>> Code in HotSpot looks good. Except that we have additional
>>>>>>>> instrumentation in jvmtiTagMap.cpp that could be added in a following
>>>>>>>> change.
>>>>>>>>
>>>>>>>> A few comments on tests:
>>>>>>>> In AbstractNativeLoop.java:
>>>>>>>> Would it make sense to make all native methods "static"? They don't
>>>>>>>> access any instance field, right?
>>>>>>>>
>>>>>>>> In NonRawRacyNativeLoopTest.java:
>>>>>>>> There are two calls to createRawLock() in NonRawRacyNativeLoopRunner.
>>>>>>>> Could the one in main() be deleted?
>>>>>>>>
>>>>>>>> In RawRacyNativeLoopTest.java:
>>>>>>>> Similarly, could the createRawLock() in main() be deleted?
>>>>>>>> I think calling createRawLock() in run() would result in each
>>>>>>>> thread creating 50000 raw monitors, due to the way in which
>>>>>>>> AbstractLoop is written. Can we avoid doing this so the test can be a bit
>>>>>>>> faster?
>>>>>>>>
>>>>>>>> -Man
>>>>>>>>
>>>>>>>>
>>>>>>>> On Wed, Apr 24, 2019 at 11:01 AM Jean Christophe Beyler <
>>>>>>>> jcbeyler at google.com> wrote:
>>>>>>>>
>>>>>>>>> Sounds good to me.
>>>>>>>>>
>>>>>>>>> Here it is:
>>>>>>>>> http://cr.openjdk.java.net/~jcbeyler/jvmti/webrev.01/
>>>>>>>>>
>>>>>>>>> I also took the idea of adding a  TSAN_RUNTIME_ONLY, I think it
>>>>>>>>> works well
>>>>>>>>> for one liners for code readability to be honest. If we want this,
>>>>>>>>> I'll do
>>>>>>>>> another pass to propagate it around the rest of the changes in a
>>>>>>>>> separate
>>>>>>>>> webrev.
>>>>>>>>> cf:
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~jcbeyler/jvmti/webrev.01/src/hotspot/share/tsan/tsan.hpp.html
>>>>>>>>>
>>>>>>>>> And now with Arthur's change, all tests now pass (still would like
>>>>>>>>> to fix
>>>>>>>>> the racing tests to be more restrictive),
>>>>>>>>> Jc
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Wed, Apr 24, 2019 at 10:18 AM Arthur Eubanks <
>>>>>>>>> aeubanks at google.com> wrote:
>>>>>>>>>
>>>>>>>>> > Yeah we should have another tsan header. Kind of like how our
>>>>>>>>> interface
>>>>>>>>> > for the other TSAN functions is through SharedRuntime. Only that
>>>>>>>>> file would
>>>>>>>>> > include tsanExternalDecls.hpp. The other files would include the
>>>>>>>>> new header
>>>>>>>>> > file.
>>>>>>>>> >
>>>>>>>>> > On Wed, Apr 24, 2019 at 10:14 AM Jean Christophe Beyler <
>>>>>>>>> > jcbeyler at google.com> wrote:
>>>>>>>>> >
>>>>>>>>> >> Hi Arthur,
>>>>>>>>> >>
>>>>>>>>> >> What do you mean: "pulling in tsanExternalDecls.hpp" is wrong?
>>>>>>>>> Seems like
>>>>>>>>> >> it is what we did internally and what you were doing for the
>>>>>>>>> lock webrev as
>>>>>>>>> >> well, ie have that as our include header. Are you suggesting we
>>>>>>>>> make
>>>>>>>>> >> another tsan header for these methods.
>>>>>>>>> >>
>>>>>>>>> >> Ok for all other points (except the indentation, just the way
>>>>>>>>> webrev does
>>>>>>>>> >> things by default, the indentation is actually correct in my
>>>>>>>>> code),
>>>>>>>>> >> Jc
>>>>>>>>> >>
>>>>>>>>> >> On Wed, Apr 24, 2019 at 9:58 AM Arthur Eubanks <
>>>>>>>>> aeubanks at google.com>
>>>>>>>>> >> wrote:
>>>>>>>>> >>
>>>>>>>>> >>> Overall:
>>>>>>>>> >>> Since AnnotateRWLock* are direct TSAN calls, I think we should
>>>>>>>>> have a
>>>>>>>>> >>> wrapper around them. Then we can have things like checking if
>>>>>>>>> the lock is
>>>>>>>>> >>> NULL, etc, in a central location. And pulling in
>>>>>>>>> tsanExternalDecls.hpp
>>>>>>>>> >>> seems wrong.
>>>>>>>>> >>> "#if INCLUDE_TSAN" ends with "#endif // INCLUDE_TSAN", could
>>>>>>>>> you add the
>>>>>>>>> >>> comment for the #endifs.
>>>>>>>>> >>>
>>>>>>>>> >>> test/hotspot/jtreg/tsan/TsanRunner.java
>>>>>>>>> >>> Could you rename "args" to "vmArgs"?
>>>>>>>>> >>>
>>>>>>>>> >>> src/hotspot/share/prims/jvmtiEnv.cpp.udiff
>>>>>>>>> >>> Indentation around 3258 is wrong.
>>>>>>>>> >>>
>>>>>>>>> >>>
>>>>>>>>> >>> On Tue, Apr 23, 2019 at 2:29 PM Jean Christophe Beyler <
>>>>>>>>> >>> jcbeyler at google.com> wrote:
>>>>>>>>> >>>
>>>>>>>>> >>>> Hi all,
>>>>>>>>> >>>>
>>>>>>>>> >>>> I added the locks for the JVMTI raw monitors and added two
>>>>>>>>> tests, it is
>>>>>>>>> >>>> failing the non racy ones due to the JVM locks not yet
>>>>>>>>> included but I
>>>>>>>>> >>>> think
>>>>>>>>> >>>> that will resolve itself soon :)
>>>>>>>>> >>>>
>>>>>>>>> >>>> I also moved the agent library to C++.
>>>>>>>>> >>>>
>>>>>>>>> >>>> FWIW, we also need to clean up our tests that check for
>>>>>>>>> failing, right
>>>>>>>>> >>>> now
>>>>>>>>> >>>> they pass because they generically just are checking for a
>>>>>>>>> TSAN race but
>>>>>>>>> >>>> not specifically which one, which is a bit wrong / imprecise.
>>>>>>>>> >>>>
>>>>>>>>> >>>> Webrev: http://cr.openjdk.java.net/~jcbeyler/jvmti/webrev.00/
>>>>>>>>> >>>>
>>>>>>>>> >>>> Let me know what you think,
>>>>>>>>> >>>> Jc
>>>>>>>>> >>>>
>>>>>>>>> >>>
>>>>>>>>> >>
>>>>>>>>> >> --
>>>>>>>>> >>
>>>>>>>>> >> Thanks,
>>>>>>>>> >> Jc
>>>>>>>>> >>
>>>>>>>>> >
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Jc
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Jc
>>>>>>>
>>>>>>
>>>>>
>>>>> --
>>>>>
>>>>> Thanks,
>>>>> Jc
>>>>>
>>>>
>>>>
>>>> --
>>>>
>>>> Thanks,
>>>> Jc
>>>>
>>>
>
> --
>
> Thanks,
> Jc
>

More information about the tsan-dev mailing list