Re: [PATCH 2/2] Revert "x86/hpet: Reduce HPET counter read contention"

[Waiman Long]

On 03/18/2019 04:44 AM, Zhenzhong Duan wrote:
>
> On 2019/3/15 22:17, Waiman Long wrote:
>> On 03/15/2019 05:25 AM, Peter Zijlstra wrote:
>>> On Thu, Mar 14, 2019 at 04:42:12PM +0800, Zhenzhong Duan wrote:
>>>> This reverts commit f99fd22e4d4bc84880a8a3117311bbf0e3a6a9dc.
>>>>
>>>> It's unnecessory after commit "acpi_pm: Fix bootup softlockup due
>>>> to PMTMR
>>>> counter read contention", the simple HPET access code could be
>>>> restored.
>>>>
>>>> On a general system with good TSC, TSC is the final default
>>>> clocksource.
>>>> So the potential performce loss is only at bootup stage before TSC
>>>> replacing HPET, we didn't observe obvious delay of bootup.
>>> The timeline here is:
>>>
>>> Â - Len took out SKX from native_calibrate_tsc
>>> ÂÂÂ b51120309348 ("x86/tsc: Fix erroneous TSC rate on Skylake Xeon")
>>>
>>> ÂÂÂ This causes the TSC to run through the calibration code, which
>>> ÂÂÂ completes _after_ SMP bringup.
>>>
>>> Â - This then caused HPET to be used during SMP bringup, which resulted
>>> ÂÂÂ in Waiman doing the patch you now propose removing.
>>>
>>> ÂÂÂ Because large (multi-socket) SKX machines would barely boot.
>>>
>>> ÂÂÂ f99fd22e4d4b ("x86/hpet: Reduce HPET counter read contention")
>>>
>>> Â - Now, I figured that was all crazy to begin with, and introduced
>>> ÂÂÂ clocksource_tsc_early, such that we can run at the guestimate TSC
>>> ÂÂÂ frequency until we've completed calibration and then swap to the
>>> real
>>> ÂÂÂ TSC clocksource.
>>>
>>> ÂÂÂ aa83c45762a2 ("x86/tsc: Introduce early tsc clocksource")
>>> ÂÂÂ (and assorted fixes)
>>>
>>> This means that we now only use HPET for a very short time in early
>>> boot, _IFF_ TSC is stable.
>>>
>>> Now, given the amount of wreckage we still see with TSC, I'm very
>>> reluctant to revert this patch. Because the moment TSC goes out the
>>> window, we're back on HPET, and this patch does make a huge difference.
>>>
>>> Yes, its sad, gross and nasty... but the same is true for TSC still
>>> being
>>> a trainwreck.
>>>
>>> So NAK.
>> I concur. In the uncontended case, the overhead is mostly just the
>> additional cmpxchg instruction for acquiring the spinlock. Even then, it
>> isn't significant when compared with the time needed to actually read
>> from the HPET. Without that code, any fallback to HPET for whatever
>> reason will likely see degradation in performance especially on systems
>> with large number of CPUs.
>
> Thank Peter and Waiman for reply.
>
> I see, we still care the performance on a system with wreckage TSC.
>
>
> So now we come back to the old question, do we care the softlockup
>
> and the performance when pmtmr is chosed for whatever reason?
>
> For which I had provide two different fixes:
>
> https://lkml.org/lkml/2019/1/22/1172
>
> and
>
> https://lkml.org/lkml/2019/3/15/101
>
I think what Thomas was asking is to provide a REALISTIC use case where
TSC is wrecked and HPET is somehow not used and we have to fall back to
use PM timer. If such use case exists, I am sure Thomas will be happy to
take it.

Cheers,
Longman