Navigation Menu

Skip to content

Commit

Permalink
8266557: assert(SafepointMechanism::local_poll_armed(_handshakee)) fa…
Browse files Browse the repository at this point in the history
…iled: Must be

Reviewed-by: pchilanomate, dcubed
  • Loading branch information
robehn committed Jun 9, 2021
1 parent 4d1cf51 commit 2bfd708
Show file tree
Hide file tree
Showing 6 changed files with 116 additions and 75 deletions.
80 changes: 38 additions & 42 deletions src/hotspot/share/runtime/handshake.cpp
Expand Up @@ -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
}
Expand Down Expand Up @@ -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) {
Expand All @@ -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() {
Expand All @@ -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();
Expand All @@ -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
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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");

if (op == match_op) {
pr_ret = HandshakeState::_succeeded;
}
assert(op != NULL, "Must have an op");
assert(SafepointMechanism::local_poll_armed(_handshakee), "Must be");
assert(op->_target == NULL || _handshakee == op->_target, "Wrong thread");

op->prepare(_handshakee, current_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");

_active_handshaker = current_thread;
op->do_handshake(_handshakee);
_active_handshaker = NULL;
op->prepare(_handshakee, current_thread);

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() {
Expand Down
43 changes: 17 additions & 26 deletions src/hotspot/share/runtime/handshake.hpp
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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);
Expand All @@ -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 {
Expand All @@ -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:
Expand Down
3 changes: 2 additions & 1 deletion src/hotspot/share/runtime/safepointMechanism.cpp
Expand Up @@ -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);
}
Expand All @@ -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 (;;) {
Expand Down
11 changes: 7 additions & 4 deletions src/hotspot/share/utilities/filterQueue.hpp
@@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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 changes: 26 additions & 1 deletion src/hotspot/share/utilities/filterQueue.inline.hpp
@@ -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
Expand Down Expand Up @@ -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 changes: 26 additions & 1 deletion test/hotspot/gtest/utilities/test_filterQueue.cpp
@@ -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
Expand Down Expand Up @@ -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.";
}

Expand All @@ -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);
}
Expand All @@ -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.";
Expand All @@ -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);
Expand All @@ -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.";

Expand All @@ -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.";
Expand Down Expand Up @@ -160,6 +184,7 @@ class FilterQueueTestThread : public JavaTestThread {
}
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) {}
}
}
Expand Down

1 comment on commit 2bfd708

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.