[aarch64-port-dev ] RFR 8135018: AARCH64: Missing memory barriers for CMS collector

[Liu Xin]

"store store barriers"
http://gee.cs.oswego.edu/dl/jmm/cookbook.html

I think the JDK-8135018 inserts a membar(StoreStore) in the middle of
put_field and update_cardTable on purpose. it can guarantee memory orders
seen by Concurrent Mark Threads.
if you put the storestore membar after two stores, IMHO, the hazard is
still there.


On Mon, Dec 16, 2019 at 3:34 AM Liu Xin <navy.xliu at gmail.com> wrote:

> hi, Felix,
>
> The original storestore appears right before strb.  your
> membar(storeStore) is after strb.  is it the same?
> if it's same, we can place membar out of if/else to dedup.
>
>
> diff --git a/src/hotspot/src/share/vm/c1/c1_LIRGenerator.cpp
> b/src/hotspot/src/share/vm/c1/c1_LIRGenerator.cpp
> index 19ec64d8..068f0797 100644
> --- a/src/hotspot/src/share/vm/c1/c1_LIRGenerator.cpp
> +++ b/src/hotspot/src/share/vm/c1/c1_LIRGenerator.cpp
> @@ -1615,6 +1615,9 @@ void
> LIRGenerator::CardTableModRef_post_barrier(LIR_OprDesc* addr, LIR_OprDesc*
>                new LIR_Address(tmp, load_constant(card_table_base),
>                                T_BYTE));
>    }
> +  if (UseConcMarkSweepGC && CMSPrecleaningEnabled) {
> +      __ membar_storestore();
> +  }
>  #endif
>  }
>
> On Mon, Dec 16, 2019 at 3:21 AM Yangfei (Felix) <felix.yang at huawei.com>
> wrote:
>
>> Hi Liu Xin,
>>
>>   Based on what Andrew said about UseCondCardMark, I agree with Martin
>> that we go with your new backport webrev for 8135018 first.
>>   My only concern is that we changed to store
>> CardTableModRefBS::dirty_card_val() instead of constant zero to card_addr
>> in c1_LIRGenerator.cpp.
>>   It looks to me unnecessary to add this change when you are adding the
>> missing memory bars.
>>   So I suggest we do:
>>
>>   diff -r 09d4b646f756 src/share/vm/c1/c1_LIRGenerator.cpp
>> --- a/src/share/vm/c1/c1_LIRGenerator.cpp       Tue Nov 12 17:54:52 2019
>> +0800
>> +++ b/src/share/vm/c1/c1_LIRGenerator.cpp       Mon Dec 16 19:17:51 2019
>> +0800
>> @@ -1622,10 +1622,16 @@
>>    if (can_inline_as_constant(card_table_base)) {
>>      __ move(LIR_OprFact::intConst(0),
>>                new LIR_Address(tmp, card_table_base->as_jint(), T_BYTE));
>> +    if (UseConcMarkSweepGC && CMSPrecleaningEnabled) {
>> +      __ membar_storestore();
>> +    }
>>    } else {
>>      __ move(LIR_OprFact::intConst(0),
>>                new LIR_Address(tmp, load_constant(card_table_base),
>>                                T_BYTE));
>> +    if (UseConcMarkSweepGC && CMSPrecleaningEnabled) {
>> +      __ membar_storestore();
>> +    }
>>    }
>>  #endif
>>  }
>>
>>
>> Thanks,
>> Felix
>>
>> >
>> > From: Liu Xin <navy.xliu at gmail.com>
>> > Sent: Montag, 16. Dezember 2019 10:48
>> > To: Andrew Haley <aph at redhat.com>
>> > Cc: Doerr, Martin <martin.doerr at sap.com>;
>> > aarch64-port-dev at openjdk.java.net; Hohensee, Paul
>> > <hohensee at amazon.com>
>> > Subject: Re: RFR 8135018: AARCH64: Missing memory barriers for CMS
>> > collector
>> >
>> > Hi, Andrew and Felix,
>> >
>> > I did backport as Felix suggested. To get UseCondCardMark, we need to
>> > backport other 3 patches.
>> >
>> > hg qseries as follows ( applied in order)
>> > 8076987: https://cr.openjdk.java.net/~xliu/8076987/00/webrev/
>> > 8078438: https://cr.openjdk.java.net/~xliu/8078438/00/webrev/
>> > 8079315: https://cr.openjdk.java.net/~xliu/8079315/00/webrev/
>> > 8135018: https://cr.openjdk.java.net/~xliu/8135018/02/webrev/
>> >
>> > As Andrew pointed out, I also notice that it introduces a storeLoad
>> membar for
>> > interpreter if UseCondCardMark is on.
>> > The reason I didn't backport UseCondCardMark at first place because
>> it's off by
>> > default. I need to run all tests again with the new parameter
>> > -XX:+UseCondCardMark to validate it.  Another reason is that all
>> performance
>> > evaluation was done on x86_64.
>> > It's really time consuming to evaluate performance impact on aarch64.
>> So I
>> > shortcut to fix my primary bug.
>> >
>> > Could you sponsor my previous webrev?
>> > https://cr.openjdk.java.net/~xliu/8135018/01/webrev/
>> >
>> > It does have a conflict with c1_LIRGenerator.cpp. How about I send
>> another
>> > webrev to jdk8u and update it as well?
>> >
>> > thanks,
>> > --lx
>> >
>> >
>> > On Mon, Dec 16, 2019 at 1:00 AM Andrew Haley
>> > <aph at redhat.com<mailto:aph at redhat.com>> wrote:
>> > On 12/14/19 4:47 AM, Liu Xin wrote:
>> > > I trace the feature UseCondCardMark. It was introduced by the
>> > > following changes.
>> > > https://bugs.openjdk.java.net/browse/JDK-8076987 C1
>> > > https://bugs.openjdk.java.net/browse/JDK-8078438 interpreter
>> > > UseCondCardMark is only supported by C2 currently.
>> >
>> > UseCondCardMark was supposed to be a performance enhancement for very
>> > large multi-core systems, but we realized that it needed a StoreLoad
>> barrier.
>> > Because StoreLoad is so expensive there is no performance advantage,
>> > therefore as far as I can see the whole UseCondCardMark exercise is a
>> waste
>> > of time. There is no point back-porting changes to the interpreter and
>> C1.
>> >
>> > Unless someone does some performance measurements to prove that there is
>> > some performance gain in C1 and the interpreter, UseCondCardMark changes
>> > should not be back-ported.
>> >
>> > --
>> > Andrew Haley  (he/him)
>> > Java Platform Lead Engineer
>> > Red Hat UK Ltd. <https://www.redhat.com> https://keybase.io/andrewhaley
>> > EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
>>
>