RFR (S) 5103339: Strengthen NoSafepointVerifier
David Holmes
david.holmes at oracle.com
Wed Aug 7 02:36:23 UTC 2019
Hi Coleen,
On 7/08/2019 8:43 am, coleen.phillimore at oracle.com wrote:
>
> Thanks David for reading this.
>
> On 8/5/19 9:47 PM, David Holmes wrote:
>> Hi Coleen,
>>
>> Could please update the bug explaining why it was re-opened and what
>> the intended enhancement is. It was closed in 2005 as not necessary
>> because of the unhandled-oop checking.
>
> Yes, I added a comment.
Thanks!
>>
>> On 6/08/2019 2:00 am, coleen.phillimore at oracle.com wrote:
>>> Summary: Add NSV check at possible safepoint transition or places
>>> that could take out locks. Consolidate with clearing unhandled oops.
>>>
>>> This change checks for NoSafepointVerifier no_safepoint_counts at
>>> possible safepoints. The starting set is at transitions, and in the
>>
>> Wouldn't it be better placed in the actual Safepoint checking methods
>> rather than callers of those methods?
>
> See my reply to Robbin. We want to make the checks in places that
> conditionally may not safepoint poll, so that timing doesn't prevent
> finding when we've violated the NoSafepointVerifier check.
Okay.
>>
>>> "else" clauses where CHECK_UNHANDLED_OOPS were clearing unhandled
>>> oops. Some of these were removed because they weren't places with
>>> possible safepoints, so were wrong.
>>>
>>> The unhandled oops clearing and no_safepoint counter check are now
>>> done in the same function. MemAllocator ->
>>> check_for_valid_allocation_state calls
>>> check_for_valid_safepoint_state which calls check_possible_safepoint.
>>>
>>> Calls to check_possible_safepoint are in DEBUG_ONLY when
>>> Thread::current() is called.
>>>
>>> I had to remove it from the else clause in JvmtiThreadState because
>>> it's called from a place that cannot safepoint (see vtableStubs.cpp).
>>
>> Can you elaborate on that further please. It's not obvious, without
>> following call chains, when any code may or may not hit a safepoint
>> check.
>
> No, it isn't. There's quite a large call stack here, but the
> CompiledICLocker has an embedded NoSafepointVerifier, so here's the call
> stack:
>
> V [libjvm.so+0x16c5d05] Thread::check_possible_safepoint()+0x55
> V [libjvm.so+0x10fb15c]
> JvmtiExport::post_dynamic_code_generated_while_holding_locks(char
> const*, unsigned char*, unsigned char*)+0x4c
> V [libjvm.so+0x17c8fac] VtableStubs::find_stub(bool, int)+0x2ac
> V [libjvm.so+0x9dd18e] CompiledIC::set_to_megamorphic(CallInfo*,
> Bytecodes::Code, bool&, Thread*)+0x9e
> V [libjvm.so+0x1562bb4]
> SharedRuntime::handle_ic_miss_helper_internal(Handle, CompiledMethod*,
> frame const&, methodHandle, Bytecodes::Code, CallInfo&, bool&,
> Thread*)+0x434
> V [libjvm.so+0x156d202]
> SharedRuntime::handle_ic_miss_helper(JavaThread*, Thread*)+0x372
> V [libjvm.so+0x156d7a4]
> SharedRuntime::handle_wrong_method_ic_miss(JavaThread*)+0x184
>
> The function handle_ic_miss_helper_internal has a CompiledICLocker,
> which has an embedded NoSafepointVerifier.
>
> This seems like a lot of code to be protected with a
> NoSafepointVerifier, but I assume that at least the find_stub code
> shouldn't get cleaned up by a safepoint, so would require it.
Okay so because you added the call to jvmti_thread_state() in the
vtableStubs code you have to remove the check from the else-clause in
jvmti_thread_state(), because it could fail with the new call path.
> I also ran tests with an assert that the jvmti_thread_state() != NULL,
> just to see if it's ever NULL and it wasn't. I would have liked to keep
> the else.
>>
>>> os.cpp ResourceMark needed for debugging.
>>
>> Interesting catch - I would have expected the RM to be higher up the
>> call if needed. How did you detect this?
>
> Debugging, but I don't remember which thing I was debugging though.
>>
>>> Tested with tier1 on all Oracle platforms, and tier 1-3 on
>>> linux-x64-debug.
>>>
>>> open webrev at
>>> http://cr.openjdk.java.net/~coleenp/2019/5103339.01/webrev
>>> bug link https://bugs.openjdk.java.net/browse/JDK-5103339
>>
>> src/hotspot/share/oops/objArrayKlass.cpp
>> src/hotspot/share/oops/typeArrayKlass.cpp
>>
>> Unclear why the check is predicated on !or_null ??
>
> If the parameter passed or_null is true, then the function never
> safepoints. I added this comment.
>
> // If passed or_null, this function will not try to safepoint.
> if (!or_null) THREAD->check_possible_safepoint();
Sorry I'm confused by the placement. We want to call
check_possible_safepoint() on code paths that can safepoint, so that we
ensure there is no NSV higher up the call chain when we take that path.
So notwithstanding where the check_unhandled_oops was, instead of
352 } else {
353 if (!or_null) THREAD->check_possible_safepoint();
354 }
355
356 ObjArrayKlass *ak = ObjArrayKlass::cast(higher_dimension());
357 if (or_null) {
358 return ak->array_klass_or_null(n);
359 }
360 return ak->array_klass(n, THREAD);
shouldn't/couldn't we just have:
352 }
353
354
355
356 ObjArrayKlass *ak = ObjArrayKlass::cast(higher_dimension());
357 if (or_null) {
358 return ak->array_klass_or_null(n);
359 }
THREAD->check_possible_safepoint();
360 return ak->array_klass(n, THREAD);
?
That said, are we certain that the xxx_or_null functions cannot
safepoint? They won't try to load a class but surely they could
potentially safepoint for other reasons?
> This is also called with NSV with or_null true. I can't remember
> where, I think it was the heap walker.
>
>>
>> ---
>>
>> src/hotspot/share/code/vtableStubs.cpp
>>
>> // cause a safepoint in this code that has NSV.
>>
>> Unclear what "code" has the NSV ??
>
> See above. I can change the comment to:
>
> 237 // all locks. Only post this event if a new state is not required.
> Creating a new state would
> 238 // cause a safepoint and the caller of this code has a
> NoSafepointVerifier.
Sounds good!
>>
>> ---
>>
>> src/hotspot/share/runtime/interfaceSupport.inline.hpp
>>
>> As mentioned above why not put the check inside
>> SafepointMechanism::block_if_requested instead?
>
> The other callers of block_if_requested already have a
> check_possible_safepoint call. The only caller that made sense to add
> was transition from or to vm.
Got it.
>>
>> ---
>>
>> src/hotspot/share/runtime/mutex.cpp
>>
>> The Mutex constructor cleanup is incidental but okay. (Does make me
>> wonder about the whole Monitor construction process though ...
>> ClearMonitor doesn't need to be protected any more.)
>
> This leaked into this change. I'll take it out and post a different RFR
> for it. The no-arg constructor Monitor isn't needed either so
> ClearMonitor can be deleted.
Okay.
>>
>> ---
>>
>> src/hotspot/share/runtime/thread.hpp
>>
>> // NSV checking
>> void check_for_valid_safepoint_state(bool potential_vm_operation)
>> NOT_DEBUG_RETURN;
>> void check_possible_safepoint() NOT_DEBUG_RETURN;
>>
>> Please expand NSV. Does the comment apply to both methods or only the
>> first?
>
> Both methods. I'll change it to:
>
> // These functions check conditions on a JavaThread before possibly
> going to a safepoint,
> // including NoSafepointVerifier.
> void check_for_valid_safepoint_state(bool potential_vm_operation)
> NOT_DEBUG_RETURN;
> void check_possible_safepoint() NOT_DEBUG_RETURN;
Okay on the new comment.
Thanks,
David
-----
>
> Thanks!
> Coleen
>>
>> Thanks,
>> David
>> -----
>>
>>> Thanks,
>>> Coleen
>
More information about the hotspot-runtime-dev
mailing list