From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755759AbdKODxV (ORCPT ); Tue, 14 Nov 2017 22:53:21 -0500 Received: from mail.cn.fujitsu.com ([183.91.158.132]:53654 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752686AbdKODxM (ORCPT ); Tue, 14 Nov 2017 22:53:12 -0500 X-IronPort-AV: E=Sophos;i="5.43,368,1503331200"; d="scan'208";a="30187562" Subject: Re: [PATCH v8 6/6] x86/tsc: use tsc early To: Pavel Tatashin References: <20171109030201.5991-1-pasha.tatashin@oracle.com> <20171109030201.5991-7-pasha.tatashin@oracle.com> <7a799891-8df3-ff53-b2b8-dc06b64fea47@cn.fujitsu.com> CC: Steve Sistare , Daniel Jordan , , , , John Stultz , , , , , Thomas Gleixner , From: Dou Liyang Message-ID: <6813baaf-909e-e32e-b074-6c466ceabcc6@cn.fujitsu.com> Date: Wed, 15 Nov 2017 11:53:07 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.167.226.106] X-yoursite-MailScanner-ID: 5CAD141860D5.ADC6B X-yoursite-MailScanner: Found to be clean X-yoursite-MailScanner-From: douly.fnst@cn.fujitsu.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Pavel, At 11/15/2017 05:46 AM, Pavel Tatashin wrote: > Hi Dou, > > Great comments, my replies below: > >>> static inline unsigned long long paravirt_sched_clock(void) >>> { >>> - return PVOP_CALL0(unsigned long long, pv_time_ops.sched_clock); >>> + return PVOP_CALL0(unsigned long long, >>> pv_time_ops.active_sched_clock); >>> } >>> >> >> Should in the 5th patch > > Actually, it has to be in patch 6, because otherwise patch 5 without > patch 6 would cause native_sched_clock() to be used even when a > platform specific clock is set, thus may cause performance regressions > where it is not optimal to use tsc for clock. > Indeed. >> + tsc_early_disable(); >> + __sched_clock_offset = sched_clock_early() - sched_clock(); >> + pr_info("sched clock early is finished, offset[%lld.%09lds]\n", t, r); >> +} s/sched clock early/early sched clock/ >> +#endif /* CONFIG_X86_TSC */ >> Add definitions for the situation of X86_TSC = no : >> >> #else /* CONFIG_X86_TSC */ >> static inline void tsc_early_init(unsigned int khz) { } >> static inline void tsc_early_fini(void) { } > > Excellent point, I totally forgot about X86_TSC = no, however, a > better fix is to simply remove #ifdef CONFIG_X86_TSC from my > functions. Apparently, even with X86_TSC=no we can use TSC unless Agree with removing #ifdef CONFIG_X86_TSC > notsc kernel parameter is passed. This will be in the next patchset. > >> >> According to tsc_early_delay_calibrate(), if (!boot_cpu_has(X86_FEATURE_TSC >> || !tsc_khz ), tsc_early_init(tsc_khz) >> will never be called, so here is redundant. >> >>> return; >>> } >>> >>> @@ -1302,6 +1385,7 @@ void __init tsc_init(void) >>> if (!tsc_khz) { >>> mark_tsc_unstable("could not calculate TSC khz"); >>> setup_clear_cpu_cap(X86_FEATURE_TSC_DEADLINE_TIMER); >>> + tsc_early_fini(); >> >> >> ditto > > Right, in both case we still want to call tsc_early_fini(). Because, > it calls tsc_early_disable() even when tsc_early_init() was never > called. tsc_early_disable() either sets static branch > __tsc_early_static to false or changes active_sched_clock to be > platform specific, depending on CONFIG_PARAVIRT. Yes, the function names confused me, the actually purpose of tsc_early_fini() is switching schedule clock to the final one. right? How about replacing *return;* with *goto final_sched_clock;* Thanks, dou. > >> BTW, seems you forgot to cc Peter Zijlstra in both V7 >> and V8 patchsets. > > Thank you for noticing this! I will include Peter when I send out > patchset version 9. > > Thank you, > Pavel > > >