RFR(M): 8033552: Fix missing missing volatile specifiers in CAS operations in GC code

[Kim Barrett]

> On Sep 13, 2016, at 11:28 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
> 
> Hi Kim,
> 
> New webrev with your proposed changes:
> http://cr.openjdk.java.net/~eosterlund/8033552/webrev.05/

Looks good.

A few replies inline below.

> On 2016-09-13 00:20, Kim Barrett wrote:
>> src/share/vm/gc/shared/taskqueue.hpp
>>  129     void increment() volatile {
>>  130       _fields._top = increment_index(_fields._top);
>>  131       if (_fields._top == 0) ++_fields._tag;
>>  132     }
>> 
>> Perhaps this should be
>> 
>>   uint new_top = increment_index(_fields._top);
>>   _fields._top = new_top;
>>   if (new_top == 0) ++_fields._tag;
>> 
>> Although if it matters then we probably have other
>> problems... e.g. adding a volatile qualifier doesn't make this
>> thread-safe. And if it doesn't need to be thread-safe, then why add
>> the volatile qualifier?
> 
> I do not have strong opinions on this one. It should not matter as you say, but it seemed more consistent when the other member functions above were implemented with volatile.
> I decided to withdraw that volatile specifier on this one as it seems you would prefer that.

The addition of the volatile qualifier suggests a need for
thread-safety, but the existing code isn't (nor is my suggested
change, though it's a step along the way).

So I looked at how increment() is used, and the volatile isn't needed.
It is only applied to a freshly made copy.

So I accept the withdrawal of that change.

>> -----------------------------------------------------------------------------
>> [This question from my initial review seems not covered anywhere.  I
>> don't know if there is actually a bug here.  If there is, it should be
>> a new CR.]
>> 
>> src/share/vm/gc/g1/sparsePRT.hpp
>>  -- no direct comments, however:
>> 
>> src/share/vm/gc/g1/sparsePRT.cpp
>> 
>> Can add_to_expanded_list be called by multiple threads with the same
>> argument at the same time?  If so, it doesn't look correct.  It has a
>> comment saying multiple expansions are possible, but should only be
>> put on list once.  But it's not clear whether that can happen from
>> multiple threads at the same time.  The check expanded / set expanded
>> dance is not thread safe.
> 
> I did answer it, but sorry if I was not explicit enough that my comment was linked to this. Yes it can be called by multiple threads. However, I can not see what is wrong about that.
> If you are thinking about the classic ABA problem, I can not see how that would be triggered, because the list is operated on in split phases protected by a safepoint where it's either add-only (outside of safepoint, while adding sparsePRT data), or remove-only (inside of safepoint, before card scanning).
> 
> Having multiple competing threads only adding or only removing from the list without explicit ABA-protection mechanisms should be safe.
> The only remaining problem is the variable _expanded. And it is changed only on the current sparsePRT, while under the per-region lock in OtherRegionsTable::add_reference() calling SparsePRT::add_card, calling SparsePRT::add_to_expanded_list, ultimately calling add_to_expanded_list that we are currently looking at.
> 
> So the data for the specific SparsePRT is changed under a per-region lock, and it's added to a list (which can happen concurrent to other SparsePRTs being expanded), but that is fine as the ABA-problem can't occur as the entries do not get deleted until after a safepoint synchronization, when the list is consumed during cleanup before card scanning.

The point is that there's a comment that it should only be added to
the list once, but the mechanism for doing so is not thread-safe, so
unless there is a lock somewhere protecting the call (which Thomas
provided a pointer to - thanks Thomas), then the constraint described
by the comment may not be met.