[jdk17] RFR: 8269865: Async UL needs to handle ERANGE on exceeding SEM_VALUE_MAX [v3]

[David Holmes]

On Tue, 6 Jul 2021 22:09:13 GMT, Xin Liu <xliu at openjdk.org> wrote:

>> This patch solved the sempahore overflow issue with errno ERANGE or EOVERFLOW.
>> Previously, we have asymmetric p/v operations for semaphore _sem. Each iteration
>> only decrements _sem 1 but dequeues N messages. If logging threads keep preempting
>> async logging thread, it may cause the value of _sem accumulates until overflow! 
>> 
>> The patch corrects the value of _sem after write(). n messages are dequeued/processed.
>> We need to invoke _sem.wait() max(n-1, 1) time. This ensures that each iteration
>> decrements n instead of 1.
>
> Xin Liu has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - Ignore sem_post failure when _sem of AsyncLogWriter is overflown.
>  - Revert "8269865: Async UL needs to handle ERANGE on exceeding SEM_VALUE_MAX"
>    
>    This reverts commit 269a1580990fa955c1f9d37ce1d04a6349959992.
>  - Revert "Fix build issue on Windows and simplify the loop."
>    
>    This reverts commit e385f42fa4e03510242039518281077e3c9897cf.

I've been thinking about how to handle the ERANGE/EOVERFLOW in the Semaphore API. A default arg is one way. I was also considering a specific signal_xx method that allows overflow; or report to the caller that overflow happened. Not sure what might be best. What you have might be simplest for 17 but there are still some issues that need further investigation. See comments below. I think we need other opinions here too.

I'm surprised macOS has no overflow checks, it looks like its counter will just wrap when it overflows - which would break things, but of course is very unlikely in real use.

Thanks,
David

src/hotspot/os/windows/semaphore_windows.cpp line 42:

> 40: }
> 41: 
> 42: void WindowsSemaphore::signal(uint count, bool ignore_overflow) {

I believe the Windows ReleaseSemaphore can also fail with the equivalent of EOVERFLOW if the count would exceed the maximum allowed. We either need to dig deeper into what value of GetlastError we would see, or else ignore all errors if ignore_overflow is set. I think I can tweak a gtest to see what happens.

src/hotspot/share/logging/logAsyncWriter.hpp line 113:

> 111: // it ignores errno ERANGE and EOVERFLOW.
> 112: class AsyncLogSemaphore : public CHeapObj<mtSynchronizer> {
> 113:   SemaphoreImpl _impl;

I don't think this is necessary, just call wait(1, true) where needed.

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

Changes requested by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk17/pull/216