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

[Thomas Schatzl]

Hi,

On Mon, 2016-09-12 at 18:20 -0400, Kim Barrett wrote:
> > 
> > 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?

  they remain open. There is no automatism behind the CR types afaik.

It might be useful to make these sub-tasks real bugs or enhancements,
because many queries do not catch them.

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

I do not think this makes a difference.

Maybe add braces around the body of the "if" if you decide to keep the
volatile.

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

No. All changes to the sparse prt for a given remembered set are
protected by its remembered set lock.
In this case, if you expand the callee list, expansion happens when
adding a card fails, which is only ever called under a lock.

That also explains its performance (or lack of).

- please also fix copyrights, several touched files are still 2015 (I
do not need re-review for that), e.g. mutableSpace.?pp,
parallelScavengeHeap.hpp, psYoungGen.hpp, vmStructs_parallelgc.hpp,
defNewGeneration.hpp, ...

Looks good.

Thanks,
  Thomas