RFR: 8343938: TestStressBailout triggers "Should not be locked when freed" assert

[Vladimir Kozlov]

On Mon, 20 Jan 2025 09:45:05 GMT, Theo Weidmann <tweidmann at openjdk.org> wrote:

> This patch addresses a race condition that occurs when the compiler queues shut down because compilation was disabled forever.
> 
> The following changes enforce a scheduling such that the race condition (almost) always occurs and the JVM crashes:
> 
> 
> diff --git a/src/hotspot/share/compiler/compileBroker.cpp b/src/hotspot/share/compiler/compileBroker.cpp
> index 518a53ca26e..61434d1f5f2 100644
> --- a/src/hotspot/share/compiler/compileBroker.cpp
> +++ b/src/hotspot/share/compiler/compileBroker.cpp
> @@ -371,6 +371,10 @@ void CompileQueue::free_all() {
>        MutexLocker ct_lock(current->lock());
>        current->lock()->notify();
>      }
> +    do {
> +      sleep(2);
> +    } while (current->is_free());
> +    tty->print_cr("!! other thread has the lock! let's be fast and call free to crash!");
>      // Put the task back on the freelist.
>      CompileTask::free(current);
>    }
> @@ -1739,6 +1743,8 @@ void CompileBroker::wait_for_completion(CompileTask* task) {
>      free_task = true;
>      while (!task->is_complete() && !is_compilation_disabled_forever()) {
>        ml.wait();
> +      tty->print_cr("I woke up and will give the other thread 10 seconds to crash!");
> +      sleep(10);
>      }
>    }
> 
> 
> The reason for the race condition is that in `CompileQueue::free_all()` the wake up (`notify`, line 372 below) to the thread waiting for the task is immediately succeeded by calling `CompileTask::free` on the task:
> 
> https://github.com/openjdk/jdk/blob/4b4b1e912a3193cc95c956acc770015f707449b1/src/hotspot/share/compiler/compileBroker.cpp#L361-L375
> 
> `CompileTask::free` asserts that the task is not locked:
> 
> https://github.com/openjdk/jdk/blob/4b4b1e912a3193cc95c956acc770015f707449b1/src/hotspot/share/compiler/compileTask.cpp#L65-L69
> 
> However, as shown by the diff above, a scheduling exists, where the waiting thread in `CompileBroker::wait_for_completion` will acquire the lock as part of the wake up (line 1741 below) *before* the other thread calls `CompileTask::free` but *not make any progress* (that is, keeps the lock) before the other thread executes `CompileTask::free`. In this case, the assert triggers as the lock is locked.
> 
> https://github.com/openjdk/jdk/blob/4b4b1e912a3193cc95c956acc770015f707449b1/src/hotspot/share/compiler/compileBroker.cpp#L1737-L1749
> 
> The patch adds a counter to each task to indicate if other threads are waiting for the task. `CompileQueue::free_all` wakes up all waiting threads before continuing and only tries to free the task itself if no other threads were waiting.

This looks good.

src/hotspot/share/compiler/compileBroker.cpp line 372:

> 370:     {
> 371:       MutexLocker ct_lock(current->lock());
> 372:       assert(current->waiting_for_completion_count() <= 1, "more than one thread are waiting for task");

Yes, only one Java thread can wait on compilation finish (`-Xbatch` flag is used).
There is check in `compilation_is_in_queue()` to prevent several Java threads waiting.

src/hotspot/share/compiler/compileBroker.cpp line 385:

> 383:       // Otherwise, by convention, it's the waiters responsibility to free the task.
> 384:       // Put the task back on the freelist.
> 385:       CompileTask::free(current);

It is possible both threads (this and one waiting task) will call `CompileTask::free()`. But it is fine since that method uses `CompileTaskAlloc_lock`.

-------------

Marked as reviewed by kvn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23195#pullrequestreview-2565302455
PR Review Comment: https://git.openjdk.org/jdk/pull/23195#discussion_r1924131295
PR Review Comment: https://git.openjdk.org/jdk/pull/23195#discussion_r1924183008