[PATCH][RFC] Apply NTP frequency/tick changes immediately.

[PATCH][RFC] Apply NTP frequency/tick changes immediately.

[john stultz]

Hey Roman,
	It was brought to my attention that since the GENERIC_TIME changes
landed, the adjtimex behavior changed for struct timex.tick and .freq
changes. When the tick or freq value is set, we adjust the
tick_length_base in ntp_update_frequency(). However, this new value
doesn't get applied to tick_length until the next second (via
second_overflow). 

This means some applications that do quick time tweaking do not see the
requested change made as quickly as expected.

Looking at the code, it doesn't seem like it would be too hard to
immediately apply the change to the tick_length value, so its changed in
the following NTP_INTERVAL, which this patch does.

I've run a few tests with this change, and ntpd still functions fine. I
do however note that the drift value for my test system changed from
~170ppm to ~18ppm, which I didn't quite expect, and needs some
additional research.

Anyway, I just wanted to see if you had any thoughts on this sort of
change.

thanks
-john


Apply NTP tick/frequency adjustments immediately instead of waiting for
the next second to pass.

Signed-off-by: John Stultz <johnstul@us.ibm.com>

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index f5f793d..9fd0d15 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -51,6 +51,7 @@ static long ntp_tick_adj;
 
 static void ntp_update_frequency(void)
 {
+	u64 old_tick_length_base = tick_length_base;
 	u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ)
 				<< NTP_SCALE_SHIFT;
 	second_length += (s64)ntp_tick_adj << NTP_SCALE_SHIFT;
@@ -60,6 +61,11 @@ static void ntp_update_frequency(void)
 
 	tick_nsec = div_u64(second_length, HZ) >> NTP_SCALE_SHIFT;
 	tick_length_base = div_u64(tick_length_base, NTP_INTERVAL_FREQ);
+	
+	/* Don't wait for the next second_overflow, apply
+	 * the change to the tick length immediately
+	 */
+	tick_length += tick_length_base - old_tick_length_base;
 }
 
 static void ntp_update_offset(long offset)

[PATCH][RFC(again)] Apply NTP frequency/tick changes immediately.

[john stultz]

On Thu, 2009-02-12 at 19:02 -0800, john stultz wrote:
> Hey Roman,
> 	It was brought to my attention that since the GENERIC_TIME changes
> landed, the adjtimex behavior changed for struct timex.tick and .freq
> changes. When the tick or freq value is set, we adjust the
> tick_length_base in ntp_update_frequency(). However, this new value
> doesn't get applied to tick_length until the next second (via
> second_overflow). 
> 
> This means some applications that do quick time tweaking do not see the
> requested change made as quickly as expected.
> 
> Looking at the code, it doesn't seem like it would be too hard to
> immediately apply the change to the tick_length value, so its changed in
> the following NTP_INTERVAL, which this patch does.
> 
> I've run a few tests with this change, and ntpd still functions fine. I
> do however note that the drift value for my test system changed from
> ~170ppm to ~18ppm, which I didn't quite expect, and needs some
> additional research.
> 
> Anyway, I just wanted to see if you had any thoughts on this sort of
> change.

Hey Roman, Just wanted to ping you again to see if you had any
objections to this change.

I did find the cause of my test system's drift switching from 170ppm to
18ppm, and it ends up its due to my bouncing between kernel versions.
The 170ppm drift was established after running w/ a older 2.6.24 based
kernel for awhile, and after the NTP_INTERVAL_LENGTH changes
(10a398d04c4a1fc395840f4d040493375f562302) landed the ppm value was
expected to change. Comparing the same kernel 2.6.29-rc4 with and
without this patch, the ppm value did not change.

Anyway, here it is again.

thanks
-john


Apply NTP tick/frequency adjustments immediately instead of waiting for
the next second to pass.

Signed-off-by: John Stultz <johnstul@us.ibm.com>

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index f5f793d..9fd0d15 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -51,6 +51,7 @@ static long ntp_tick_adj;
 
 static void ntp_update_frequency(void)
 {
+	u64 old_tick_length_base = tick_length_base;
 	u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ)
 				<< NTP_SCALE_SHIFT;
 	second_length += (s64)ntp_tick_adj << NTP_SCALE_SHIFT;
