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

[Kim Barrett]

> On Sep 12, 2016, at 11:14 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
> 
> Hi Kim,
> 
> Thank you for your long and thorough review of my slightly too long patch. I have rebased the patch on the latest hotspot sources and split the patch into smaller chunks and CRs.
> I will start sending them out soon so we can get this over and done with.
> 
> In general, the card table changes have been backed out so we can defer that to another day. The changes where you had no comments will remain in the CR. The rest have been split out to new CRs.
> 
> Here is a list of the new related CRs:
> 
> 1) g1ConcurrentMark is missing volatile specifiers for _finger.
>    Bug: https://bugs.openjdk.java.net/browse/JDK-8165856
> 2) CMS _overflow_list is missing volatile specifiers.
>    Bug: https://bugs.openjdk.java.net/browse/JDK-8165857
> 3) heapRegionManager is missing volatile specifier for _claims.
>    Bug: https://bugs.openjdk.java.net/browse/JDK-8165858
> 4) gcTaskThread is missing volatile specifier and barriers for _time_stamps.
>    Bug: https://bugs.openjdk.java.net/browse/JDK-8165859
> 5) workgroup classes are missing volatile specifiers for lock-free code.
>    Bug: https://bugs.openjdk.java.net/browse/JDK-8165860

Thanks for doing this.

However, a proceedural issue: These new CRs have been marked as
subtasks of the main CR.  I'm not sure what happens when the main CR
is resolved (by pushing something like the updated change set) but
subtasks remain open?

> Here is a new webrev for the rebased patch with the above changes and card table changes moved out:
> http://cr.openjdk.java.net/~eosterlund/8033552/webrev.01/

------------------------------------------------------------------------------  
src/share/vm/gc/parallel/mutableSpace.hpp
  72   HeapWord* volatile* top_addr()                    { return &_top; }

Body out of alignment with nearby aligned function bodies.

------------------------------------------------------------------------------ 
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?

------------------------------------------------------------------------------
[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.

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