Re: [PATCH 4/4] hv_util: improve time adjustment accuracy by disabling interrupts

[Vitaly Kuznetsov]

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