Messages in this thread | | | From | Vitaly Kuznetsov <> | Subject | Re: [PATCH 4/4] hv_util: improve time adjustment accuracy by disabling interrupts | Date | Tue, 03 Jan 2017 13:28:31 +0100 |
| |
Stephen Hemminger <stephen@networkplumber.org> writes:
> On Mon, 2 Jan 2017 20:41:14 +0100 > Vitaly Kuznetsov <vkuznets@redhat.com> 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 <vkuznets@redhat.com> >> --- >> 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
| |