@@ -60,6 +61,11 @@ static void ntp_update_frequency(void)
 
 	tick_nsec = div_u64(second_length, HZ) >> NTP_SCALE_SHIFT;
 	tick_length_base = div_u64(tick_length_base, NTP_INTERVAL_FREQ);
+	
+	/* Don't wait for the next second_overflow, apply
+	 * the change to the tick length immediately
+	 */
+	tick_length += tick_length_base - old_tick_length_base;
 }
 
 static void ntp_update_offset(long offset)

Re: [PATCH][RFC(again)] Apply NTP frequency/tick changes immediately.

[Andrew Morton]

On Wed, 18 Feb 2009 16:02:22 -0800
john stultz <johnstul@us.ibm.com> wrote:

> On Thu, 2009-02-12 at 19:02 -0800, john stultz wrote:
> > Hey Roman,
> > 	It was brought to my attention that since the GENERIC_TIME changes
> > landed, the adjtimex behavior changed for struct timex.tick and .freq
> > changes. When the tick or freq value is set, we adjust the
> > tick_length_base in ntp_update_frequency(). However, this new value
> > doesn't get applied to tick_length until the next second (via
> > second_overflow). 
> > 
> > This means some applications that do quick time tweaking do not see the
> > requested change made as quickly as expected.
> > 
> > Looking at the code, it doesn't seem like it would be too hard to
> > immediately apply the change to the tick_length value, so its changed in
> > the following NTP_INTERVAL, which this patch does.
> > 
> > I've run a few tests with this change, and ntpd still functions fine. I
> > do however note that the drift value for my test system changed from
> > ~170ppm to ~18ppm, which I didn't quite expect, and needs some
> > additional research.
> > 
> > Anyway, I just wanted to see if you had any thoughts on this sort of
> > change.
> 
> Hey Roman, Just wanted to ping you again to see if you had any
> objections to this change.
> 
> I did find the cause of my test system's drift switching from 170ppm to
> 18ppm, and it ends up its due to my bouncing between kernel versions.
> The 170ppm drift was established after running w/ a older 2.6.24 based
> kernel for awhile, and after the NTP_INTERVAL_LENGTH changes
> (10a398d04c4a1fc395840f4d040493375f562302) landed the ppm value was
> expected to change. Comparing the same kernel 2.6.29-rc4 with and
> without this patch, the ppm value did not change.

<tries to find a changelog in there, gives up>

> Anyway, here it is again.
> 
> thanks
> -john
> 
> 
> Apply NTP tick/frequency adjustments immediately instead of waiting for
> the next second to pass.
> 

Please modify the changelog so that it explains the reason for making
the change.

I _could_ copy-n-paste the doubly-quoted text from up top, but perhaps
that isn't how you'd have wanted to changelog it, had you wanted to
changelog it ;)

I'm particularly looking for a sense of how urgent this fix is.

> 
> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> index f5f793d..9fd0d15 100644
> --- a/kernel/time/ntp.c
> +++ b/kernel/time/ntp.c
> @@ -51,6 +51,7 @@ static long ntp_tick_adj;
>  
>  static void ntp_update_frequency(void)
>  {
> +	u64 old_tick_length_base = tick_length_base;
>  	u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ)
>  				<< NTP_SCALE_SHIFT;
>  	second_length += (s64)ntp_tick_adj << NTP_SCALE_SHIFT;
> @@ -60,6 +61,11 @@ static void ntp_update_frequency(void)
>  
>  	tick_nsec = div_u64(second_length, HZ) >> NTP_SCALE_SHIFT;
>  	tick_length_base = div_u64(tick_length_base, NTP_INTERVAL_FREQ);
> +	
> +	/* Don't wait for the next second_overflow, apply
> +	 * the change to the tick length immediately
> +	 */

the comment-layout police will get ya.

> +	tick_length += tick_length_base - old_tick_length_base;
>  }
>  
>  static void ntp_update_offset(long offset)

The patch adds new trailing whitespace.  checkpatch whines.

