RFR: 8229020: Failure on CPUs allowing loads reordering: assert(_tasks[t] == 1) failed: What else?

[Kim Barrett]

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