[EXTERNAL] A question about memory order on x86_64

[David Holmes]

On 22/06/2021 9:56 am, Liu, Xin wrote:
> Hi, David,
> 
> LogConfiguration::configure_output() is intriguing. That's why I try to
> explain/comment it in that PR.
> 
> For sync logging, the RCU reader counter remains > 0 in "writing"
> period. LogOutputList::Iterator is a RAII class. Its ctor calls
> increase_readers() and dtor calls decrease_readers().
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/logging/logTagSet.cpp#L77
> 
> When the users decide to disable a LogOutput, they need to adjust levels
> first. eg. change tagset 'gc' from DEBUG to OFF. There's a RCU
> synchronization "wait_until_no_readers()" inside of
> LogOutputList::update_output_level().
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/logging/logConfiguration.cpp#L241
> 
> When this function returns, it means that all logsites have seen that
> tagset's level change and all ongoing "writings" have done because
> _active_readers was once back to zero. There're 200+ tagsets. Each
> changed tagset needs a RCU synchronization.
> 
> void LogOutputList::wait_until_no_readers() const {
>    OrderAccess::storeload();
>    while (_active_readers != 0) {
>      // Busy wait
>    }
> }
> 
> Of course, logsites are still active after wait_until_no_readers(), but
> they have seen new levels for the tagsets. They won't emit logs to the
> deleting LogOutput. Therefore, it's safe to delete it.

So they essentially lock out the change while they are writing - which 
means when the writing could block indefinitely then the change is also 
blocked indefinitely:

void LogTagSet::log(LogLevelType level, const char* msg) {
   LogDecorations decorations(level, *this, _decorators);
   for (LogOutputList::Iterator it = _output_list.iterator(level); it != 
_output_list.end(); it++) {
     (*it)->write(decorations, msg);
   }
}

> I would like to make async log a natural extension of current lock-free
> mechanism. The RCU counters only control "enqueuing" periods instead of
> "writing" periods. As long as flush() works, it's still correct.

Where is AsyncLogWriter::write participating in this RCU mechanism? The 
AsyncLogMessages that were enqueued, and have now been dequeued ready to 
be written, have already captured their "output", but there is nothing 
to protect those outputs. You can't guarantee that the flush() call 
handles all pre-existing enqueued messages, before that thread deletes 
the output. You would have to ensure that the write() from flush() waits 
for the AsyncLogWriter thread to complete any in-progress write() call 
before the flushing write() proceeds. I think you need to hold the 
io_sem across the entire write() to ensure it is executed atomically (we 
just need to make sure that can't lead to any deadlock scenarios with 
the other locking mechanisms in use.

David

> thanks,
> --lx
> 
> 
> 
> On 6/21/21 4:28 PM, David Holmes wrote:
>> But we can't do that as we could block indefinitely while writing.
>>
>> Backing up a step lets ignore async logging for a moment. How does
>> synchronous logging deal with this problem? What ensures that a log
>> request can't use an output that is being concurrently deleted by a
>> configuration change?