RFR: 8277486: NMT: Cleanup ThreadStackTracker

[Zhengyu Gu]

On Tue, 23 Nov 2021 13:54:57 GMT, Zhengyu Gu <zgu at openjdk.org> wrote:

>>> _thread_counter is updated with ThreadCritical lock, but is read without the lock. Change to atomic update to ensure reader will not read stale value.
>> 
>> Use of atomic updates with MO_RELAXED will not affect whether an Atomic::load sees a stale value or not.
>
>> > _thread_counter is updated with ThreadCritical lock, but is read without the lock. Change to atomic update to ensure reader will not read stale value.
>> 
>> Use of atomic updates with MO_RELAXED will not affect whether an Atomic::load sees a stale value or not.
> 
> I thought we settled this in [PR 6065](https://github.com/openjdk/jdk/pull/6065), as you stated:
> 
>> I see now that the C++ "Modification Order" definition requires the write to the counter to be (for want of a better term) "immediately visible" to any subsequent read - so no stale value could be read.
> 
> If you are referring the inaccurate read of the counter w/o the lock, then yes, it is a general problem with NMT, where various counters are *not* strictly sync'd to avoid taking locks.

> Sorry @zhengyu123 but every time I re-read the C++ memory model stuff I get a different take on exactly what different MO's provide. The write-read coherence in the modification order requires that the write happens-before the read. And the inter-thread happens-before ordering then states the five conditions, any of which, establish the happens-before ordering. But I do not see any of those conditions being met for the Atomic::load that does the read and the Atomic::inc or dec that last updated the counter. They do not synchronize-with each other (unlike C++ atomics), there is no dependency order and no obvious transitive happens-before or synchronizes-with ordering. The atomic load is AFAICS no different from a memory ordering or visibility perspective, to a plain load as it takes no MO parameter and is not specified to provide any specific MO semantics - unlike in C++ where the default is seq-cst. So unless the Atomic::inc/dec provide some form of full memory synchronization barr
 ier, the load need not see the most recent write.

@dholmes-ora I am with you. In fact, our internal discussion started when I questioned assertions in NMT, e.g. `MemoryCounter::deallocation()` it asserts `allocation count > 0` and `allocation amount >= deallocation amount`, where loads are pain loads.

>From another perspective, in [PR 6065](https://github.com/openjdk/jdk/pull/6065), I mentioned that atomic r-m-w update at single location is total order, guaranteed by coherence protocol (AFAICT, all supported platforms are coherence based multiple processor systems).

For a processor to perform atomic r-m-w operation, it has to acquire corresponding cache line in exclusive state. Therefore, other processors who happen to own the cache line, have to invalidate the cache line before atomic operation can be performed.

After atomic r-m-w operation, when other thread tries to load the value, coherence protocol ensures that it sees new value.

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

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