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

[Vitaly Kuznetsov]

John Stultz  writes:

> On Wed, Jan 4, 2017 at 9:24 AM, Vitaly Kuznetsov  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 
>> ---
>>  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 7e97231..4e50a42 100644
>> --- a/drivers/hv/hv_util.c
>> +++ b/drivers/hv/hv_util.c
>> @@ -187,6 +187,9 @@ static void hv_set_host_time(struct work_struct *work)
>> struct timespec64 host_ts, our_ts;
>> struct timex txc = {0};
>> int ret;
>> +   unsigned long flags;
>> +
>> +   local_irq_save(flags);
>>
>> wrk = container_of(work, struct adj_time_work, work);
>>
>> @@ -218,6 +221,7 @@ static void hv_set_host_time(struct work_struct *work)
>>  * ordered to sync our time by the host.
>>  */
>> if (abs(delta) > MAXPHASE || wrk->flags & ICTIMESYNCFLAG_SYNC) {
>> +   local_irq_restore(flags);
>> do_settimeofday64(_ts);
>> return;
>> }
>> @@ -232,6 +236,8 @@ static void hv_set_host_time(struct work_struct *work)
>> ret = do_adjtimex();
>> if (ret)
>> pr_debug("Failed to adjust system time: %d\n", ret);
>> +
>> +   local_irq_restore(flags);
>
> This seems like a long time to disable irqs for what your trying to
> do. I'd guess you really only want to disable irqs while you aquire
> the host and guest timestamps (so they are as close together as
> possible).  Since the delta is then calculated, I'm not sure what
> disabling irqs for calling adjtimex gets you.
>

True,

interrupts disabling would only be beneficial for the
do_settimeofday64() case but we can't actually do it there. I'll shorten
the interrupts disabling to the delta calculation only.

-- 
  Vitaly
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[John Stultz]

On Wed, Jan 4, 2017 at 9:24 AM, Vitaly Kuznetsov  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 
> ---
>  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 7e97231..4e50a42 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -187,6 +187,9 @@ static void hv_set_host_time(struct work_struct *work)
> struct timespec64 host_ts, our_ts;
> struct timex txc = {0};
> int ret;
> +   unsigned long flags;
> +
> +   local_irq_save(flags);
>
> wrk = container_of(work, struct adj_time_work, work);
>
> @@ -218,6 +221,7 @@ static void hv_set_host_time(struct work_struct *work)
>  * ordered to sync our time by the host.
>  */
> if (abs(delta) > MAXPHASE || wrk->flags & ICTIMESYNCFLAG_SYNC) {
> +   local_irq_restore(flags);
> do_settimeofday64(_ts);
> return;
> }
> @@ -232,6 +236,8 @@ static void hv_set_host_time(struct work_struct *work)
> ret = do_adjtimex();
> if (ret)
> pr_debug("Failed to adjust system time: %d\n", ret);
> +
> +   local_irq_restore(flags);


This seems like a long time to disable irqs for what your trying to
do. I'd guess you really only want to disable irqs while you aquire
the host and guest timestamps (so they are as close together as
possible).  Since the delta is then calculated, I'm not sure what
disabling irqs for calling adjtimex gets you.

thanks
-john
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Stephen Hemminger]

On Thu, 05 Jan 2017 13:35:58 +0100
Vitaly Kuznetsov  wrote:

> 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?

With out holding timekeeper_lock, I don't see how you can do the adjtime
atomically.  The userspace NTP doesn't worry about it, but in the kernel
you can be more accurate. But to do that you would need to write a new
function that is kernel specific.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Vitaly Kuznetsov]

Stephen Hemminger  writes:

> On Wed,  4 Jan 2017 18:24:39 +0100
> Vitaly Kuznetsov  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 
>
> 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Stephen Hemminger]

On Wed,  4 Jan 2017 18:24:39 +0100
Vitaly Kuznetsov  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 

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.

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?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel