RFR: Add JVMTI raw monitor lock/unlock mechanisms
Man Cao
manc at google.com
Mon Apr 29 21:18:08 UTC 2019
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.
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.
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
>>
>
More information about the tsan-dev
mailing list