From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A0FBEC43144 for ; Sun, 24 Jun 2018 07:31:44 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5681A24E11 for ; Sun, 24 Jun 2018 07:31:44 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5681A24E11 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=linutronix.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751950AbeFXHbm (ORCPT ); Sun, 24 Jun 2018 03:31:42 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:44744 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750737AbeFXHbj (ORCPT ); Sun, 24 Jun 2018 03:31:39 -0400 Received: from p4fea482e.dip0.t-ipconnect.de ([79.234.72.46] helo=nanos) by Galois.linutronix.de with esmtpsa (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1fWzUA-0002lR-RS; Sun, 24 Jun 2018 09:30:59 +0200 Date: Sun, 24 Jun 2018 09:30:58 +0200 (CEST) From: Thomas Gleixner To: Pavel Tatashin cc: Steven Sistare , Daniel Jordan , linux@armlinux.org.uk, schwidefsky@de.ibm.com, Heiko Carstens , John Stultz , sboyd@codeaurora.org, x86@kernel.org, LKML , mingo@redhat.com, hpa@zytor.com, douly.fnst@cn.fujitsu.com, peterz@infradead.org, prarit@redhat.com, feng.tang@intel.com, Petr Mladek , gnomes@lxorguk.ukuu.org.uk, linux-s390@vger.kernel.org Subject: Re: [PATCH v12 09/11] x86/tsc: prepare for early sched_clock In-Reply-To: Message-ID: References: <20180621212518.19914-1-pasha.tatashin@oracle.com> <20180621212518.19914-10-pasha.tatashin@oracle.com> User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, 23 Jun 2018, Pavel Tatashin wrote: > > > As soon as sched_clock() starts output non-zero values, we start > > > output time without correcting the output as it is done in > > > sched_clock_local() where unstable TSC and backward motion are > > > detected. But, since early in boot interrupts are disabled, we cannot > > > really correct invalid time, and therefore must rely on sched_clock() > > > to provide us with a contiguous and sane time. > > > > In early boot the TSC frequency is not changing at all so the whole > > unstable thing is completely irrelevant. And that also applies to backward > > motion. There is no such thing during early boot. > > Yes, however, the frequency changes slightly during re-calibration. I > see it every time in my no-kvm qemu VM, and I think even on physical > machine. Now, as you suggest below, I can remove the second > calibration entirely, and keep only the early calibration, I am not > sure what the historical reason to why linux has two of them in the git log/blame exist for a reason and in case that does not work asking around might give answers as well. > first place. But, I assumed the later one was more precise because of > some outside reasons, such as different cpu p-state, or something > else. cpu p->states are exactly the same. This is still early boot. Nothing has fiddled with any of this. > > > In earlier versions of this project, I was solving this problem by > > > adjusting __sched_clock_offset every time sched_clock()'s continuity was > > > changed by using a callback function into sched/clock.c. But, Peter was > > > against that approach. > > > > So your changelog is completely misleading and utterly wrong. What the heck > > has this to do with jiffies and the use_tsc jump label? Exactly nothing. > > This output is what happens after: "sched: early sched_clock" but > before"x86/tsc: use tsc early", hence the title of patch: "x86/tsc: > prepare for early sched_clock" > So, before "x86/tsc: prepare for early sched_clock" we go from > jiffies to real tsc, and thats where the problem happens. I assume > theoretically, the same problem could happen if we can't calibrate TSC > early, but succeed later. Now, this is, however, a moot point, as you > suggest to remove the second calibration. Please stop assuming things. Figure the root cause out, really. > After thinking about this some more, in the future revision I will > simply switch order for "x86/tsc: use tsc early" to go before "sched: > early sched_clock", since transitions jiffies -> tsc and tsc -> > jiffies won't be possible with the changes you suggest below. > > > > > > [ 0.004000] ..TIMER: vector=0x30 apic1=0 pin1=2 apic2=-1 pin2=-1 > > > [ 0.009000] tsc: Fast TSC calibration using PIT > > > [ 0.010000] tsc: Detected 3192.137 MHz processor > > > [ 0.011000] clocksource: tsc-early: mask: 0xffffffffffffffff max_cycles: 0x2e03465ceb2, max_idle_ns: 440795259855 ns > > > > > static_branch_enable(__use_tsc) is called, and timestamps became precise > > > but reduced: > > > > > [ 0.002233] Calibrating delay loop (skipped), value calculated using timer frequency.. 6384.27 BogoMIPS (lpj=3192137) > > > > This is crap, because this is not what the current implementation does. The > > current implementation enables the static branch when the early TSC sched > > clock is enabled. So even the timestamps are crap because with the early > > sched clock the time stamps are precise _before_ the timer interrupt is > > initialized. That's the whole purpose of the exercise, right? > > > > This made me assume that its an existing problem. Heck, changelogs are > > about facts and not fairy tales. And it's completely non interesting that > > you observed this problem with some random variant of your patches. What's > > interesting is the factual problem which makes the change necessary. > > The changelog was about a fact, as stated above: output is from what > happens after "sched: early sched_clock" but before "x86/tsc: use tsc > early", (i.e. "x86/tsc: use tsc early" might be reverted or > bisected). I can see the intention now, but this is exactly why precise wording and problem description and root cause analysis matter. It just confused me completely, > For this particular patch, I politely asked for suggestions in cover letter: > > v10-v11 > - I added one more patch: > "x86/tsc: prepare for early sched_clock" which fixes a problem > that I discovered while testing. I am not particularly happy with > the fix, as it adds a new argument that is used only in one > place, but if you have a suggestion for a different approach on > how to address this problem please let me know. Fair enough. I missed that. It would have been more obvious to mark the patch RFC and add exactly this into the changelog. Thanks, tglx