RFR: 8264123: add ThreadsList.is_valid() support

[David Holmes]

On Tue, 30 Mar 2021 16:17:27 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> src/hotspot/share/runtime/threadSMR.cpp line 285:
>> 
>>> 283:     }
>>> 284: 
>>> 285:     guarantee(ThreadsList::is_valid(current_list), "current_list="
>> 
>> Why a guarantee and not an assert? What is the cost of doing this check?
>
> This is a guarantee() instead of an assert() because this failure mode
> is rarely caught with 'release' bits and very, very rarely caught with
> 'fastdebug' or 'slowdebug' bits. It's a very tight race.
> 
> My plan is to keep this as a guarantee() for April and switch it to an
> assert in mid to late May.
> 
> I don't have data on what this correctness check costs, but it's just
> a value check on the new `_magic` field value. Very much like other
> "magic" field value checks that we have all over the VM...

I should have edited this comment once I saw what the actual check was.

Keeping this as a guarantee initially then switching to assert seems a good strategy.

>> src/hotspot/share/runtime/threadSMR.hpp line 209:
>> 
>>> 207:   bool includes(const JavaThread * const p) const;
>>> 208: 
>>> 209:   static bool is_valid(ThreadsList* list) { return list->_magic == THREADS_LIST_MAGIC; }
>> 
>> This seems like a necessary, but not sufficient, check for validity. Unless we zap memory when these things are freed I don't see how this ensures validity.
>
> You missed L652 in the `ThreadsList::~ThreadsList()` above:
> 
>  _magic = 0xDEADBEEF;
> 
> When the destructor runs, the magic value is stomped. I've captured
> failures where the is_valid() check observes 0xDEADBEEF and also
> failures where the ThreadsList has been recycled into something else
> and the `_magic` value is some other value that doesn't match
> `THREADS_LIST_MAGIC`.

Sorry yes I did miss it - I misread the diff due to hidden lines.

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

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