RFR: 8267926: AsyncLogGtest.java fails on assert with: decorator was not part of the decorator set specified at creation. [v3]

[David Holmes]

On Mon, 31 May 2021 17:09:48 GMT, Xin Liu <xliu at openjdk.org> wrote:

>> The root cause of the intermittent failure is that _decorators in LogDecorations 
>> may be inconsistent with LogOutput::_decorators. It could happen when gtest disables a
>> Log output via set_log_config(TestLogFileName, "all=off").
>> 
>> Since we copy the entire logDecorations, it's reasonable to copy _decorator as well. 
>> LogDecorator is a bitmask of uint. It's even smaller than a reference on LP64 platforms.
>> 
>> This patch not only fixes intermittent crash in JDK-8267926, but also fixes JDK-8267952.
>> Two hazards of async logging in `LogConfiguration::configure_output` are eliminated. 
>> This patch also adds a storestore barrier to make sure it's safe on weak consistency machines.
>
> Xin Liu has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Increase reader's counter before creating LogDecoration.

A few comments below - mostly typos - but I think the main race still exists.

I need more time to try and digest the other changes and understand the existing MT-safety aspects.

Meanwhile I've submitted for tiers 1-4 testing.

Thanks,
David

src/hotspot/share/logging/logAsyncWriter.cpp line 105:

> 103: 
> 104:     if (*counter > 0) {
> 105:       LogDecorations decorations(LogLevel::Warning, none::tagset(), LogDecorators::All);

This looks odd to use LogDecorators::All - what exactly will that produce?

src/hotspot/share/logging/logConfiguration.cpp line 266:

> 264:   // see the new tags and decorators. It's worth noting that the synchronization happens even level doesn't change.
> 265:   //
> 266:   // LogDecorator is a set of decorators represented in a uint. sizeof(uinit) is not greater than a machine word,

Typo: uinit

src/hotspot/share/logging/logConfiguration.cpp line 267:

> 265:   //
> 266:   // LogDecorator is a set of decorators represented in a uint. sizeof(uinit) is not greater than a machine word,
> 267:   // so store of it is atomic on the mainstream processors. I.e. readers see either its older value or new value.

We should use Atomic::store and Atomic::load regardless.

src/hotspot/share/logging/logConfiguration.cpp line 273:

> 271:   //
> 272:   // There are 2 hazards in async logging. A flush operation guarantees to all pending messages in buffer are written
> 273:   // before returning. Therefore, the hardards won't appear. It's a nop if async logging is not set.

Typo: hardards

src/hotspot/share/logging/logConfiguration.cpp line 279:

> 277:   //
> 278:   AsyncLogWriter::flush();
> 279: 

As soon as this completes, new logging requests could have been enqueued, thus restoring the hazard.

src/hotspot/share/logging/logDecorators.cpp line 30:

> 28: template <LogDecorators::Decorator d>
> 29: struct AllBitmask {
> 30:   // Use recursive template deduction to calculcate the bitmask of all decorations.

typo: calculcate

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

PR: https://git.openjdk.java.net/jdk/pull/4257