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

From: Waiman Long <longman@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>,
	Zhenzhong Duan <zhenzhong.duan@oracle.com>
Cc: linux-kernel@vger.kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Srinivas Eeda <srinivas.eeda@oracle.com>,
	x86@kernel.org
Subject: Re: [PATCH 2/2] Revert "x86/hpet: Reduce HPET counter read contention"
Date: Fri, 15 Mar 2019 10:17:23 -0400	[thread overview]
Message-ID: <09d5ca8b-9cee-c013-01b4-bd2e78672b57@redhat.com> (raw)
In-Reply-To: <20190315092529.GU5996@hirez.programming.kicks-ass.net>

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.

Cheers,
Longman