RFR: 8166862: CMS needs klass_or_null_acquire

[David Holmes]

On 5/10/2016 9:12 AM, Kim Barrett wrote:
>> On Oct 4, 2016, at 5:04 AM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> Hi Kim,
>>
>> On 2/10/2016 7:29 AM, Kim Barrett wrote:
>>> Please review this change to the CMS collector to replace uses of
>>> klass_or_null with klass_or_null_acquire where needed.
>>>
>>> [Reviewing on hotspot-dev; only GC code is involved, but some non-GC
>>> folks have expressed interest.]
>>>
>>> All non-assert uses of klass_or_null are being replaced with the
>>> acquire variant.  In most cases, the size is being accessed soon
>>> after, and the acquire is needed to ensure order of klass and possible
>>> inline size access.  In a few cases, the need for acquire is less
>>> immediately obvious:
>>>
>>> - CompatibleFreeListSpace::block_is_obj - Size access is not lexically
>>>  apparent, but callers access the size or other data ordered with the
>>>  klass.
>>>
>>> - MarkFromRootsClosure::do_bit and ParMarkFromRootsClosure::do_bit:
>>>  These perform other operations that need to be properly ordered wrto
>>>  the klass access.
>>
>> Can you point me to the release-store that is being paired with these load-acquires please?
>
> It’s the release_set_klass recently added here:
> http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/fd16b627ebc5

Okay so this generally just sync's with object allocation.

I don't have anywhere near enough knowledge of the GC code to know how 
these read paths may interact with the object allocation code, to 
determine if acquire semantics are needed. So as I said the change seem 
fine and are perfectly "correct" in a functional sense.

Thanks,
David

>> Superficially this all seems fine and can't have any correctness issues, it is just a matter of where acquire semantics are actually needed. And it is hard to tell that without understanding exactly how the different read-paths may execute relative to the write-path.
>
> In MarkFromRootsClosure::do_bit, the first one is protecting the call to scanOopsInOop.  The second is protecting the call to mark_range, which might not be strictly necessary, but seems like this should be pretty rare.  And being consistent about the use of klass_or_null_acquire in non-assert contexts seems of value.  Though I admit to finding this block of code confusing; it’s unconditional in a product build, but only executed if !_verifying in a debug build.  Strange… but it’s been like that “forever”.
>
> In ParMarkFromRootsClosure::do_bit, protecting call to scan_oops_in_oop.
>
> The scanners need to be protected against reading an uninitialize length or uninitialized fields / array elements.
>