Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8273381: Assert in PtrQueueBufferAllocatorTest.stress_free_list_allocator_vm

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -180,12 +180,18 @@ class BufferNode::TestSupport::ProcessorThread : public JavaTestThread {
{}

virtual void main_run() {
bool shutdown_requested = false;
while (true) {
BufferNode* node = _cbl->pop();
if (node != NULL) {
_allocator->release(node);
} else if (!Atomic::load_acquire(_continue_running)) {
} else if (shutdown_requested) {
return;
} else if (!Atomic::load_acquire(_continue_running)) {
// To avoid a race that could leave buffers in the list after this
Copy link
Contributor

@tschatzl tschatzl Oct 5, 2021

Choose a reason for hiding this comment

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

s/avoid/handle/ (or something similar).

The change does not eliminate (or avoids) the race, but handles the (not logic-breaking) results of the race.

Copy link
Contributor

@tschatzl tschatzl Oct 5, 2021

Choose a reason for hiding this comment

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

Suggested change
// To avoid a race that could leave buffers in the list after this
// To handle a benign race that could leave buffers in the list after this

Copy link
Author

@kimbarrett kimbarrett Oct 5, 2021

Choose a reason for hiding this comment

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

The race involved is not a benign data race, it's a logic race that's not at all benign. I could describe the race in more detail if you want.

Copy link
Contributor

@tschatzl tschatzl Oct 6, 2021

Choose a reason for hiding this comment

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

I meant "benign" wrt to what is actually tested (functionality of the ptrqueuebuffer), but you are right about that my suggestion is the wrong wording. Just leave it as is then.

Thomas

// thread has shut down, continue processing until the list is empty
// *after* the shut down request has been received.
shutdown_requested = true;
}
ThreadBlockInVM tbiv(this); // Safepoint check.
}