RFR: 8214556: Crash in DependencyContext::remove_dependent_nmethod still happens

[Erik Österlund]

Hi Kim,

Thanks for the review.

On 2018-12-04 04:43, Kim Barrett wrote:
>> On Dec 3, 2018, at 5:13 PM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>>
>> Hi Kim,
>>
>> Thank you for reviewing this.
>>
>> New full webrev:
>> http://cr.openjdk.java.net/~eosterlund/8214556/webrev.01/
>>
>> Incremental:
>> http://cr.openjdk.java.net/~eosterlund/8214556/webrev.00_01/
> 
> Looks good, and I don't need a new webrev for fixing the claim_cleanup
> comment mentioned below.
> 
> ------------------------------------------------------------------------------
> 265 // We use a safepoint counter to track the safepoint counter the last time a given
> 266 // dependency context was cleaned. GC threads claim cleanup tasks by performing
> 267 // a CAS on this value.
> 268 bool DependencyContext::claim_cleanup() {
> 
> Comment needs to be updated.

Will fix before pushing.

> ------------------------------------------------------------------------------
> 
> Regarding the use of uint64_t as the epoch type (below), I *think*
> wrapping is only a problem because of the comparison in claim_cleanup.
> But I'm not sure why this is using a >= test rather than an == test; I
> think there's only one cleaning pass in progress at a time (e.g.
> cleaning_start/stop pairs never overlap). Even if there can be
> several, the 1/2 range technique used by GlobalCounter would seem
> applicable.
> 
> But in the interest of getting this bug fixed sooner (since it's
> causing a lot of testing noise), I'm okay with leaving the counters as
> uint64_t and discussing it offline.

Yeah, I'm happy discussing this offline.

Thanks,
/Erik

> 
>> On 2018-12-03 22:44, Kim Barrett wrote:
>>> src/hotspot/share/code/dependencyContext.hpp
>>> 81   volatile uint64_t*       _last_cleanup_addr;
>>> ...
>>> 93   static uint64_t                _cleaning_epoch_monotonic;
>>> 94   static volatile uint64_t       _cleaning_epoch;
>>> Do these really need to be uint64_t? It seems like 64bit volatile
>>> (with atomic accesses) could be costly or problematic for a 32bit
>>> platform. (And the Atomic documentation says verification is
>>> required.) Maybe uintx? Though even just uint seems like it might be
>>> adequate. Oh, but a value of zero has special meaning. So if the
>>> global monotonic counter does wrap (which seems entirely possible)
>>> then an extra increment would be needed. Alternatively, start it at 1
>>> and increment by 2?
>>> (The _monotonic suffix should perhaps also be reconsidered if such a
>>> change is made.  "latest"?)
>>
>> I would like to keep this counter uint64_t. If wrapping happens, we are in a lot of trouble. All platforms support atomic load/store on uint64_t, including 32 bit platforms. It's other atomic operations like add-and-fetch that does not necessarily support that on 32 bit platforms. And I only need loads and stores.
>