Re: Regression: can't apply frequency offsets above 1000ppm.

[John Stultz]

On Thu, Sep 3, 2015 at 4:26 AM, Miroslav Lichvar <mlichvar@xxxxxxxxxx> wrote:
> On Wed, Sep 02, 2015 at 04:16:00PM -0700, John Stultz wrote:
>> On Tue, Sep 1, 2015 at 6:14 PM, Nuno GonÃalves <nunojpg@xxxxxxxxx> wrote:
>> > And just installing chrony from the feeds. With any kernel from 3.17
>> > you'll have wrong estimates at chronyc sourcestats.
>>
>> Wrong estimates? Could you be more specific about what the failure
>> you're seeing is here? The
>>
>> I installed the image above, which comes with a 4.1.6 kernel, and
>> chrony seems to have gotten my BBB into ~1ms sync w/ servers over the
>> internet fairly quickly (at least according to chronyc tracking).
>
> To see the bug with chronyd the initial offset shouldn't be very close
> to zero, so it's forced to correct the offset by adjusting the
> frequency in a larger step.
>
> I'm attaching a simple C program that prints the frequency offset
> as measured between the REALTIME and MONOTONIC_RAW clocks when the
> adjtimex tick is set to 9000. It should show values close to -100000
> ppm and I suspect on the BBB it will be much smaller.

So I spent some time on this late last night and this afternoon.

It was a little odd because things don't seem totally broken, but
something isn't quite right.

Digging around it seems the iterative logrithmic approximation done in
timekeeping_freqadjust() wasn't working right. Instead of making
smaller order alternating positive and negative adjustments, it was
doing strange growing adjustments for the same value that wern't large
enough to actually correct things very quickly. This made it much
slower to adapt to specified frequency values.

The odd bit, is it seems to come down to:
    tick_error = abs(tick_error);

Haven't chased down why yet, but apparently abs() isn't doing what one
would think when passed a s64 value.

Anyway, the attached patch seems to improve things for me. If you can
confirm it resolves things for you I'll run it through some additional
testing after the (long holiday) weekend is over and try to get the
fix pushed out.

Thanks again for the issue report!
-john
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index bca3667..81dc975 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1586,7 +1586,7 @@ static __always_inline void timekeeping_freqadjust(struct timekeeper *tk,
 	s64 interval = tk->cycle_interval;
 	s64 xinterval = tk->xtime_interval;
 	s64 tick_error;
-	bool negative;
+	bool negative = 0;
 	u32 adj;
 
 	/* Remove any current error adj from freq calculation */
@@ -1604,10 +1604,12 @@ static __always_inline void timekeeping_freqadjust(struct timekeeper *tk,
 		return;
 
 	/* preserve the direction of correction */
-	negative = (tick_error < 0);
+	if (tick_error < 0) {
+		tick_error = -tick_error;
+		negative = 1;
+	}
 
 	/* Sort out the magnitude of the correction */
-	tick_error = abs(tick_error);
 	for (adj = 0; tick_error > interval; adj++)
 		tick_error >>= 1;