Re: [PATCH][RFC(again)] Apply NTP frequency/tick changes immediately.

[john stultz]

On Thu, 2009-02-19 at 14:12 -0800, Andrew Morton wrote:
> On Wed, 18 Feb 2009 16:02:22 -0800
> john stultz <johnstul@us.ibm.com> wrote:
> 
> > On Thu, 2009-02-12 at 19:02 -0800, john stultz wrote:
> > > Hey Roman,
> > > 	It was brought to my attention that since the GENERIC_TIME changes
> > > landed, the adjtimex behavior changed for struct timex.tick and .freq
> > > changes. When the tick or freq value is set, we adjust the
> > > tick_length_base in ntp_update_frequency(). However, this new value
> > > doesn't get applied to tick_length until the next second (via
> > > second_overflow). 
> > > 
> > > This means some applications that do quick time tweaking do not see the
> > > requested change made as quickly as expected.
> > > 
> > > Looking at the code, it doesn't seem like it would be too hard to
> > > immediately apply the change to the tick_length value, so its changed in
> > > the following NTP_INTERVAL, which this patch does.
> > > 
> > > I've run a few tests with this change, and ntpd still functions fine. I
> > > do however note that the drift value for my test system changed from
> > > ~170ppm to ~18ppm, which I didn't quite expect, and needs some
> > > additional research.
> > > 
> > > Anyway, I just wanted to see if you had any thoughts on this sort of
> > > change.
> > 
> > Hey Roman, Just wanted to ping you again to see if you had any
> > objections to this change.
> > 
> > I did find the cause of my test system's drift switching from 170ppm to
> > 18ppm, and it ends up its due to my bouncing between kernel versions.
> > The 170ppm drift was established after running w/ a older 2.6.24 based
> > kernel for awhile, and after the NTP_INTERVAL_LENGTH changes
> > (10a398d04c4a1fc395840f4d040493375f562302) landed the ppm value was
> > expected to change. Comparing the same kernel 2.6.29-rc4 with and
> > without this patch, the ppm value did not change.
> 
> <tries to find a changelog in there, gives up>
> 
> > Anyway, here it is again.
> > 
> > thanks
> > -john
> > 
> > 
> > Apply NTP tick/frequency adjustments immediately instead of waiting for
> > the next second to pass.
> > 
> 
> Please modify the changelog so that it explains the reason for making
> the change.

Sorry about that. It was a bit too brief. Here's one if you want to
paste it in, but I can also resubmit the entire thing:


Since the GENERIC_TIME changes landed, the adjtimex behavior changed for
struct timex.tick and .freq changed. When the tick or freq value is set,
we adjust the tick_length_base in ntp_update_frequency(). However, this
new value doesn't get applied to tick_length until the next second (via
second_overflow).

This means some applications that do quick time tweaking do not see the
requested change made as quickly as expected.

This patch patch makes it so changes to timex.tick and .freq are
immediately applied to the next NTP_INTERVAL_LENGTH.


> I _could_ copy-n-paste the doubly-quoted text from up top, but perhaps
> that isn't how you'd have wanted to changelog it, had you wanted to
> changelog it ;)
> 
> I'm particularly looking for a sense of how urgent this fix is.

Not urgent. The behavior has been this way for quite some time. So I was
hoping to sort out any objections and ideally get it ready for the
2.6.30 window.

Don't get me wrong, I appreciate folks picking things up quickly, its
much better then things getting lost, but I I had to mail Ingo to say
much of the same earlier today.

RFC still means request for comments, not request for commit, right? :)


> >  	tick_nsec = div_u64(second_length, HZ) >> NTP_SCALE_SHIFT;
> >  	tick_length_base = div_u64(tick_length_base, NTP_INTERVAL_FREQ);
> > +	
> > +	/* Don't wait for the next second_overflow, apply
> > +	 * the change to the tick length immediately
> > +	 */
> 
> the comment-layout police will get ya.
> 
> > +	tick_length += tick_length_base - old_tick_length_base;
> >  }
> >  
> >  static void ntp_update_offset(long offset)
> 
> The patch adds new trailing whitespace.  checkpatch whines.

Ah. Sorry, thanks for catching that.
-john