RFR (XS): 8188877: Improper synchronization in offer_termination

[Kim Barrett]

> On Oct 6, 2017, at 7:13 PM, White, Derek <Derek.White at cavium.com> wrote:
> 
> Hi All,
>  
> The field ParallelTaskTerminator::_offered_termination is modified atomically, but is read as a normal field access.
>  
> In ParallelTaskTerminator::offer_termination(), this field is read within a loop, but there's nothing stopping a compiler from hoisting the load out of the loop. See CR for examples of possible load hoisting.
>  
> Trivial fix: The _offered_termination field should be declared volatile.
>  
> This was found by code inspection. I haven't seen an actual error, but compiler optimizations or unrelated code changes could make an error occur in the future.
>  
> CR: https://bugs.openjdk.java.net/browse/JDK-8188877
> Webrev: http://cr.openjdk.java.net/~drwhite/8188877/webrev.01/
>  
> Minor build and testing done, but inspected the generated code for offer_termination() and found no differences.
>  
> 	• Derek

------------------------------------------------------------------------------ 
src/hotspot/share/gc/shared/taskqueue.cpp 

The change to make _offered_termination volatile prevents the compiler
from hoisting it out of the loop, but I'm not sure it is enough to
prevent unnecessary delays in recognizing the termination condition.
In particular, while offer_termination is spinning, it's not clear
there's anything to ensure an update by some other thread will become
visible during the repeated spin cycles.  yield and sleep seem likely
to involve fences that will make such updates visible, but not so much
for SpinPause.

I also noticed the SpinPause implementations for linux_arm(64) and
linux_aarch64 are different, with the latter perhaps being lacking.

------------------------------------------------------------------------------  

In addition, some pre-existing issues:

------------------------------------------------------------------------------   
src/hotspot/share/gc/shared/taskqueue.cpp 

[pre-existing]
The casts in ParallelTaskTerminator::offer_termination's
Atomic::inc/dec of _offered_termination are no longer needed.

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/taskqueue.cpp 

[pre-existing]
In ParallelTaskTerminator::offer_termination, a reference to
WorkStealingHardSpins is cast to uint.  The only reason for that cast
is that the variable is declared uintx, when it really ought to be
uint.  Probably all three of the WorkStealingMUMBLE experimental
parameters ought to be of type uint rather than uintx.

------------------------------------------------------------------------------ 
src/hotspot/share/gc/shared/taskqueue.cpp 

[pre-existing]
ParallelTaskTerminator::offer_termination computes the hard_spin_limit
as
  hard_spin_limit = WorkStealingHardSpins >> WorkStealingSpinToYieldRatio;
  hard_spin_limit = MAX2(hard_spin_limit, 1U);

The use of a left shift will result in undefined behavior for
seemingly reasonable non-default choices for the ratio parameter,
e.g. any value >= number of bits in type of WorkStealingHardSpins.

Perhaps this should be addressed by limiting the permitted range for
the ratio parameter.

------------------------------------------------------------------------------