RFR: 8267952: async logging supports to dynamically change tags and decorators

[David Holmes]

On Wed, 9 Jun 2021 18:51:14 GMT, Xin Liu <xliu at openjdk.org> wrote:

>> src/hotspot/share/logging/logTagSet.cpp line 79:
>> 
>>> 77:   LogDecorations decorations(level, *this, _decorators);
>>> 78: 
>>> 79:   for (; it != _output_list.end(); it++) {
>> 
>> The reordering here needs a comment, else someone will be tempted to restore it to the original form.
>
> The order does matter. There is a very tricky race condition.  Here is the original order.  
> 
> 
> A: LogDecorations decorations(level, *this, _decorators);
>> B: LogOutputList::Iterator it = _output_list.iterator(level);
> C: for (; it != _output_list.end(); it++) {
> 
> 
> The context switch may happen right before stmt B.  Stmt A has been executed and it used the old _decorators. 
> However, the global counter hasn't increased yet so `wait_until_no_readers()` doesn't take any effect because the reader counter is zero.  Another thread which is executing `LogConfiguration::configure_output` can change LogOutput::decorators.  If the user selects more decorators than old,  `LogFileStreamOutput::write_decorations`
> will print more decorators which are not initialized in `LogDecorations` ctor.
> 
> The newly added LogConfigurationTest.reconfigure_decorators_MT unveils this issue. It could happen with both synchronous logging or async logging.  Tier-1 test on github workflow uses 2 cores. it's easy to trigger this problem if Linux preemptive is enabled.

I understand there is a race. I'm asking you to add a comment explaining there is a race so that someone is not tempted to reorder these statements again. That said it seems to me that we also require a `loadload` barrier between these (which would pair with the storestore barrier in `LogConfiguration::configure_output`).

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

PR: https://git.openjdk.java.net/jdk/pull/4408