RFR (M): 8165857: CMS _overflow_list is missing volatile qualifiers

[Kim Barrett]

> On Sep 20, 2016, at 8:57 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
> 
> Hi Kim,
> 
> On 2016-09-19 23:28, Kim Barrett wrote:
>>> On Sep 19, 2016, at 7:43 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>>> 
>>> Hi,
>>> 
>>> This bug is a split out of the larger JDK-8033552 for adding volatile qualifiers to lock-free code.
>>> The motivation is to make our implementation more robust since these kind of issues have struck us a few times too many already.
>>> 
>>> CR: https://bugs.openjdk.java.net/browse/JDK-8165857
>>> Webrev: http://cr.openjdk.java.net/~eosterlund/8165857/webrev.00/
>>> 
>>> This specific CR handles the lack of volatile qualifiers for the _overflow_list fields of the ParallelGC ConcurrentMarkSweepGeneration and ParNewGeneration classes.
>>> The type of the overflow list has been changed to HeapWord *volatile per Kim's recommendation as an oop variant requires changes to the oop class with new overloads, which seems a bit unnecessary considering the overflow lists are hardly proper oop lists either; it has e.g. sentinel values.
>>> 
>>> I went ahead and did the required changes to make the lists HeapWord *volatile, which should make them a little bit more robust.
>>> 
>>> Testing: JPRT
>>> 
>>> I will need a sponsor to push this.
>>> 
>>> Thanks,
>>> /Erik
>> There are a lot more casts than I expected.
>> 
>> After talking to Coleen, I think a better solution is to declare these
>> lists to be "oopDesc* volatile" (rather than my previous suggestion of
>> "HeapWord* volatile" that was used in webrev.00).
>> 
>> That still dodges the need for additional overloads on the oop class
>> when CHECK_UNHANDLED_OOPS is true that one gets if the lists are
>> "volatile oop".  It also removes the need for all the casts to deal
>> with oop/oopDesc* <-> HeapWord*.  There will still be casts needed for
>> the cmpxchg_ptr results, but those are needed anyway.
> 
> Thank you for the review.
> 
> Yes you are right. It worked fine with just oopDesc* volatile, and that did spread considerably less. Here is a new patch with just that:
> http://cr.openjdk.java.net/~eosterlund/8165857/webrev.02/

Why the change to ParNewGeneration::overflow_list() ?
Implicit conversion should handle that.

>> Because of implicit conversions, I think just the "oopDesc* volatile"
>> change to the list heads might build and run without any further
>> changes.  However, it might be desirable to change the list
>> manipulations to explicitly use oopDesc* rather than oop.  Or maybe
>> even consider introducing a new typedef for oopDesc* for each of these
>> cases?
>> 
> 
> Yes, perhaps. If you prefer, I made a webrev with the explicit casts. Here is a new patch with that included:
> http://cr.openjdk.java.net/~eosterlund/8165857/webrev.03/
> ...and incremental from the one with implicit casts above:
> http://cr.openjdk.java.net/~eosterlund/8165857/webrev.02_03/

That's not what I had in mind.  Rather, I was suggesting changing
locals like observed_overflow_list &etc to be "oopDesc*" rather than
"oop", so there are no conversions (implicit or explicit).

I have a mild preference for explicit oopDesc* rather than implicit
oop conversions here.  I wonder what other reviewers might have to
say.