RFR: 8305819: LogConfigurationTest intermittently fails on AArch64 [v2]

[Dan Gao]

On Mon, 8 May 2023 04:22:40 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Dan Gao has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   comment fixme
>
> src/hotspot/share/logging/logOutputList.cpp line 132:
> 
>> 130:   // Prevent the reordering of adding node to list and setting the value of node.
>> 131:   // Such a reordering would lead to reading incorrect values.
>> 132:   OrderAccess::release();
> 
> I think if we were to treat this code as the usual pattern of allowing lock-free iteration of a linked-list then we would do a release_store on line 137 and a release_store on line 144. We could then factor the release out ahead of the two loops and place it in the current position. However we would also normally use Atomic::store for the actual stores in the loops, and we would use Atomic::load for the loads in the loops. So:
> 
> // To allow lock-free iteration of the output list the updates in the loops
> // below require release semantics.
> OrderAccess::release();
> 
>  // Update the _level_start index
>   for (int l = LogLevel::Last; l >= level; l--) {
>     LogOutputNode* lnode = Atomic::load(&_level_start[l]);
>     if ( lnode == nullptr || lnode->_level < level) {
>       Atomic::store(&_level_start[l], node);
>     }
>   }
>   // Add the node the list
>   for (LogOutputNode* cur = Atomic::load(&_level_start[LogLevel::Last]); 
>         cur != nullptr; 
>         cur = Atomic::load(&cur->_next)) {
>     if (cur != node && Atomic::load(&cur->_next) == node->_next) {
>       Atomic::store(&cur->_next, node);
>       break;
>     }
> }
> 
> Logically the intent is that `_level_start` array and the node `_next` field are treated as special "atomic" types and so all accesses must be Atomic::xx. But I suspect in this case that would involve quite a lot of changes.

I agree with you. In the usual pattern of allowing lock-free iteration of a linked-list, atomic access is needed especially when store the  `_level_start` and `_next`. However it may require many changes if all accesses of them be atomic. Therefore, I only made the changes as you suggested.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13421#discussion_r1188434975