[EXTERNAL] A question about memory order on x86_64

[David Holmes]

On 22/06/2021 10:39 am, David Holmes wrote:
> 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.

I made the change to hold the io_sem for all of write() and the tests 
passed in tiers 6 and 7.

David
-----

> 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?