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

[Vitaly Kuznetsov]

Stephen Hemminger <stephen@xxxxxxxxxxxxxxxxxx> writes:

> On Wed,  4 Jan 2017 18:24:39 +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>
>
> Ok, the race is between timer interrupts and calling do_adjtimex().
> NTP has the same issue already.
>
> The getnstimeofday64() (or ktime_get) return an atomic value.
> If a clock tick interrupt happens during this code, then the value
> is still correct just old.
>
> If you want to avoid all races here, it looks like it would
> be better to get timekeeper_lock and call __do_adjtimex. The existing
> code in do_adjtimex() is expecting to be called from a system call
> and changing it's assumptions is probably not a good idea.
>

I was thinking about it but to me what do_adjtimex() does looks too
low-level for drivers (e.g. calling write_seqcount_begin(),
__timekeeping_set_tai_offset(), tk_update_leap_state()). To me (again, I
probably know not that much about time keeping) it looks like we'll have
to have all this stuff around the __do_adjtimex() call here.

Are there any particular concearns on calling do_adjtimex() directly?
Yes, it was written for syscalls but I don't see any other required
things in adjtimex syscall, it's just a straitforward copy_from_user()
-> do_adjtimex() -> copy_to_user() chain.

I would very much appreciate if Thomas or John could comment what's
better here long-term.

> Rather than calling system call from user space. Maybe better
> to provide real kernel API in time subsystem for this use case.
> What does KVM do?

KVM doesn't have an NTP-like service, it just provides kvm_clock (which,
BTW, implements VCLOCK_PVCLOCK for vDSO calls -- unlike Hyper-V's TSC
page). hv_utils is going to be the first in-kernel 'NTP'.

-- 
  Vitaly