RFR: Add JVMTI raw monitor lock/unlock mechanisms
Arthur Eubanks
aeubanks at google.com
Mon Apr 29 20:10:31 UTC 2019
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
>
More information about the tsan-dev
mailing list