[PATCH 1/2][RFC] time: Fix timekeeping_freqadjust()'s incorrect use of abs() instead of abs64()

[PATCH 1/2][RFC] time: Fix timekeeping_freqadjust()'s incorrect use of abs() instead of abs64()

[John Stultz]

The internal clocksteering done for fine-grained error correction
uses a logarithmic approximation, so any time adjtimex() adjusts
the clock steering, timekeeping_freqadjust() quickly approximates
the correct clock frequency over a series of ticks.

Unfortunately, the logic in timekeeping_freqadjust(), introduced
in commit dc491596f639 ("timekeeping: Rework frequency adjustments
to work better w/ nohz"), used the abs() function with a s64 error
value to calculate the size of the approximated adjustment to be
made.

Per include/linux/kernel.h: "abs() should not be used for 64-bit
types (s64, u64, long long) - use abs64()".

Thus on 32-bit platforms, this resulted in the clocksteering to
take a quite dampended random walk trying to converge on the
proper frequency, which caused the adjustments to be made much
slower then intended (most easily observed when large adjustments
are made).

This patch fixes the issue by using abs64() instead.

Cc: Nuno Gonçalves <nunojpg@gmail.com>
Cc: Miroslav Lichvar <mlichvar@redhat.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: <stable@vger.kernel.org> # v3.17+
Reported-by: Nuno Gonçalves <nunojpg@gmail.com>
Tested-by: Nuno Goncalves <nunojpg@gmail.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/timekeeping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index f6ee2e6..3739ac6 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1614,7 +1614,7 @@ static __always_inline void timekeeping_freqadjust(struct timekeeper *tk,
 	negative = (tick_error < 0);
 
 	/* Sort out the magnitude of the correction */
-	tick_error = abs(tick_error);
+	tick_error = abs64(tick_error);
 	for (adj = 0; tick_error > interval; adj++)
 		tick_error >>= 1;
 
-- 
1.9.1

[PATCH 2/2][RFC] kselftest: timers: Add adjtick test to validate adjtimex() tick adjustments

[John Stultz]

Recently an issue was reported that was difficult to detect except
by tweaking the adjtimex tick value, and noticing how quickly the
adjustment took to be made:
	https://lkml.org/lkml/2015/9/1/488

Thus this patch introduces a new test which manipulates the adjtimex
tick value and validates the results are what we expect.

Cc: Nuno Gonçalves <nunojpg@gmail.com>
Cc: Miroslav Lichvar <mlichvar@redhat.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Shuah Khan <shuahkh@osg.samsung.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 tools/testing/selftests/timers/Makefile  |   3 +-
 tools/testing/selftests/timers/adjtick.c | 186 +++++++++++++++++++++++++++++++
 2 files changed, 188 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/timers/adjtick.c

diff --git a/tools/testing/selftests/timers/Makefile b/tools/testing/selftests/timers/Makefile
index 89a3f44..4a1be1b 100644
--- a/tools/testing/selftests/timers/Makefile
+++ b/tools/testing/selftests/timers/Makefile
@@ -8,7 +8,7 @@ LDFLAGS += -lrt -lpthread
 TEST_PROGS = posix_timers nanosleep nsleep-lat set-timer-lat mqueue-lat \
 	     inconsistency-check raw_skew threadtest rtctest
 
-TEST_PROGS_EXTENDED = alarmtimer-suspend valid-adjtimex change_skew \
+TEST_PROGS_EXTENDED = alarmtimer-suspend valid-adjtimex adjtick change_skew \
 		      skew_consistency clocksource-switch leap-a-day \
 		      leapcrash set-tai set-2038
 
@@ -24,6 +24,7 @@ include ../lib.mk
 run_destructive_tests: run_tests
 	./alarmtimer-suspend
 	./valid-adjtimex
+	./adjtick
 	./change_skew
 	./skew_consistency
 	./clocksource-switch
diff --git a/tools/testing/selftests/timers/adjtick.c b/tools/testing/selftests/timers/adjtick.c
new file mode 100644
index 0000000..b13c7fc
--- /dev/null
+++ b/tools/testing/selftests/timers/adjtick.c
@@ -0,0 +1,186 @@
+/* adjtimex() tick adjustment test
+ *		by:   John Stultz <john.stultz@linaro.org>
+ *		(C) Copyright Linaro Limited 2015
+ *		Licensed under the GPLv2
+ *
+ *  To build:
+ *	$ gcc adjtick.c -o adjtick -lrt
+ *
+ *   This program is free software: you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation, either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ */
+
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <sys/time.h>
+#include <sys/timex.h>
+#include <time.h>
+#ifdef KTEST
+#include "../kselftest.h"
+#else
+static inline int ksft_exit_pass(void)
+{
+	exit(0);
+}
+static inline int ksft_exit_fail(void)
+{
+	exit(1);
+}
+#endif
+
+
+#define CLOCK_MONOTONIC_RAW		4
+#define NSEC_PER_SEC 1000000000LL
+#define MILLION 1000000
+
+long long llabs(long long val)
+{
+	if (val < 0)
+		val = -val;
+	return val;
+}
+
+unsigned long long ts_to_nsec(struct timespec ts)
+{
+	return ts.tv_sec * NSEC_PER_SEC + ts.tv_nsec;
+}
+
+struct timespec nsec_to_ts(long long ns)
+{
+	struct timespec ts;
+
+	ts.tv_sec = ns/NSEC_PER_SEC;
+	ts.tv_nsec = ns%NSEC_PER_SEC;
+	return ts;
+}
+
+long long diff_timespec(struct timespec start, struct timespec end)
+{
+	long long start_ns, end_ns;
+
+	start_ns = ts_to_nsec(start);
+	end_ns = ts_to_nsec(end);
+	return end_ns - start_ns;
+}
+
+void get_monotonic_and_raw(struct timespec *mon, struct timespec *raw)
+{
+	struct timespec start, mid, end;
+	long long diff = 0, tmp;
+	int i;
+
+	clock_gettime(CLOCK_MONOTONIC, mon);
+	clock_gettime(CLOCK_MONOTONIC_RAW, raw);
+
+	/* Try to get a more tightly bound pairing */
+	for (i = 0; i < 3; i++) {
+		long long newdiff;
+
+		clock_gettime(CLOCK_MONOTONIC, &start);
+		clock_gettime(CLOCK_MONOTONIC_RAW, &mid);
+		clock_gettime(CLOCK_MONOTONIC, &end);
+
+		newdiff = diff_timespec(start, end);
+		if (diff == 0 || newdiff < diff) {
+			diff = newdiff;
+			*raw = mid;
+			tmp = (ts_to_nsec(start) + ts_to_nsec(end))/2;
+			*mon = nsec_to_ts(tmp);
+		}
+	}
+}
+
+long long get_ppm_drift(void)
+{
+	struct timespec mon_start, raw_start, mon_end, raw_end;
+	long long delta1, delta2, eppm;
+
+	get_monotonic_and_raw(&mon_start, &raw_start);
+
+	sleep(10);
+
+	get_monotonic_and_raw(&mon_end, &raw_end);
+
+	delta1 = diff_timespec(mon_start, mon_end);
+	delta2 = diff_timespec(raw_start, raw_end);
+
+	eppm = (delta1*MILLION)/delta2 - MILLION;
+	return eppm;
+}
+
+int check_tick_adj(long tickval)
+{
+	long long eppm, ppm;
+	struct timex tx1;
+
+	tx1.modes = ADJ_TICK;
+	tx1.modes |= ADJ_OFFSET;
+	tx1.modes |= ADJ_FREQUENCY;
+	tx1.offset = 0;
+	tx1.freq = 0;
+	tx1.tick = tickval;
+	adjtimex(&tx1);
+	sleep(1);
+
+	ppm = ((long long)tickval * MILLION)/10000 - MILLION;
+	printf("Estimating tick (act: %ld usec, %lld ppm): ", tickval, ppm);
+
+	eppm = get_ppm_drift();
+	printf("%lld usec, %lld ppm", 10000 + (10000 * eppm / MILLION), eppm);
+
+	tx1.modes = 0;
+	adjtimex(&tx1);
+	if (tx1.offset || tx1.freq || tx1.tick != tickval) {
+		printf("WARNING: Unexpected adjtimex return values, make sure ntpd is not running. ");
+		return -1;
+	}
+
+	if (llabs(eppm - ppm) > 10) {
+		printf("	[FAILED]\n");
+		return -1;
+	}
+	printf("	[OK]\n");
+	return  0;
+}
+
+int main(int argv, char **argc)
+{
+	struct timespec raw;
+	long tick, err;
+	struct timex tx1;
+
+	err = 0;
+	setbuf(stdout, NULL);
+
+	if (clock_gettime(CLOCK_MONOTONIC_RAW, &raw)) {
+		printf("ERR: NO CLOCK_MONOTONIC_RAW\n");
+		return -1;
+	}
+
+	for (tick = 9000; tick < 11000; tick += 250)
+		if (check_tick_adj(tick)) {
+			err = 1;
+			break;
+		}
+
+	/* Reset things to zero */
+	tx1.modes = ADJ_TICK;
+	tx1.modes |= ADJ_OFFSET;
+	tx1.modes |= ADJ_FREQUENCY;
+	tx1.offset = 0;
+	tx1.freq = 0;
+	tx1.tick = 10000;
+	adjtimex(&tx1);
+
+	if (err)
+		return ksft_exit_fail();
+	return ksft_exit_pass();
+}
-- 
1.9.1

Re: [PATCH 2/2][RFC] kselftest: timers: Add adjtick test to validate adjtimex() tick adjustments

[Miroslav Lichvar]

On Tue, Sep 08, 2015 at 10:57:06PM -0700, John Stultz wrote:
> Recently an issue was reported that was difficult to detect except
> by tweaking the adjtimex tick value, and noticing how quickly the
> adjustment took to be made:
> 	https://lkml.org/lkml/2015/9/1/488
> 
> Thus this patch introduces a new test which manipulates the adjtimex
> tick value and validates the results are what we expect.

Great!

> +	ppm = ((long long)tickval * MILLION)/10000 - MILLION;

I think this needs to be based on sysconf(_SC_CLK_TCK) or similar,
since the user-space HZ is not 100 on all archs.

> +	for (tick = 9000; tick < 11000; tick += 250)
> +		if (check_tick_adj(tick)) {

This too.

> +	tx1.tick = 10000;
> +	adjtimex(&tx1);

And this too.

-- 
Miroslav Lichvar

Re: [PATCH 2/2][RFC] kselftest: timers: Add adjtick test to validate adjtimex() tick adjustments

[John Stultz]

On Wed, Sep 9, 2015 at 1:32 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote:
> On Tue, Sep 08, 2015 at 10:57:06PM -0700, John Stultz wrote:
>> Recently an issue was reported that was difficult to detect except
>> by tweaking the adjtimex tick value, and noticing how quickly the
>> adjustment took to be made:
>>       https://lkml.org/lkml/2015/9/1/488
>>
>> Thus this patch introduces a new test which manipulates the adjtimex
>> tick value and validates the results are what we expect.
>
> Great!
>
>> +     ppm = ((long long)tickval * MILLION)/10000 - MILLION;
>
> I think this needs to be based on sysconf(_SC_CLK_TCK) or similar,
> since the user-space HZ is not 100 on all archs.

Good point. Fixing that up now. Will resend in a bit.

thanks
-john