RFR: 8229020: Failure on CPUs allowing loads reordering: assert(_tasks[t] == 1) failed: What else?
Jie Fu
fujie at loongson.cn
Sat Aug 3 01:27:05 UTC 2019
Hi Thomas and Kim,
Thanks for your review and nice suggestion.
Updated: http://cr.openjdk.java.net/~jiefu/8229020/webrev.02/
I had removed the assert and added reviewers in the patch.
I need a sponsor. Could someone help to push it?
Thanks a lot.
Best regards,
Jie
On 2019/8/3 上午4:30, Kim Barrett wrote:
>> On Aug 2, 2019, at 11:39 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>>
>> Hi Jie,
>>
>> On 02.08.19 02:44, Jie Fu wrote:
>>> Hi all,
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8229020
>>> Webrev: http://cr.openjdk.java.net/~jiefu/8229020/webrev.00/
>>> *Background*
>>> The failure was first observed on our Loongson CPUs which allow loads reordering with the following test
>>> ---------------------------------------------------------
>>> make test TEST="compiler/codecache/stress/UnexpectedDeoptimizationTest.java" CONF=fastdebug
>>> ---------------------------------------------------------
>>> *Analysis*
>>> The failure was caused by the loads reordering on CPUs with weak memory consistency.
>>> Just imagine the following case:
>>> - If the load of _tasks[t] in line 436 is floating up before the load of _tasks[t] in line 432, and
>>> - the load in line 436 may read 0 and the load in line 432 may read 1,
>>> - then the if-condition in line 433 is false, so Atomic::cmpxchg won't be executed,
>>> - then the assert in line 436 fails.
>>> ---------------------------------------------------------
>>> 429
>>> 430 bool SubTasksDone::try_claim_task(uint t) {
>>> 431 assert(t < _n_tasks, "bad task id.");
>>> 432 uint old = _tasks[t];
>>> 433 if (old == 0) {
>>> 434 old = Atomic::cmpxchg(1u, &_tasks[t], 0u);
>>> 435 }
>>> 436 assert(_tasks[t] == 1, "What else?");
>>> 437 bool res = old == 0;
>>> 438 #ifdef ASSERT
>>> 439 if (res) {
>>> 440 assert(_claimed < _n_tasks, "Too many tasks claimed; missing clear?");
>>> 441 Atomic::inc(&_claimed);
>>> 442 }
>>> 443 #endif
>>> 444 return res;
>>> 445 }
>>> 446
>>> ---------------------------------------------------------
>>> *Fix*
>>> It would be better to insert a memory fence before line 436 to prevent the load from floating up.
>>> Could you please review it?
>> remove the assert. It's old paranoid code adding no further information imho.
>>
>> Thanks,
>> Thomas
> I agree with Thomas; remove the assert. I'm slightly surprised this
> hasn't come up before with other weakly ordered platforms.
>
More information about the hotspot-gc-dev
mailing list