[EXTERNAL] A question about memory order on x86_64

[David Holmes]

On 23/06/2021 3:11 am, Liu Xin wrote:
>  > 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.
> 
> Yes,  this kind of synchronization is biased to readers(cheap). updater 
> has to wait(expensive).
> I think it's fair enough, changing log configuration is rare.
> 
>  > Where is AsyncLogWriter::write participating in this RCU mechanism?
> 
> when  logsites enqueue messages to the async buffer, they use RCU 
> counters as well.
> 
> The intention of flush()  is to flush all pending messages, either by 
> flush() or by async log thread.
> _io_sem is a handshake protocol for two stakeholders.  You showed me 
> that this so-called "protocol" was broken if one thread invokes multiple 
> flush() in a row.
> 
> I changed flush() yesterday.  It just inserts a special token to the 
> buffer and waits for completion. This guarantees to
> serialize multiple flush() calls.  I have run overnight, no problem is 
> identified.
> 
> How about this change?
> https://openjdk.github.io/cr/?repo=jdk&pr=4408&range=05#udiff-0-src/hotspot/share/logging/logAsyncWriter.cpp 
> <https://openjdk.github.io/cr/?repo=jdk&pr=4408&range=05#udiff-0-src/hotspot/share/logging/logAsyncWriter.cpp>
> 
>  > I made the change to hold the io_sem for all of write() and the tests
> passed in tiers 6 and 7.
> 
> What did you change?

I did io_sem.wait at the start of write(), so that it is executed 
atomically, to completion.

David
-----

> P/V operations perform on io_sem(1) once every write(), but I feel it's 
> difficult to control the scheduler.
> 
> 
> 
> 
> On Mon, Jun 21, 2021 at 5:39 PM David Holmes <david.holmes at oracle.com 
> <mailto:david.holmes at oracle.com>> 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
>     <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
>     <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?
>