stan/jdk-24
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

8266557: assert(SafepointMechanism::local_poll_armed(_handshakee)) failed: Must be

Browse Source 
Reviewed-by: pchilanomate, dcubed
This commit is contained in:
Robbin Ehn 2021-06-09 07:21:37 +00:00
parent 4d1cf51b1d
commit 2bfd708e92
6 changed files with 116 additions and 75 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

80
src/hotspot/share/runtime/handshake.cpp
View File

@ -327,7 +327,9 @@ void HandshakeOperation::do_handshake(JavaThread* thread) {
// here to make sure memory operations executed in the handshake
// closure are visible to the VMThread/Handshaker after it reads
// that the operation has completed.
Atomic::dec(&_pending_threads, memory_order_release);
Atomic::dec(&_pending_threads);
// Trailing fence, used to make sure removal of the operation strictly
// happened after we completed the operation.
// It is no longer safe to refer to 'this' as the VMThread/Handshaker may have destroyed this operation
}
@ -419,22 +421,14 @@ void HandshakeState::add_operation(HandshakeOperation* op) {
bool HandshakeState::operation_pending(HandshakeOperation* op) {
MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
class MatchOp {
HandshakeOperation* _op;
public:
MatchOp(HandshakeOperation* op) : _op(op) {}
bool operator()(HandshakeOperation* op) {
return op == _op;
}
};
MatchOp mo(op);
return _queue.contains(mo);
}
HandshakeOperation* HandshakeState::pop_for_self() {
HandshakeOperation* HandshakeState::get_op_for_self() {
assert(_handshakee == Thread::current(), "Must be called by self");
assert(_lock.owned_by_self(), "Lock must be held");
return _queue.pop();
return _queue.peek();
};
static bool non_self_queue_filter(HandshakeOperation* op) {
@ -447,10 +441,17 @@ bool HandshakeState::have_non_self_executable_operation() {
return _queue.contains(non_self_queue_filter);
}
HandshakeOperation* HandshakeState::pop() {
HandshakeOperation* HandshakeState::get_op() {
assert(_handshakee != Thread::current(), "Must not be called by self");
assert(_lock.owned_by_self(), "Lock must be held");
return _queue.pop(non_self_queue_filter);
return _queue.peek(non_self_queue_filter);
};
void HandshakeState::remove_op(HandshakeOperation* op) {
assert(_lock.owned_by_self(), "Lock must be held");
MatchOp mo(op);
HandshakeOperation* ret = _queue.pop(mo);
assert(ret == op, "Popped op must match requested op");
};
bool HandshakeState::process_by_self() {
@ -469,9 +470,9 @@ bool HandshakeState::process_by_self() {
}
bool HandshakeState::process_self_inner() {
while (should_process()) {
while (has_operation()) {
MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
HandshakeOperation* op = pop_for_self();
HandshakeOperation* op = get_op_for_self();
if (op != NULL) {
assert(op->_target == NULL || op->_target == Thread::current(), "Wrong thread");
bool async = op->is_async();
@ -481,12 +482,14 @@ bool HandshakeState::process_self_inner() {
if (!async) {
HandleMark hm(_handshakee);
PreserveExceptionMark pem(_handshakee);
op->do_handshake(_handshakee);
op->do_handshake(_handshakee); // acquire, op removed after
remove_op(op);
} else {
// An asynchronous handshake may put the JavaThread in blocked state (safepoint safe).
// The destructor ~PreserveExceptionMark touches the exception oop so it must not be executed,
// since a safepoint may be in-progress when returning from the async handshake.
op->do_handshake(_handshakee);
op->do_handshake(_handshakee); // acquire, op removed after
remove_op(op);
log_handshake_info(((AsyncHandshakeOperation*)op)->start_time(), op->name(), 1, 0, "asynchronous");
delete op;
return true; // Must check for safepoints
@ -531,6 +534,7 @@ bool HandshakeState::claim_handshake() {
// just adds an operation we may see it here. But if the handshakee is not
// armed yet it is not safe to proceed.
if (have_non_self_executable_operation()) {
OrderAccess::loadload(); // Matches the implicit storestore in add_operation()
if (SafepointMechanism::local_poll_armed(_handshakee)) {
return true;
}
@ -565,39 +569,31 @@ HandshakeState::ProcessResult HandshakeState::try_process(HandshakeOperation* ma
Thread* current_thread = Thread::current();
HandshakeState::ProcessResult pr_ret = HandshakeState::_processed;
int executed = 0;
HandshakeOperation* op = get_op();
do {
HandshakeOperation* op = pop();
if (op != NULL) {
assert(SafepointMechanism::local_poll_armed(_handshakee), "Must be");
assert(op->_target == NULL || _handshakee == op->_target, "Wrong thread");
log_trace(handshake)("Processing handshake " INTPTR_FORMAT " by %s(%s)", p2i(op),
op == match_op ? "handshaker" : "cooperative",
current_thread->is_VM_thread() ? "VM Thread" : "JavaThread");
assert(op != NULL, "Must have an op");
assert(SafepointMechanism::local_poll_armed(_handshakee), "Must be");
assert(op->_target == NULL || _handshakee == op->_target, "Wrong thread");
if (op == match_op) {
pr_ret = HandshakeState::_succeeded;
}
log_trace(handshake)("Processing handshake " INTPTR_FORMAT " by %s(%s)", p2i(op),
op == match_op ? "handshaker" : "cooperative",
current_thread->is_VM_thread() ? "VM Thread" : "JavaThread");
op->prepare(_handshakee, current_thread);
op->prepare(_handshakee, current_thread);
_active_handshaker = current_thread;
op->do_handshake(_handshakee);
_active_handshaker = NULL;
executed++;
}
} while (have_non_self_executable_operation());
set_active_handshaker(current_thread);
op->do_handshake(_handshakee); // acquire, op removed after
set_active_handshaker(NULL);
remove_op(op);
_lock.unlock();
log_trace(handshake)("%s(" INTPTR_FORMAT ") executed %d ops for JavaThread: " INTPTR_FORMAT " %s target op: " INTPTR_FORMAT,
log_trace(handshake)("%s(" INTPTR_FORMAT ") executed an op for JavaThread: " INTPTR_FORMAT " %s target op: " INTPTR_FORMAT,
current_thread->is_VM_thread() ? "VM Thread" : "JavaThread",
p2i(current_thread), executed, p2i(_handshakee),
pr_ret == HandshakeState::_succeeded ? "including" : "excluding", p2i(match_op));
return pr_ret;
p2i(current_thread), p2i(_handshakee),
op == match_op ? "including" : "excluding", p2i(match_op));
return op == match_op ? HandshakeState::_succeeded : HandshakeState::_processed;
}
void HandshakeState::do_self_suspend() {

43
src/hotspot/share/runtime/handshake.hpp
View File

@ -29,6 +29,7 @@
#include "memory/iterator.hpp"
#include "runtime/flags/flagSetting.hpp"
#include "runtime/mutex.hpp"
#include "runtime/orderAccess.hpp"
#include "utilities/filterQueue.hpp"
class HandshakeOperation;
@ -85,7 +86,7 @@ class HandshakeState {
// JavaThread suspend/resume operations.
Monitor _lock;
// Set to the thread executing the handshake operation.
Thread* _active_handshaker;
Thread* volatile _active_handshaker;
bool claim_handshake();
bool possibly_can_process_handshake();
@ -99,8 +100,20 @@ class HandshakeState {
bool process_self_inner();
bool have_non_self_executable_operation();
HandshakeOperation* pop_for_self();
HandshakeOperation* pop();
HandshakeOperation* get_op_for_self();
HandshakeOperation* get_op();
void remove_op(HandshakeOperation* op);
void set_active_handshaker(Thread* thread) { Atomic::store(&_active_handshaker, thread); }
class MatchOp {
HandshakeOperation* _op;
public:
MatchOp(HandshakeOperation* op) : _op(op) {}
bool operator()(HandshakeOperation* op) {
return op == _op;
}
};
public:
HandshakeState(JavaThread* thread);
@ -113,28 +126,6 @@ class HandshakeState {
bool operation_pending(HandshakeOperation* op);
// Both _queue and _lock must be checked. If a thread has seen this _handshakee
// as safe it will execute all possible handshake operations in a loop while
// holding _lock. We use lock free addition to the queue, which means it is
// possible for the queue to be seen as empty by _handshakee but as non-empty
// by the thread executing in the loop. To avoid the _handshakee continuing
// while handshake operations are being executed, the _handshakee
// must take slow path, process_by_self(), if _lock is held.
bool should_process() {
// The holder of the _lock can add an asynchronous handshake to queue.
// To make sure it is seen by the handshakee, the handshakee must first
// check the _lock, and if held go to slow path.
// Since the handshakee is unsafe if _lock gets locked after this check
// we know other threads cannot process any handshakes.
// Now we can check the queue to see if there is anything we should processs.
if (_lock.is_locked()) {
return true;
}
// Lock check must be done before queue check, force ordering.
OrderAccess::loadload();
return !_queue.is_empty();
}
bool process_by_self();
enum ProcessResult {
@ -147,7 +138,7 @@ class HandshakeState {
};
ProcessResult try_process(HandshakeOperation* match_op);
Thread* active_handshaker() const { return _active_handshaker; }
Thread* active_handshaker() const { return Atomic::load(&_active_handshaker); }
// Suspend/resume support
private:

3
src/hotspot/share/runtime/safepointMechanism.cpp
View File

@ -94,7 +94,7 @@ void SafepointMechanism::process(JavaThread *thread) {
// 3) Before the handshake code is run
StackWatermarkSet::on_safepoint(thread);
need_rechecking = thread->handshake_state()->should_process() && thread->handshake_state()->process_by_self();
need_rechecking = thread->handshake_state()->has_operation() && thread->handshake_state()->process_by_self();
} while (need_rechecking);
}
@ -113,6 +113,7 @@ uintptr_t SafepointMechanism::compute_poll_word(bool armed, uintptr_t stack_wate
}
void SafepointMechanism::update_poll_values(JavaThread* thread) {
assert(thread == Thread::current(), "Must be");
assert(thread->thread_state() != _thread_blocked, "Must not be");
assert(thread->thread_state() != _thread_in_native, "Must not be");
for (;;) {

11
src/hotspot/share/utilities/filterQueue.hpp
View File

@ -1,5 +1,5 @@
/*
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@ -67,10 +67,10 @@ class FilterQueue {
template <typename MATCH_FUNC>
bool contains(MATCH_FUNC& match_func);
// Same as pop(MATCH_FUNC& match_func) but matches everything, thus returning
// Same as peek(MATCH_FUNC& match_func) but matches everything, thus returning
// the first inserted item.
E pop() {
return pop(match_all);
E peek() {
return peek(match_all);
}
// Applies the match_func to each item in the queue and returns the first
@ -81,6 +81,9 @@ class FilterQueue {
// calls.
template <typename MATCH_FUNC>
E pop(MATCH_FUNC& match_func);
template <typename MATCH_FUNC>
E peek(MATCH_FUNC& match_func);
};
#endif

27
src/hotspot/share/utilities/filterQueue.inline.hpp
View File

@ -1,5 +1,5 @@
/*
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@ -112,4 +112,29 @@ E FilterQueue<E>::pop(MATCH_FUNC& match_func) {
} while (true);
}
// MT-Unsafe, external serialization needed.
template <class E>
template <typename MATCH_FUNC>
E FilterQueue<E>::peek(MATCH_FUNC& match_func) {
Node* first = load_first();
Node* cur = first;
Node* match = NULL;
if (cur == NULL) {
return (E)NULL;
}
do {
if (match_func(cur->_data)) {
match = cur;
}
cur = cur->_next;
} while (cur != NULL);
if (match == NULL) {
return (E)NULL;
}
return (E)match->_data;
}
#endif // SHARE_UTILITIES_FILTERQUEUE_INLINE_HPP

27
test/hotspot/gtest/utilities/test_filterQueue.cpp
View File

@ -1,5 +1,5 @@
/*
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@ -57,6 +57,8 @@ static void is_empty(FilterQueue<uintptr_t>& queue) {
EXPECT_EQ(queue.is_empty(), true) << "Must be empty.";
EXPECT_EQ(queue.contains(match_1), false) << "Must be empty.";
EXPECT_EQ(queue.contains(match_all), false) << "Must be empty.";
EXPECT_EQ(queue.peek(match_1), (uintptr_t)0) << "Must be empty.";
EXPECT_EQ(queue.peek(match_all), (uintptr_t)0) << "Must be empty.";
EXPECT_EQ(queue.pop(match_all), (uintptr_t)0) << "Must be empty.";
}
@ -68,6 +70,9 @@ TEST_VM(FilterQueue, one) {
EXPECT_EQ(queue.contains(match_1), true) << "Must contain a value.";
EXPECT_EQ(queue.contains(match_all), true) << "Must contain a value.";
EXPECT_EQ(queue.contains(match_even), false) << "Must not contain a value.";
EXPECT_EQ(queue.peek(match_1), (uintptr_t)1) << "Must match 1.";
EXPECT_NE(queue.peek(match_all), (uintptr_t)0) << "Must contain a value.";
EXPECT_EQ(queue.peek(match_even), (uintptr_t)0) << "Must not contain a value.";
EXPECT_EQ(queue.pop(match_all), (uintptr_t)1) << "Must not be empty.";
is_empty(queue);
}
@ -84,6 +89,11 @@ TEST_VM(FilterQueue, two) {
EXPECT_EQ(queue.contains(match_all), true) << "Must contain a value.";
EXPECT_EQ(queue.contains(match_even), true) << "Must contain a value.";
EXPECT_EQ(queue.peek(match_1), (uintptr_t)1) << "Must contain a value.";
EXPECT_EQ(queue.peek(match_2), (uintptr_t)2) << "Must contain a value.";
EXPECT_NE(queue.peek(match_all), (uintptr_t)0) << "Must contain a value.";
EXPECT_NE(queue.peek(match_even), (uintptr_t)0) << "Must contain a value.";
EXPECT_EQ(queue.pop(match_all), (uintptr_t)1) << "Must not be empty.";
EXPECT_EQ(queue.is_empty(), false) << "Must be not empty.";
@ -92,8 +102,14 @@ TEST_VM(FilterQueue, two) {
EXPECT_EQ(queue.contains(match_all), true) << "Must contain a value.";
EXPECT_EQ(queue.contains(match_even), true) << "Must contain a value.";
EXPECT_EQ(queue.peek(match_1), (uintptr_t)0) << "Must not contain a value.";
EXPECT_EQ(queue.peek(match_2), (uintptr_t)2) << "Must contain a value.";
EXPECT_NE(queue.peek(match_all), (uintptr_t)0) << "Must contain a value.";
EXPECT_NE(queue.peek(match_even), (uintptr_t)0) << "Must contain a value.";
queue.push(3);
EXPECT_EQ(queue.peek(match_even), (uintptr_t)2) << "Must not be empty.";
EXPECT_EQ(queue.pop(match_even), (uintptr_t)2) << "Must not be empty.";
queue.push(2);
@ -106,6 +122,11 @@ TEST_VM(FilterQueue, two) {
EXPECT_EQ(queue.contains(match_all), true) << "Must contain a value.";
EXPECT_EQ(queue.contains(match_even), false) << "Must not contain a value.";
EXPECT_EQ(queue.peek(match_3), (uintptr_t)3) << "Must contain a value.";
EXPECT_EQ(queue.peek(match_2), (uintptr_t)0) << "Must be empty.";
EXPECT_EQ(queue.peek(match_all), (uintptr_t)3) << "Must contain a value.";
EXPECT_EQ(queue.peek(match_even), (uintptr_t)0) << "Must be empty.";
EXPECT_EQ(queue.pop(match_even), (uintptr_t)0) << "Must be empty.";
EXPECT_EQ(queue.pop(match_all), (uintptr_t)3) << "Must not be empty.";
@ -128,6 +149,9 @@ TEST_VM(FilterQueue, three) {
EXPECT_EQ(queue.contains(match_all), true) << "Must contain a value.";
EXPECT_EQ(queue.contains(match_even), true) << "Must contain a value.";
EXPECT_EQ(queue.peek(match_even), (uintptr_t)2) << "Must not be empty.";
EXPECT_EQ(queue.peek(match_all), (uintptr_t)1) << "Must not be empty.";
EXPECT_EQ(queue.pop(match_even), (uintptr_t)2) << "Must not be empty.";
EXPECT_EQ(queue.pop(match_even), (uintptr_t)0) << "Must be empty.";
EXPECT_EQ(queue.pop(match_all), (uintptr_t)1) << "Must not be empty.";
@ -160,6 +184,7 @@ public:
}
for (int j = 0; j < 10; j++) {
MutexLocker ml(_lock, Mutex::_no_safepoint_check_flag);
while (_fq->peek(*this) == 0) {}
while (_fq->pop(*this) == 0) {}
}
}