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

[Erik Österlund]

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/

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

Thanks,
/Erik