RFR: 8305819: LogConfigurationTest intermittently fails on AArch64

[David Holmes]

On Tue, 11 Apr 2023 08:45:56 GMT, gaogao-mem <duke at openjdk.org> wrote:

> LogConfigurationTest*reconfigure*MT* crash intermittently on AArch64.
> According to the crash log and coredump, we found it crash as follows:
> 
> void LogTagSet::log(LogLevelType level, const char* msg) {
>   LogOutputList::Iterator it = _output_list.iterator(level);
>   LogDecorations decorations(level, *this, _decorators);
> 
>   for (; it != _output_list.end(); it++) {
>     (*it)->write(decorations, msg);//crash 
>   }
> }
> 
> In the test, two threads write into the log while another thread dynamically changes the decorators and tags. During this time, the  _output_list will be modified. Because of the relax memory model of aarch64, while adding LogOutputNode to LogOutputList, adding node to list and setting the value of node may be reordered, therefore the read thread may not read the correct value of the node's content. Consequently, storestore memory barrier is needed to ensure the order of writing. 
> By the way, applying this patch may affect performance.
> 
> How to reproduce on Linux aarch64:
> test case
> 
> /* @test
>  * @library /test/lib
>  * @modules java.base/jdk.internal.misc
>  *          java.xml
>  * @run main/native GTestWrapper --gtest_filter=LogConfigurationTest*reconfigure*MT*
>  */
> 
> Crash may occasionally occur after running continuously for 5000 times.

Sorry but I don't like this fix. Not because it is incorrect, but because it is far too low-level for this kind of shared code. The UL code should be correctly synchronized, using locking where needed to ensure full thread-safety. It should not be racy with memory barriers being inserted judiciously as per this PR. If we need a lock-free design for this for performance (do we?) then we should have a lock-free design, where the synchronization points are clear in the API.

Is the solution as simple as the test needing to use the ConfigurationLock?

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13421#pullrequestreview-1403087633
PR Comment: https://git.openjdk.org/jdk/pull/13421#issuecomment-1524524237