From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758598AbdACM2u (ORCPT ); Tue, 3 Jan 2017 07:28:50 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38986 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758572AbdACM2e (ORCPT ); Tue, 3 Jan 2017 07:28:34 -0500 From: Vitaly Kuznetsov To: Stephen Hemminger Cc: devel@linuxdriverproject.org, Haiyang Zhang , Alex Ng , linux-kernel@vger.kernel.org, John Stultz , Thomas Gleixner Subject: Re: [PATCH 4/4] hv_util: improve time adjustment accuracy by disabling interrupts References: <20170102194114.657-1-vkuznets@redhat.com> <20170102194114.657-5-vkuznets@redhat.com> <20170102115052.1eb292d6@xeon-e3> Date: Tue, 03 Jan 2017 13:28:31 +0100 In-Reply-To: <20170102115052.1eb292d6@xeon-e3> (Stephen Hemminger's message of "Mon, 2 Jan 2017 11:50:52 -0800") Message-ID: <87eg0kqqds.fsf@vitty.brq.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Tue, 03 Jan 2017 12:28:34 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Stephen Hemminger writes: > On Mon, 2 Jan 2017 20:41:14 +0100 > Vitaly Kuznetsov wrote: > >> If we happen to receive interrupts during hv_set_host_time() execution >> our adjustments may get inaccurate. Make the whole function atomic. >> Unfortunately, we can's call do_settimeofday64() with interrupts >> disabled as some cross-CPU work is being done but this call happens >> very rarely. >> >> Signed-off-by: Vitaly Kuznetsov >> --- >> drivers/hv/hv_util.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c >> index 4c0fbb0..233d5cb 100644 >> --- a/drivers/hv/hv_util.c >> +++ b/drivers/hv/hv_util.c >> @@ -186,6 +186,9 @@ static void hv_set_host_time(struct work_struct *work) >> u64 newtime; >> struct timespec64 host_ts, our_ts; >> struct timex txc = {0}; >> + unsigned long flags; >> + >> + local_irq_save(flags); >> >> wrk = container_of(work, struct adj_time_work, work); >> >> @@ -214,6 +217,7 @@ static void hv_set_host_time(struct work_struct *work) >> >> /* Try adjusting time by using phase adjustment if possible */ >> if (abs(delta) > MAXPHASE) { >> + local_irq_restore(flags); >> do_settimeofday64(&host_ts); >> return; >> } >> @@ -225,6 +229,8 @@ static void hv_set_host_time(struct work_struct *work) >> txc.status = STA_PLL; >> txc.offset = delta; >> do_adjtimex(&txc); >> + >> + local_irq_restore(flags); > > Yes, it should be atomic, but local irq save/restore is not sufficient protection > because it does not protect against premptible kernel. Why not a mutex? or a spinlock? I may be missing something, but: to make preemption happen we need to either get an interrupt or call scheduling manually (directly or via preempt_enable(), local_irq_restore(),...). Interrupts are disabled here and even if something will trigger manual schedulling it won't happen as: #define preemptible() (preempt_count() == 0 && !irqs_disabled()) I don't see a good documentation but Documentation/preempt-locking.txt says: "PREVENTING PREEMPTION USING INTERRUPT DISABLING It is possible to prevent a preemption event using local_irq_disable and local_irq_save. Note, when doing so, you must be very careful to not cause an event that would set need_resched and result in a preemption check. When in doubt, rely on locking or explicit preemption disabling." Spinlock with irqs disabled (spin_lock_irqsave()) would work too but just because we're disabling interrupts. We don't need a lock here because hv_set_host_time() is called from a workqueue and double execution is impossible. Mutex would not help at all as it is sleepable (so we may get a timer interrupt). The point I'm trying to make is: disabling interrupts is enough to prevent other code from being executed on the same CPU in the middle of hv_set_host_time(). The only exception I see is NMIs but we don't usually get them and there is no easy way of protection. -- Vitaly