Re: [PATCH RFC] timekeeping: Update multiplier when NTP frequency is set directly

[Miroslav Lichvar]

On Wed, May 23, 2018 at 11:05:34AM -0700, John Stultz wrote:
> On Wed, May 23, 2018 at 4:33 AM, Miroslav Lichvar  wrote:
> > This removes a hidden non-deterministic delay in setting of the
> > frequency and allows an extremely tight control of the system clock
> > with update rates close to or even exceeding the kernel HZ.

> I feel like we tried this years back, but had to revert it. But its
> been awhile. Am I confusing things?

IIRC we only talked about doing this. Before the recent changes in
timekeeping, namely c2cda2a5 (Don't align NTP frequency adjustments to
ticks), it wouldn't make much sense. There would still be a hidden
delay, it would just be negative (from the applications point of
view).

> > -   /* Check if there's really nothing to do */
> > -   if (offset < real_tk->cycle_interval)
> > -   goto out;
> > -
> 
> Apologies again, as I don't have a lot of context here these days, but
>  this could mean we end up doing unnecessary work on every
> update_wall_time, no?

I'm not sure. If I understand it correctly, update_wall_time() is
normally not called more than once per tick. Only when an update is
late and it happens to include the next tick, the subsequent call
might be unnecessary, right?

> Would a "force" flag be better to pass to update_wall_time() to only
> avoid the short-cut in the non-adjtimex case?

Yes, that makes sense.

Thanks,

-- 
Miroslav Lichvar

Re: [PATCH RFC] timekeeping: Update multiplier when NTP frequency is set directly

[Miroslav Lichvar]

On Wed, May 23, 2018 at 11:05:34AM -0700, John Stultz wrote:
> On Wed, May 23, 2018 at 4:33 AM, Miroslav Lichvar  wrote:
> > This removes a hidden non-deterministic delay in setting of the
> > frequency and allows an extremely tight control of the system clock
> > with update rates close to or even exceeding the kernel HZ.

> I feel like we tried this years back, but had to revert it. But its
> been awhile. Am I confusing things?

IIRC we only talked about doing this. Before the recent changes in
timekeeping, namely c2cda2a5 (Don't align NTP frequency adjustments to
ticks), it wouldn't make much sense. There would still be a hidden
delay, it would just be negative (from the applications point of
view).

> > -   /* Check if there's really nothing to do */
> > -   if (offset < real_tk->cycle_interval)
> > -   goto out;
> > -
> 
> Apologies again, as I don't have a lot of context here these days, but
>  this could mean we end up doing unnecessary work on every
> update_wall_time, no?

I'm not sure. If I understand it correctly, update_wall_time() is
normally not called more than once per tick. Only when an update is
late and it happens to include the next tick, the subsequent call
might be unnecessary, right?

> Would a "force" flag be better to pass to update_wall_time() to only
> avoid the short-cut in the non-adjtimex case?

Yes, that makes sense.

Thanks,

-- 
Miroslav Lichvar

Re: [PATCH RFC] timekeeping: Update multiplier when NTP frequency is set directly

[John Stultz]

On Wed, May 23, 2018 at 4:33 AM, Miroslav Lichvar  wrote:
> When the NTP frequency is set directly from userspace using the
> ADJ_FREQUENCY or ADJ_TICK timex mode, immediately update the
> timekeeper's multiplier instead of waiting for the next tick.
>
> This removes a hidden non-deterministic delay in setting of the
> frequency and allows an extremely tight control of the system clock
> with update rates close to or even exceeding the kernel HZ.

Thanks for the patch!

I feel like we tried this years back, but had to revert it. But its
been awhile. Am I confusing things?


> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 49cbceef5deb..6922dae7317c 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -2047,10 +2047,6 @@ void update_wall_time(void)
>tk->tkr_mono.cycle_last, 
> tk->tkr_mono.mask);
>  #endif
>
> -   /* Check if there's really nothing to do */
> -   if (offset < real_tk->cycle_interval)
> -   goto out;
> -

Apologies again, as I don't have a lot of context here these days, but
 this could mean we end up doing unnecessary work on every
update_wall_time, no?

Would a "force" flag be better to pass to update_wall_time() to only
avoid the short-cut in the non-adjtimex case?

thanks
-john

Re: [PATCH RFC] timekeeping: Update multiplier when NTP frequency is set directly

