From owner-freebsd-current@freebsd.org Thu Feb 23 17:04:00 2017 Return-Path: Delivered-To: freebsd-current@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 4071ACEB9B8 for ; Thu, 23 Feb 2017 17:04:00 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from mail.baldwin.cx (bigwig.baldwin.cx [96.47.65.170]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id F17D09EB; Thu, 23 Feb 2017 17:03:59 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from ralph.baldwin.cx (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by mail.baldwin.cx (Postfix) with ESMTPSA id 6DFA910A7B9; Thu, 23 Feb 2017 12:03:57 -0500 (EST) From: John Baldwin To: Jia-Shiun Li Cc: Konstantin Belousov , freebsd-current , Konstantin Belousov Subject: Re: TSC as timecounter makes system lag Date: Thu, 23 Feb 2017 08:55:42 -0800 Message-ID: <2204246.QKzIRnxiUQ@ralph.baldwin.cx> User-Agent: KMail/4.14.10 (FreeBSD/11.0-STABLE; KDE/4.14.10; amd64; ; ) In-Reply-To: References: <20170113120534.GC2349@kib.kiev.ua> <20170223100829.GR2092@kib.kiev.ua> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.4.3 (mail.baldwin.cx); Thu, 23 Feb 2017 12:03:57 -0500 (EST) X-Virus-Scanned: clamav-milter 0.99.2 at mail.baldwin.cx X-Virus-Status: Clean X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 23 Feb 2017 17:04:00 -0000 On Thursday, February 23, 2017 11:04:58 PM Jia-Shiun Li wrote: > On Thu, Feb 23, 2017 at 6:08 PM, Konstantin Belousov > wrote: > > > > > This is a useful analysis. > > > > Yes, I think that there is an init ordering issue. Note that > > cpu_disable_c2_sleep is only changed in tc_windup() when timecounter > > is changed. If existing and already engadged timecounter suddenly gets > > TC_FLAG_C2STOP set, tc_windup() ignores the flag. And with the early > > AP startup, tsc seems to be set as timecounter too early. > > > > Just moving order of init_TSC_tc() would not help, since tsc checks smp > > consistency, which requires started APs. Try this for now, but might > > be John has better idea how to handle the issue. You might need to add > > some extern declarations for the patch to compile. > > > > diff --git a/sys/x86/x86/tsc.c b/sys/x86/x86/tsc.c > > index 3f36fbd9f8a..f8e33069c70 100644 > > --- a/sys/x86/x86/tsc.c > > +++ b/sys/x86/x86/tsc.c > > @@ -545,6 +545,8 @@ init_TSC_tc(void) > > if (cpu_deepest_sleep >= 2 && cpu_vendor_id == CPU_VENDOR_INTEL && > > (amd_pminfo & AMDPM_TSC_INVARIANT) == 0) { > > tsc_timecounter.tc_flags |= TC_FLAGS_C2STOP; > > + if (timecounter == &tsc_timecounter) > > + cpu_disable_c2_sleep++; > > if (bootverbose) > > printf("TSC timecounter disables C2 and C3.\n"); > > } > > > > > This does not work. > > I added a printf before the outer if clause, and it says > > init_TSC_tc:546: deepest 00000000 vendor 00008086 amd_pminfo 00000000 > > full boot dmesg attached. Looks init_TSC_tc() is called too early before > acpi_cpu_attach() initializing cpu_deepest_sleep. Maybe it should be put > after > driver initialization, since it depends on probed ACPI C states? We don't actually need cpu_deepest_sleep. We could just set C2STOP always. It doesn't hurt to have the flag set if the system only supports C1 except that you get the printf in a verbose boot. Try this slight variation of Konstantin's patch. If this works we can remove cpu_deepest_sleep entirely as a followup since it will no longer be used: Index: tsc.c =================================================================== --- tsc.c (revision 314113) +++ tsc.c (working copy) @@ -542,9 +542,11 @@ init_TSC_tc(void) * result incorrect runtimes for kernel idle threads (but not * for any non-idle threads). */ - if (cpu_deepest_sleep >= 2 && cpu_vendor_id == CPU_VENDOR_INTEL && + if (cpu_vendor_id == CPU_VENDOR_INTEL && (amd_pminfo & AMDPM_TSC_INVARIANT) == 0) { tsc_timecounter.tc_flags |= TC_FLAGS_C2STOP; + if (timecounter == &tsc_timecounter) + cpu_disable_c2_sleep++; if (bootverbose) printf("TSC timecounter disables C2 and C3.\n"); } -- John Baldwin