Stephen Hemminger <stephen@xxxxxxxxxxxxxxxxxx> writes:
> On Mon, 2 Jan 2017 20:41:14 +0100
> Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
