RFR(XS): 8164038: Missing volatile keyword at CardTableRS::write_ref_field_gc_par()

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(XS): 8164038: Missing volatile keyword at CardTableRS::write_ref_field_gc_par()

sangheon.kim@oracle.com
Hi all,

Could I have some reviews for this tiny change that adds volatile qualifier?

Currently without 'volatile' we don't have any problem but I would like to add it for the code readability.

--------
* Background
Initially this problem occurred between b127 and b134 (<= b126 or >= b135 are okay).
b127 includes "JDK-8155949, Support relaxed semantics in cmpxchg" and b135 includes "JDK-8157904: Atomic::cmpxchg for jbyte is missing a fence on initial failure".
The patch for JDK-8155949 changed 'jbyte Atomic::cmpxchg()' which eventually affected not to reload the variable 'entry' at CardTableRS::write_ref_field_gc_par(), so gc/cms/TestBubbleUpRef.java failed on ARM machine. And the patch for JDK-8157904 fixed it.
But without the patch for JDK-8157904, just adding 'volatile' also fixes this problem.

After founding the root cause, I saw another CR, "JDK-8033552: Fix missing missing volatile specifiers in CAS operations in GC code" also includes adding 'volatile' from its initial patch so I closed this CR but the final patch didn't include this part intentionally. After discussing with the author of JDK-8033552, Erik Österlund, we decided to fix under this CR.

CR: https://bugs.openjdk.java.net/browse/JDK-8164038
Webrev: http://cr.openjdk.java.net/~sangheki/8164038/webrev.0
Testing: JPRT

Thanks,
Sangheon
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8164038: Missing volatile keyword at CardTableRS::write_ref_field_gc_par()

Thomas Schatzl
Hi Sangheon,

On Tue, 2017-03-07 at 17:14 -0800, sangheon wrote:
> Hi all,
>
> Could I have some reviews for this tiny change that adds volatile
> qualifier?
>
> Currently without 'volatile' we don't have any problem but I would
> like to add it for the code readability.
>

  looks good.

Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8164038: Missing volatile keyword at CardTableRS::write_ref_field_gc_par()

sangheon.kim@oracle.com
Hi Thomas,

On 03/08/2017 02:57 AM, Thomas Schatzl wrote:

> Hi Sangheon,
>
> On Tue, 2017-03-07 at 17:14 -0800, sangheon wrote:
>> Hi all,
>>
>> Could I have some reviews for this tiny change that adds volatile
>> qualifier?
>>
>> Currently without 'volatile' we don't have any problem but I would
>> like to add it for the code readability.
>>
>    looks good.
Thanks for the review.

Sangheon


>
> Thomas
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8164038: Missing volatile keyword at CardTableRS::write_ref_field_gc_par()

Kim Barrett
In reply to this post by sangheon.kim@oracle.com
> On Mar 7, 2017, at 8:14 PM, sangheon <[hidden email]> wrote:
>
> Hi all,
>
> Could I have some reviews for this tiny change that adds volatile qualifier?
>
> Currently without 'volatile' we don't have any problem but I would like to add it for the code readability.
>
> --------
> * Background
> Initially this problem occurred between b127 and b134 (<= b126 or >= b135 are okay).
> b127 includes "JDK-8155949, Support relaxed semantics in cmpxchg" and b135 includes "JDK-8157904: Atomic::cmpxchg for jbyte is missing a fence on initial failure".
> The patch for JDK-8155949 changed 'jbyte Atomic::cmpxchg()' which eventually affected not to reload the variable 'entry' at CardTableRS::write_ref_field_gc_par(), so gc/cms/TestBubbleUpRef.java failed on ARM machine. And the patch for JDK-8157904 fixed it.
> But without the patch for JDK-8157904, just adding 'volatile' also fixes this problem.
>
> After founding the root cause, I saw another CR, "JDK-8033552: Fix missing missing volatile specifiers in CAS operations in GC code" also includes adding 'volatile' from its initial patch so I closed this CR but the final patch didn't include this part intentionally. After discussing with the author of JDK-8033552, Erik Österlund, we decided to fix under this CR.
>
> CR: https://bugs.openjdk.java.net/browse/JDK-8164038
> Webrev: http://cr.openjdk.java.net/~sangheki/8164038/webrev.0
> Testing: JPRT
>
> Thanks,
> Sangheon

looks good.

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8164038: Missing volatile keyword at CardTableRS::write_ref_field_gc_par()

sangheon.kim@oracle.com
Hi Kim,

Thanks for the review!

Sangheon


On 03/08/2017 10:12 AM, Kim Barrett wrote:

>> On Mar 7, 2017, at 8:14 PM, sangheon <[hidden email]> wrote:
>>
>> Hi all,
>>
>> Could I have some reviews for this tiny change that adds volatile qualifier?
>>
>> Currently without 'volatile' we don't have any problem but I would like to add it for the code readability.
>>
>> --------
>> * Background
>> Initially this problem occurred between b127 and b134 (<= b126 or >= b135 are okay).
>> b127 includes "JDK-8155949, Support relaxed semantics in cmpxchg" and b135 includes "JDK-8157904: Atomic::cmpxchg for jbyte is missing a fence on initial failure".
>> The patch for JDK-8155949 changed 'jbyte Atomic::cmpxchg()' which eventually affected not to reload the variable 'entry' at CardTableRS::write_ref_field_gc_par(), so gc/cms/TestBubbleUpRef.java failed on ARM machine. And the patch for JDK-8157904 fixed it.
>> But without the patch for JDK-8157904, just adding 'volatile' also fixes this problem.
>>
>> After founding the root cause, I saw another CR, "JDK-8033552: Fix missing missing volatile specifiers in CAS operations in GC code" also includes adding 'volatile' from its initial patch so I closed this CR but the final patch didn't include this part intentionally. After discussing with the author of JDK-8033552, Erik Österlund, we decided to fix under this CR.
>>
>> CR: https://bugs.openjdk.java.net/browse/JDK-8164038
>> Webrev: http://cr.openjdk.java.net/~sangheki/8164038/webrev.0
>> Testing: JPRT
>>
>> Thanks,
>> Sangheon
> looks good.
>