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

[David Holmes]

On 14/06/2021 4:59 pm, Xin Liu wrote:
> On Mon, 14 Jun 2021 04:51:28 GMT, Xin Liu <xliu at openjdk.org> wrote:
> 
>>> Support dynamic reconfiguration for async logging. 2 unittests are provided.
>>> The regression test discovers a race condition in LogTagSet::log() even with
>>> synchronous logging. It's not MT-safe if context switch happens between the
>>> creation of LogDecorations  and LogOutputList::Iterator. fixed.
>>
>> Xin Liu has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>>
>>   - Merge branch 'master' into JDK-8267952
>>   - Add comments in LogTagSet::log().
>>   - 8267952: async logging supports to dynamically change tags and decorators
> 
> hi, Kim,
> 
> Thanks for the head-up.
> 
> I did prepare a patch to support Atomic::store. I think I followed what you guys did.
> https://cr.openjdk.java.net/~xliu/8267952/atomic_store_LogDecorators.diff
> 
> Because this patch uses some C++ tricks(well, advanced techniques), I have to convince myself that it's necessary. so far, I found that it always generates the same store instructions in x86_64.
> 
> @dholmes-ora
> The biggest concern is the MT-safety. Users can potentially change unified logging configurations anytime in the presence of dcmd. That's why I added 2 concurrent gtests to demonstrate it's MT-safe no matter it's synchronous logging or async logging.  We must be sure that SEGV fault never happen.
> 
> As far as I known, there are 2 reasons why we can see that crash sight.
> 
> 1.  `LogConfiguration::configure_output` may be still lack of a memory barrier somewhere.
> Some logs directed to output[idx] can still enqueue after `AsyncLogWriter::flush()`. delete_output(idx) is the hazard.
> 
> If so, synchronous logging should observe the same problem. The fact is we only observed the crash in AsyncLog Thread.
> Further, I ran `LogConfigurationTest.reconfigure_tags_MT` a couple of hours on x86_64 and aarch64 individually.  I still can't capture the crash.
> 
> 2. LogFileOutput::rotate() may nullify _stream.
> yes, I missed bugfix JDK-8268165, but this patch uses "filecount=0" to prevent log from rotating in  `LogConfigurationTest.reconfigure_tags_MT`.  In my understanding, rotate() should not be invoked by this test.
> 
> I have merged the master branch, which includes 8268165. Could you try it again and see if it will still crash at the same place?

The updated branch passes the tier6 testing that previously failed. If 
rotate() should not be being used by the test, yet the rotate() fix has 
addressed the crashes, then apparently something else must be happening 
here that is not fully understood.

David
-----

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