diff mbox

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

Message ID 1441840051-20244-1-git-send-email-john.stultz@linaro.org
State Accepted
Commit 2619d7e9c92d524cb155ec89fd72875321512e5b
Headers show

Commit Message

John Stultz Sept. 9, 2015, 11:07 p.m. UTC 
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 dc491596f639438 (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(-)

Comments

Ingo Molnar Sept. 13, 2015, 8:32 a.m. UTC | #1 
* John Stultz <john.stultz@linaro.org> wrote:

> 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 dc491596f639438 (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.

>  	/* Sort out the magnitude of the correction */
> -	tick_error = abs(tick_error);
> +	tick_error = abs64(tick_error);

Ugh, and we had this bug for almost two years!

Would it be possible to make abs() warn about 64-bit types during build time,
to prevent such mishaps?

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;