RFR: 8268228: TSC is not used for CPUTimeStampCounter on AMD processor [v2]

[David Holmes]

On 8/06/2021 3:09 pm, Yasumasa Suenaga wrote:
> On Fri, 4 Jun 2021 05:24:15 GMT, Yasumasa Suenaga <ysuenaga at openjdk.org> wrote:
> 
>>> I ran JVM on Ryzen 3300X, and I got following `jdk.CPUTimeStampCounter` event.
>>>
>>>
>>> jdk.CPUTimeStampCounter {
>>>    startTime = 10:41:14.993
>>>    fastTimeEnabled = false
>>>    fastTimeAutoEnabled = true
>>>    osFrequency = 1000000000
>>>    fastTimeFrequency = 1000000000
>>> }
>>>
>>>
>>> I confirmed 3300X supports Invariant TSC (so `fastTimeAutoEnabled` is set to `true`), however it does not seem to be used (`fastTimeEnabled` is `false`).
>>>
>>> Frequency is come from brand string from CPUID (e.g. "Intel(R) Core(TM) i3-8145U CPU @ 2.10GHz"). However AMD processor (Ryzen at least) does not have it ("AMD Ryzen 3 3300X 4-Core Processor").
>>> Fortunately rdtsc_x86.cpp can calculate the frequency like bogomips. We should fallback to it if we cannot get the frequency even if invariant TSC is supported.
>>>
>>> After this change, I got following `jdk.CPUTimeStampCounter` event. Base clock of Ryzen 3 3300X is 3.8GHz, so `fastTimeFrequency` looks good.
>>>
>>>
>>> jdk.CPUTimeStampCounter {
>>>    startTime = 10:33:52.884
>>>    fastTimeEnabled = true
>>>    fastTimeAutoEnabled = true
>>>    osFrequency = 10000000 Hz
>>>    fastTimeFrequency = 3792929124 Hz
>>> }
>>>
>>>
>>> This problem is not only for JFR. I confirmed `Rdtsc` class is used in ticks.cpp , and it relates to GC code at least.
>>
>> Yasumasa Suenaga has updated the pull request incrementally with one additional commit since the last revision:
>>
>>    Fix comments
> 
> To be honest I like TSC for calculating short time difference because execution cost is much lesser than syscalls. So I want to use it on AMD processor especially with JFR - it requires event duration which is calculated by TSC or by timestamp. I think there is not so big problem for this purpose.
> 
> However, in general, if TSC should not be used in HotSpot, I will withdraw this PR and will close JBS.

The problem here is twofold:

1. Your change potentially affects thousands of users with AMD systems 
that don't currently use the TSC, in ways we can't be sure of.

2. As Kim points out, you may be the first to actually use the bogomips 
code with this change, which is even more scarey!

So my recommendation is to withdraw the PR.

Thanks,
David

> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/4350
>