From patchwork Sat Sep 23 19:05:29 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Russell King (Oracle)" X-Patchwork-Id: 9967597 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 8DEDA602D8 for ; Sat, 23 Sep 2017 19:06:30 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 851012945B for ; Sat, 23 Sep 2017 19:06:30 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 675F92946F; Sat, 23 Sep 2017 19:06:30 +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.2 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id BAE4D2945B for ; Sat, 23 Sep 2017 19:06:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=PxF+2R1qe0egjESHqMlafVfM7AYtdI4SiL/5VgQ9gU0=; b=UTCQWoiD1yRDq+ s3aNR29lCxMq4XgShvnmIUROSWDIaoz8QYiRQtRRG2UFapcNE4G8An7PNv6L04h8Yi2B8fnHtdhp6 32mOQhOWkETZS/DTn0Nv6PGPfwVyJKAwQdX/amM0a5SwfKnjp5OVEzLXWvHqBcQX8+5G8oBc5zCfE /mmKL109TKi9hSuDEKpzI69F92xoRA3YIqntdtACGctCaKjh+5Z00KsHOXIoVXvFBwYAB2LXrhc2S ytY03rIxHYpe3uOovlfzoqpz9QhiYia/ft3mVVQzVcfL0JWz41nkHiUO+rNAZ4vdFCFiJVgQg42DL aSbkRGfCaBTysBX64cxA==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1dvpkl-0005Vj-4q; Sat, 23 Sep 2017 19:06:15 +0000 Received: from pandora.armlinux.org.uk ([2001:4d48:ad52:3201:214:fdff:fe10:1be6]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1dvpkf-0005Pc-C7 for linux-arm-kernel@lists.infradead.org; Sat, 23 Sep 2017 19:06:12 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2014; h=Sender:In-Reply-To:Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date; bh=/fDeOCUDJ9bVVn9W7/2jo6GlVN9LXLweSaI64BL+tMU=; b=BDIYhsCoytgk+RHX2tPDwrhN24mSMqzKbt9iytGPDAZ7CSj/sWXaxFy7cIe4HRIML660Qz8/GNglY67wn5uAk/7fOp+VQ8LqC41N5u+TLNm9nq5WhENX5vnByTxo82ah/WSr2E3DWohPCrCxgDnyCnnymMZiYJzcAVcUJIKLmhI=; Received: from n2100.armlinux.org.uk ([2001:4d48:ad52:3201:214:fdff:fe10:4f86]:47506) by pandora.armlinux.org.uk with esmtpsa (TLSv1:DHE-RSA-AES256-SHA:256) (Exim 4.82_1-5b7a7c0-XX) (envelope-from ) id 1dvpk5-0007E1-KO; Sat, 23 Sep 2017 20:05:33 +0100 Received: from linux by n2100.armlinux.org.uk with local (Exim 4.76) (envelope-from ) id 1dvpk2-00055e-2O; Sat, 23 Sep 2017 20:05:30 +0100 Date: Sat, 23 Sep 2017 20:05:29 +0100 From: Russell King - ARM Linux To: Jason Gunthorpe Subject: Re: On NTP, RTCs and accurately setting their time Message-ID: <20170923190529.GL20805@n2100.armlinux.org.uk> References: <20170921075907.GR20805@n2100.armlinux.org.uk> <20170921093218.GS20805@n2100.armlinux.org.uk> <20170921220510.GA11395@obsidianresearch.com> <20170921224541.GC20805@n2100.armlinux.org.uk> <20170921232040.GA15130@obsidianresearch.com> <20170922095713.GD20805@n2100.armlinux.org.uk> <20170922164850.GA13235@obsidianresearch.com> <20170922172022.GJ20805@n2100.armlinux.org.uk> <20170922182424.GA21945@obsidianresearch.com> <20170923182324.GK20805@n2100.armlinux.org.uk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170923182324.GK20805@n2100.armlinux.org.uk> User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170923_120609_866226_AD0F01EE X-CRM114-Status: GOOD ( 18.90 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Alessandro Zummo , Stephen Boyd , John Stultz , Alexandre Belloni , Thomas Gleixner , linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On Sat, Sep 23, 2017 at 07:23:24PM +0100, Russell King - ARM Linux wrote: > So we're getting it almost right - except for the sloppyness of the > workqueue being run (which is mostly dependent on the sloppiness of > the timer.) > > [ 2954.059895] sched_sync_hw_clock: adjust=1506188253.808470501 now=1506188253.808475661 next=0.191524339 target=0 fail=1 > [ 2954.255898] sched_sync_hw_clock: adjust=1506188254.004471056 now=1506188254.004490017 next=659.995509983 target=0 fail=0 > [ 3625.769764] sched_sync_hw_clock: adjust=1506188925.552468680 now=1506188925.552473640 next=0.447526360 target=0 fail=1 > [ 3626.249738] sched_sync_hw_clock: adjust=1506188926.032470718 now=1506188926.032474358 next=0.967525642 target=0 fail=1 > [ 3627.241688] sched_sync_hw_clock: adjust=1506188927.024472221 now=1506188927.024475501 next=0.975524499 target=0 fail=1 > [ 3628.233656] sched_sync_hw_clock: adjust=1506188928.016475925 now=1506188928.016493206 next=659.983506794 target=0 fail=0 > [ 4297.479450] sched_sync_hw_clock: adjust=1506189597.296468095 now=1506189597.296473256 next=0.703526744 target=0 fail=1 > [ 4298.215411] sched_sync_hw_clock: adjust=1506189598.032469007 now=1506189598.032472527 next=0.967527473 target=0 fail=1 > [ 4299.207366] sched_sync_hw_clock: adjust=1506189599.024475501 now=1506189599.024478982 next=0.975521018 target=0 fail=1 > [ 4300.199324] sched_sync_hw_clock: adjust=1506189600.016469555 now=1506189600.016487676 next=659.983512324 target=0 fail=0 > [ 4969.189207] sched_sync_hw_clock: adjust=1506190269.040474937 now=1506190269.040479897 next=0.959520103 target=0 fail=1 > [ 4970.181150] sched_sync_hw_clock: adjust=1506190270.032471016 now=1506190270.032474976 next=0.967525024 target=0 fail=1 > [ 4971.173104] sched_sync_hw_clock: adjust=1506190271.024476656 now=1506190271.024480136 next=0.975519864 target=0 fail=1 > [ 4972.165063] sched_sync_hw_clock: adjust=1506190272.016471455 now=1506190272.016489416 next=659.983510584 target=0 fail=0 > > It looks like we struggle to hit that window, even though it's +/- > 5 ticks (@250Hz, that's still 20ms, and we can see from the above > we're twice that.) > > So, I think the old timers are just too sloppy in modern kernels. > If we want to do this, we need to use a hrtimer to trigger a > workqueue - in other words, open-coding a hrtimer-delayed workqueue. > That's something I've already tried, and it seems to work a lot > better. I'll adapt it to your current patch. Same system, but using hrtimer instead: [ 6.256727] sched_sync_hw_clock: adjust=1506191994.278198320 now=1506191994.278210760 next=0.721789240 target=0 fail=1 [ 6.978658] sched_sync_hw_clock: adjust=1506191995.000428349 now=1506191995.000635599 next=659.999364401 target=0 fail=0 [ 666.863114] sched_sync_hw_clock: adjust=1506192655.001876494 now=1506192655.001897657 next=659.998102343 target=0 fail=0 [ 1326.830734] sched_sync_hw_clock: adjust=1506193315.001819282 now=1506193315.001840282 next=659.998159718 target=0 fail=0 # ./test-rtc Takes average of 64608ns to read RTC Took 63920ns to read RTC on second change RTC Time: 23-09-2017 19:01:59 System Time was: 19:01:59.002 which looks way better. Here's the hrtimer conversion I tested there: diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c index 32268e338015..7ab4f1628967 100644 --- a/kernel/time/ntp.c +++ b/kernel/time/ntp.c @@ -492,10 +492,58 @@ int second_overflow(time64_t secs) return leap; } +/* + * We need to use a hrtimer to queue our work - normal delayed works use + * the timer_list timers, which are too inaccurate: for a 10s delay, + * they can be delayed between 1 and 63 jiffies. + */ +struct hrtimer_delayed_work { + struct work_struct work; + struct hrtimer timer; +}; + +static enum hrtimer_restart hdw_queue_work(struct hrtimer *hr) +{ + struct hrtimer_delayed_work *w; + + w = container_of(hr, struct hrtimer_delayed_work, timer); + queue_work(system_power_efficient_wq, &w->work); + + return HRTIMER_NORESTART; +} + +static bool hdw_queued(struct hrtimer_delayed_work *hdw) +{ + return hrtimer_is_queued(&hdw->timer) != 0; +} + +static void hdw_queue(struct hrtimer_delayed_work *hdw, + const struct timespec64 *t) +{ + ktime_t ns = t ? timespec64_to_ktime(*t) : 0; + + hrtimer_start(&hdw->timer, ns, HRTIMER_MODE_REL); +} + +static bool hdw_initialised(struct hrtimer_delayed_work *hdw) +{ + return hdw->timer.function != NULL; +} + +static void hdw_init(struct hrtimer_delayed_work *hdw) +{ + hrtimer_init(&hdw->timer, CLOCK_REALTIME, HRTIMER_MODE_REL); + hdw->timer.function = hdw_queue_work; +} + + unsigned int sysctl_ntp_rtc_sync = true; static void sync_hw_clock(struct work_struct *work); -static DECLARE_DELAYED_WORK(sync_work, sync_hw_clock); + +static struct hrtimer_delayed_work sync_work = { + .work = __WORK_INITIALIZER(sync_work.work, sync_hw_clock), +}; static void sched_sync_hw_clock(struct timespec64 now, unsigned long target_nsec, bool fail) @@ -526,8 +574,7 @@ static void sched_sync_hw_clock(struct timespec64 now, next.tv_nsec -= NSEC_PER_SEC; } - queue_delayed_work(system_power_efficient_wq, &sync_work, - timespec64_to_jiffies(&next)); + hdw_queue(&sync_work, &next); printk(KERN_DEBUG "%s: adjust=%lld.%09ld now=%lld.%09ld next=%lld.%09ld target=%ld fail=%d\n", __func__, @@ -635,9 +682,12 @@ void ntp_notify_cmos_timer(void) if (!ntp_synced() || !sysctl_ntp_rtc_sync) return; - if (IS_ENABLED(CONFIG_GENERIC_CMOS_UPDATE) || - IS_ENABLED(CONFIG_RTC_SYSTOHC)) - queue_delayed_work(system_power_efficient_wq, &sync_work, 0); + if (!hdw_initialised(&sync_work)) + hdw_init(&sync_work); + + if ((IS_ENABLED(CONFIG_GENERIC_CMOS_UPDATE) || + IS_ENABLED(CONFIG_RTC_SYSTOHC)) && !hdw_queued(&sync_work)) + hdw_queue(&sync_work, NULL); } /*