RFR: Add JVMTI raw monitor lock/unlock mechanisms
Jean Christophe Beyler
jcbeyler at google.com
Mon Apr 29 21:47:08 UTC 2019
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