[aarch64-port-dev ] RFR(S): 8248851: CMS: Missing memory fences between free chunk check and klass read

[David Holmes]

Hi Felix,

On 17/07/2020 1:05 pm, Yangfei (Felix) wrote:
> Hi,
> 
>    Thanks for the suggestions.  It makes sense to me.
>    BTW: OrderAccess::loadload() and OrderAccess::acquire() both map to the same instruction for aarch64: dmb ishld.
>    Updated webrev:
>      http://cr.openjdk.java.net/~fyang/8248851-8u/webrev.01/
>      http://cr.openjdk.java.net/~fyang/8248851-11u/webrev.01/
>      http://cr.openjdk.java.net/~fyang/8248851-13u/webrev.01/
> 
>    Performed the same test as before, result looks good.
>    Does it look better?

It certainly looks better to me. (This code still puzzles me somewhat 
but I'm not going to expend more time trying to understand it more broadly.)

Thanks,
David
-----

> Felix
> 
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Friday, July 17, 2020 6:30 AM
>> To: Andrew Haley <aph at redhat.com>; Yangfei (Felix)
>> <felix.yang at huawei.com>; Kim Barrett <kim.barrett at oracle.com>
>> Cc: jdk8u-dev at openjdk.java.net; hotspot-gc-dev at openjdk.java.net;
>> aarch64-port-dev at openjdk.java.net; jdk-updates-dev at openjdk.java.net
>> Subject: Re: [aarch64-port-dev ] RFR(S): 8248851: CMS: Missing memory
>> fences between free chunk check and klass read
>>
>> On 16/07/2020 11:43 pm, Andrew Haley wrote:
>>> On 16/07/2020 10:16, David Holmes wrote:
>>>> Seems to me that you want OrderAccess::loadload() barriers to order
>>>> the loads, not OrderAccess::acquire(). You should only use acquire
>>>> semantics to pair with a corresponding release operation.
>>>
>>> I agree, but it's unlikely to matter in practice.
>>
>> In terms of what underlying hardware barriers get used, no it won't
>> (likely) matter in practice.
>>
>> But from a code understandability perspective it matters very much IMHO.
>> We have been actively trying to ensure that the right OrderAccess APIs are
>> used, in the right way and only where actually needed. An acquire without a
>> corresponding release shows a lack of understanding and leads to confusion
>> for other developers. If you need ordered loads then use a
>> loadload() barrier. If the loads need to be ordered you need to ensure there
>> are not corresponding writes that also need to be ordered - which may show
>> where release() is missing.
>>
>>> Having said that, I'm strongly of the opinion that if you see a naked
>>> StoreStore it may well be a bug, or at least you've got something very
>>> hard to analyse. I know of a few cases (e.g. zeroing an object) where
>>> this isn't true.
>>>
>>> https://www.hboehm.info/c++mm/no_write_fences.html, etc.
>>>
>>> But that's an argument for another day.
>>
>> Indeed :)
>>
>> Cheers,
>> David
>