RFR(s): 8212108: SafepointSynchronizer never ending counter (big enough)

[David Holmes]

Hi Robbin,

On 22/11/2018 6:50 pm, Robbin Ehn wrote:
> Hi David,
> 
> On 11/22/18 3:37 AM, David Holmes wrote:
>> Hi Robbin,
>>
>> On 21/11/2018 11:45 pm, Robbin Ehn wrote:
>>> Hi all, please review.
>>>
>>> The counter can loop around, it would mainly be an issue in reading
>>> such flight- recorder recording. I have two upcoming features which
>>> uses the fact that it is always increasing >
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8212108
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~rehn/8212108/webrev/
>>
>> I don't understand the logic for safepoint_counter_addr(). Why is 
>> endian-ness an issue here and why do you need to treat it as 32-bit in 
>> that case??
> 
> Because it is suppose to return the address to 32-bit counter.
> Returning the address to the 4 low bytes save me for updating a lot
> of assembly code in jniFastGetField.

Ah I see. That's a bit messy. :(

>>
>> I've been trying to see if there could be any issues with word-tearing 
>> on 32-bit systems ... I can't convince myself you may not see false 
>> indicators if the count is read at the same time as being updated and 
>> the 32-bit count is going to rollover from 0xFFFFFFFF to 0x00000000.
> 
> The jniFastGetField should only checks if it's different.
> It does:
> load counter
> if counter is odd (safepoint) => slowpath
> speculative load value (which handles e.g. SIGSEGV/SIGBUS)
> load counter
> if counter != counter => slowpath
> /* value is good */

And this code only looks at the lower 32-bits so no tearing is possible 
there.

Okay.

Thanks,
David

>>
>> I'm not familiar with JFR types - is "ulong" guaranteed to be 64-bit?
> 
> It translate to an u8, so yes.
> 
>>
>>> Passes t1-3 and the two updated FR tests.
>>
>> Does any of that actually run long enough to overflow a 32-bit counter?
> 
> We have no tests that run long enough to rollover.
> 
> Thanks, Robbin
> 
>>
>> Thanks,
>> David
>>
>>>
>>> Thanks Robbin