lkml.org 
[lkml]   [2017]   [Jan]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 4/4] hv_util: improve time adjustment accuracy by disabling interrupts
    Date
    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

    \
     
     \ /
      Last update: 2017-01-03 13:29    [W:3.369 / U:0.876 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site