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

[Erik Österlund]

Hi Kim,

On 2016-09-20 20:25, Kim Barrett wrote:
>> 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.

Yes it sure will handle that. Updated webrev of the minimalist version:
http://cr.openjdk.java.net/~eosterlund/8165857/webrev.01_v2/

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

Okay, I tried that idea, here is the result:
http://cr.openjdk.java.net/~eosterlund/8165857/webrev.03_v2/

I'm not sure if this is a huge improvement in not spreading too much 
compared to the HeapWord version.
What do you think?

Thanks,
/Erik