From patchwork Thu Apr 13 13:23:49 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peter Zijlstra X-Patchwork-Id: 9679385 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 2F8CC60326 for ; Thu, 13 Apr 2017 13:23:57 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 209182862B for ; Thu, 13 Apr 2017 13:23:57 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 1359C28647; Thu, 13 Apr 2017 13:23:57 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 676922862B for ; Thu, 13 Apr 2017 13:23:56 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 9AC0A6E864; Thu, 13 Apr 2017 13:23:55 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) by gabe.freedesktop.org (Postfix) with ESMTPS id D33166E864 for ; Thu, 13 Apr 2017 13:23:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20170209; h=In-Reply-To:Content-Type:MIME-Version :References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=tiM/rbnoFxMTIwFkS+lghrNtJwyyqsaRzbGHJ6p3sog=; b=fy0lsLOQRUKGG/WXVxBSG3bP4 8VluHGCck16NJY+lSf3MOqhoi60du6e6UsTD2sjITd/+Pbmbg//uRiHGG/CZbWLod/oFsZoJQXUgx 22ULhIK897JBbdC38/4uX4jCWoSV/OKDy8Uj7ZRWsw44vKCHXNnFrkQugxjlWsXGgKkuNc8KDj+vh SACLvREle7nVQzwHPcqGIs5YYQcqs4KaV0AQNhB4WjggodWJksLCmdqfb4NTUqhyaKOCWFXOtHgIp LigXowC7QWTs3Ny//KFkoGIoBW3ZTg4Qyjgr69JQgJXETzMT2tRFCa5ieUg7R3w06G7lNVg2NBk56 nUUJyMOaA==; Received: from j217100.upc-j.chello.nl ([24.132.217.100] helo=hirez.programming.kicks-ass.net) by bombadil.infradead.org with esmtpsa (Exim 4.87 #1 (Red Hat Linux)) id 1cyej3-0005Rw-2z; Thu, 13 Apr 2017 13:23:53 +0000 Received: by hirez.programming.kicks-ass.net (Postfix, from userid 1000) id B922E202682C2; Thu, 13 Apr 2017 15:23:49 +0200 (CEST) Date: Thu, 13 Apr 2017 15:23:49 +0200 From: Peter Zijlstra To: Martin Peres Message-ID: <20170413132349.thxkwptdymsfsyxb@hirez.programming.kicks-ass.net> References: <20170412123134.GD5680@worktop> <9fc50a4b-da78-5e18-8fed-3bf91a7091a7@linux.intel.com> <20170412143222.5523ltuiz5kdmdl7@hirez.programming.kicks-ass.net> <8b94e683-3d09-d5dc-cfd1-746f179f3678@linux.intel.com> <20170413114803.poziwglvrzbmtmvb@hirez.programming.kicks-ass.net> <173f3091-c9cd-bc00-bb5b-b4486933102d@linux.intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <173f3091-c9cd-bc00-bb5b-b4486933102d@linux.intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Cc: "intel-gfx@lists.freedesktop.org" , Thomas Gleixner , "pasha.tatashin@oracle.com" Subject: Re: [Intel-gfx] freedesktop bug id: 100548, bisected to sched/clock commit X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Apr 13, 2017 at 03:30:25PM +0300, Martin Peres wrote: > On 13/04/17 14:48, Peter Zijlstra wrote: > > On Wed, Apr 12, 2017 at 05:49:53PM +0300, Martin Peres wrote: > > > > > Good to know. Is there a way to disable this behaviour, as a workaround for > > > our CI system until a proper fix lands? We already pushed locally the revert > > > for this patch, but that may affect other platforms which do not exhibit the > > > problem. > > > > Blergh, so the patch is correct, but the __gtod_offset calculation is > > fed with absolute crap numbers due to 'circumstances' and then using it > > ends up being worse than not using it. > > Thanks for taking this bug seriously! So I've not actually dug out a Core2 machine, so have only tested this by poking random values into the TSC MSR on an otherwise 'good' machine. Could you give it a go to see if it works for you? Thomas, how much hate? --- Subject: sched/clock,x86/tsc: Improve clock continuity for stable->unstable transition From: Peter Zijlstra Date: Thu Apr 13 14:56:44 CEST 2017 Marta reported that commit: 7b09cc5a9deb ("sched/clock: Fix broken stable to unstable transfer") Appeared to have broken things on a Core2Duo machine. While that patch is in fact correct, it exposes a problem with commit: 5680d8094ffa ("sched/clock: Provide better clock continuity") Where we hoped that TSC would not make big jumps after SMP bringup. Of course, TSC needs to prove us wrong. Because Core2 comes up with a semi-stable TSC and only goes funny once we probe the idle drivers, because Core2 stops TSC on idle. Now we could of course delay the final switch to stable longer, but it would be better to entirely remove the assumption that TSC doesn't make big jumps and improve things all-round. So instead we have the clocksource watchdog call a special function when it finds the TSC is still good (there's a race, it could've gotten bad between us determining it's still good and calling our function, do we care?). This function then updates the __gtod_offset using sane values, which is the value needed for clock continuity when being marked unstable. Cc: Pavel Tatashin Cc: Martin Peres Reported-by: "Lofstedt, Marta" Fixes: 5680d8094ffa ("sched/clock: Provide better clock continuity") Signed-off-by: Peter Zijlstra (Intel) --- arch/x86/kernel/tsc.c | 12 ++++++++++ include/linux/clocksource.h | 1 include/linux/sched/clock.h | 2 - kernel/sched/clock.c | 50 ++++++++++++++++++++++++-------------------- kernel/time/clocksource.c | 3 ++ 5 files changed, 45 insertions(+), 23 deletions(-) --- a/arch/x86/kernel/tsc.c +++ b/arch/x86/kernel/tsc.c @@ -374,6 +374,8 @@ static int __init tsc_setup(char *str) tsc_clocksource_reliable = 1; if (!strncmp(str, "noirqtime", 9)) no_sched_irq_time = 1; + if (!strcmp(str, "unstable")) + mark_tsc_unstable("boot parameter"); return 1; } @@ -1127,6 +1129,15 @@ static void tsc_cs_mark_unstable(struct pr_info("Marking TSC unstable due to clocksource watchdog\n"); } +static void tsc_cs_tick_stable(struct clocksource *cs) +{ + if (tsc_unstable) + return; + + if (using_native_sched_clock()) + sched_clock_tick_stable(); +} + /* * .mask MUST be CLOCKSOURCE_MASK(64). See comment above read_tsc() */ @@ -1140,6 +1151,7 @@ static struct clocksource clocksource_ts .archdata = { .vclock_mode = VCLOCK_TSC }, .resume = tsc_resume, .mark_unstable = tsc_cs_mark_unstable, + .tick_stable = tsc_cs_tick_stable, }; void mark_tsc_unstable(char *reason) --- a/include/linux/clocksource.h +++ b/include/linux/clocksource.h @@ -96,6 +96,7 @@ struct clocksource { void (*suspend)(struct clocksource *cs); void (*resume)(struct clocksource *cs); void (*mark_unstable)(struct clocksource *cs); + void (*tick_stable)(struct clocksource *cs); /* private: */ #ifdef CONFIG_CLOCKSOURCE_WATCHDOG --- a/include/linux/sched/clock.h +++ b/include/linux/sched/clock.h @@ -63,8 +63,8 @@ extern void clear_sched_clock_stable(voi */ extern u64 __sched_clock_offset; - extern void sched_clock_tick(void); +extern void sched_clock_tick_stable(void); extern void sched_clock_idle_sleep_event(void); extern void sched_clock_idle_wakeup_event(u64 delta_ns); --- a/kernel/sched/clock.c +++ b/kernel/sched/clock.c @@ -152,25 +152,15 @@ static void __clear_sched_clock_stable(v { struct sched_clock_data *scd = this_scd(); - /* - * Attempt to make the stable->unstable transition continuous. - * - * Trouble is, this is typically called from the TSC watchdog - * timer, which is late per definition. This means the tick - * values can already be screwy. - * - * Still do what we can. - */ - __gtod_offset = (scd->tick_raw + __sched_clock_offset) - (scd->tick_gtod); + if (!sched_clock_stable()) + return; printk(KERN_INFO "sched_clock: Marking unstable (%lld, %lld)<-(%lld, %lld)\n", scd->tick_gtod, __gtod_offset, scd->tick_raw, __sched_clock_offset); tick_dep_set(TICK_DEP_BIT_CLOCK_UNSTABLE); - - if (sched_clock_stable()) - schedule_work(&sched_clock_work); + schedule_work(&sched_clock_work); } void clear_sched_clock_stable(void) @@ -347,21 +337,37 @@ void sched_clock_tick(void) { struct sched_clock_data *scd; + if (sched_clock_stable()) + return; + + if (unlikely(!sched_clock_running)) + return; + WARN_ON_ONCE(!irqs_disabled()); - /* - * Update these values even if sched_clock_stable(), because it can - * become unstable at any point in time at which point we need some - * values to fall back on. - * - * XXX arguably we can skip this if we expose tsc_clocksource_reliable - */ scd = this_scd(); scd->tick_raw = sched_clock(); scd->tick_gtod = ktime_get_ns(); + sched_clock_local(scd); +} - if (!sched_clock_stable() && likely(sched_clock_running)) - sched_clock_local(scd); +void sched_clock_tick_stable(void) +{ + u64 gtod, clock; + + if (!sched_clock_stable()) + return; + + /* + * Called under watchdog_lock. + * + * The watchdog just found this TSC to (still) be stable, so now is a + * good moment to update our __gtod_offset. Because once we find the + * TSC to be unstable, any computation will be computing crap. + */ + gtod = ktime_get_ns(); + clock = sched_clock(); + __gtod_offset = (clock + __sched_clock_offset) - gtod; } /* --- a/kernel/time/clocksource.c +++ b/kernel/time/clocksource.c @@ -233,6 +233,9 @@ static void clocksource_watchdog(unsigne continue; } + if (cs == curr_clocksource && cs->tick_stable) + cs->tick_stable(cs); + if (!(cs->flags & CLOCK_SOURCE_VALID_FOR_HRES) && (cs->flags & CLOCK_SOURCE_IS_CONTINUOUS) && (watchdog->flags & CLOCK_SOURCE_IS_CONTINUOUS)) {