RFR: 8305819: LogConfigurationTest intermittently fails on AArch64

[gaogao-mem]

On Fri, 21 Apr 2023 09:54:45 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> The list is read in src/hotspot/share/logging/logTagSet.cpp by iterator in src/hotspot/share/logging/logOutputList.hpp.
>> For example, in void LogTagSet::log(LogLevelType level, const char* msg), the list is read as follows:
>>   ```
>> src/hotspot/share/logging/logTagSet.cpp
>> 
>> LogOutputList::Iterator it = _output_list.iterator(level);
>>   LogDecorations decorations(level, *this, _decorators);
>> 
>>   for (; it != _output_list.end(); it++) {
>>     (*it)->write(decorations, msg);
>>   }
>> 
>> And I intended to add loadload in Iterator as follows:
>>    ```
>> src/hotspot/share/logging/logOutputList.hpp
>> 
>> void operator++(int) {
>>           _current = _current->_next;
>> +      OrderAccess::loadload();
>>   }
>>   Iterator iterator(LogLevelType level = LogLevel::Last) {
>>         increase_readers();
>> -    return Iterator(this, _level_start[level]);
>> +    LogOutputNode* start=_level_start[level];
>> +    OrderAccess::loadload();
>> +    return Iterator(this, start);
>>    }
>
> I don't understand why some sort of atomic operation isn't required on the reading side. I'd think something like `Atomic::load_acquire()` would be appropriate here, along with `Atomic::release_store()` on the writing side. I think I need you to explain, with references, why you don't think so.

In the test, two threads read the list while another thread insert to the list . 
The writing thread insert to the list as follows:

void LogOutputList::add_output(LogOutput* output, LogLevelType level) {
  LogOutputNode* node = new LogOutputNode();
  node->_value = output;
  node->_level = level;

  // Set the next pointer to the first node of a lower level
  for (node->_next = _level_start[level];
       node->_next != nullptr && node->_next->_level == level;
       node->_next = node->_next->_next) {
  }

  // Update the _level_start index
  for (int l = LogLevel::Last; l >= level; l--) {
    if (_level_start[l] == nullptr || _level_start[l]->_level < level) {
      _level_start[l] = node;
    }
  }

  // Add the node the list
  for (LogOutputNode* cur = _level_start[LogLevel::Last]; cur != nullptr; cur = cur->_next) {
    if (cur != node && cur->_next == node->_next) {
      cur->_next = node;
      break;
    }
  }
}

On the writing side, it set the value, level and next pointer of the node, then update the _level_start array(_level_start[i] stores the head node of the list whose level is i) and add the node to the list.

And on the reading side, it reads as follows:

void LogTagSet::log(LogLevelType level, const char* msg) {
  // Increasing the atomic reader counter in iterator(level) must
  // happen before the creation of LogDecorations instance so
  // wait_until_no_readers() in LogConfiguration::configure_output()
  // synchronizes _decorations as well. The order is guaranteed by
  // the implied memory order of Atomic::add().
  LogOutputList::Iterator it = _output_list.iterator(level); // obtain the head node of the list from _level_start[level]
  LogDecorations decorations(level, *this, _decorators);

  for (; it != _output_list.end(); it++) {
    (*it)->write(decorations, msg); //crash
  }
}

it++ acquires the next node of the list, and then (*it) reads the value of the node.

In the test, it crash at (*it)->write(decorations, msg), it may due to fault loading on the read side. On the writing side, setting the value of node and adding node to list may be reordered, therefore memory barrier is necessary. While on the reading side, I think getting the pointer of node and reading the value is dependent so memory barrier is unnecessary.
By the way, I have tested millions of times and it hasn't failed when just adding memory barrier on the writing side.

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

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