[John Stultz]

On Wed, May 23, 2018 at 4:33 AM, Miroslav Lichvar  wrote:
> When the NTP frequency is set directly from userspace using the
> ADJ_FREQUENCY or ADJ_TICK timex mode, immediately update the
> timekeeper's multiplier instead of waiting for the next tick.
>
> This removes a hidden non-deterministic delay in setting of the
> frequency and allows an extremely tight control of the system clock
> with update rates close to or even exceeding the kernel HZ.

Thanks for the patch!

I feel like we tried this years back, but had to revert it. But its
been awhile. Am I confusing things?


> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 49cbceef5deb..6922dae7317c 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -2047,10 +2047,6 @@ void update_wall_time(void)
>tk->tkr_mono.cycle_last, 
> tk->tkr_mono.mask);
>  #endif
>
> -   /* Check if there's really nothing to do */
> -   if (offset < real_tk->cycle_interval)
> -   goto out;
> -

Apologies again, as I don't have a lot of context here these days, but
 this could mean we end up doing unnecessary work on every
update_wall_time, no?

Would a "force" flag be better to pass to update_wall_time() to only
avoid the short-cut in the non-adjtimex case?

thanks
-john

[PATCH RFC] timekeeping: Update multiplier when NTP frequency is set directly

[Miroslav Lichvar]

When the NTP frequency is set directly from userspace using the
ADJ_FREQUENCY or ADJ_TICK timex mode, immediately update the
timekeeper's multiplier instead of waiting for the next tick.

This removes a hidden non-deterministic delay in setting of the
frequency and allows an extremely tight control of the system clock
with update rates close to or even exceeding the kernel HZ.

Cc: Thomas Gleixner 
Cc: John Stultz 
Cc: Richard Cochran 
Cc: Prarit Bhargava 
Signed-off-by: Miroslav Lichvar 
---
 kernel/time/timekeeping.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 49cbceef5deb..6922dae7317c 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -2047,10 +2047,6 @@ void update_wall_time(void)
   tk->tkr_mono.cycle_last, tk->tkr_mono.mask);
 #endif
 
-   /* Check if there's really nothing to do */
-   if (offset < real_tk->cycle_interval)
-   goto out;
-
/* Do some additional sanity checking */
timekeeping_check_update(tk, offset);
 
@@ -2332,6 +2328,10 @@ int do_adjtimex(struct timex *txc)
write_seqcount_end(_core.seq);
raw_spin_unlock_irqrestore(_lock, flags);
 
+   /* Update the multiplier immediately if frequency was set directly */
+   if (txc->modes & (ADJ_FREQUENCY | ADJ_TICK))
+   update_wall_time();
+
if (tai != orig_tai)
clock_was_set();
 
-- 
2.14.3

[PATCH RFC] timekeeping: Update multiplier when NTP frequency is set directly

[Miroslav Lichvar]

When the NTP frequency is set directly from userspace using the
ADJ_FREQUENCY or ADJ_TICK timex mode, immediately update the
timekeeper's multiplier instead of waiting for the next tick.

This removes a hidden non-deterministic delay in setting of the
frequency and allows an extremely tight control of the system clock
with update rates close to or even exceeding the kernel HZ.

Cc: Thomas Gleixner 
Cc: John Stultz 
Cc: Richard Cochran 
Cc: Prarit Bhargava 
Signed-off-by: Miroslav Lichvar 
---
 kernel/time/timekeeping.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 49cbceef5deb..6922dae7317c 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -2047,10 +2047,6 @@ void update_wall_time(void)
   tk->tkr_mono.cycle_last, tk->tkr_mono.mask);
 #endif
 
-   /* Check if there's really nothing to do */
-   if (offset < real_tk->cycle_interval)
-   goto out;
-
/* Do some additional sanity checking */
timekeeping_check_update(tk, offset);
 
@@ -2332,6 +2328,10 @@ int do_adjtimex(struct timex *txc)
write_seqcount_end(_core.seq);
raw_spin_unlock_irqrestore(_lock, flags);
 
+   /* Update the multiplier immediately if frequency was set directly */
+   if (txc->modes & (ADJ_FREQUENCY | ADJ_TICK))
+   update_wall_time();
+
if (tai != orig_tai)
clock_was_set();
 
-- 
2.14.3