fix adjtimex() freq return error
PATCH 1/1 fix adjtimex() freq return error
Martin Ziegler reported a bug in adjtimex(), where odd positive frequencies are reduced by 1 while negative values are correct.
See http://bugzilla.kernel.org/show_bug.cgi?id=11370 for details
The issue was introduced in the following commit that increased time_freq to a 64bit value and increased its resolution:
The problem is how we convert from our internal high res value back to userland. Instead of dividing, we shift, then mult, then shift again, causing the slight error seen in the bug.
I don't think adjtimex() is a terribly hot path, so this patch simply uses div_s64() to convert back to the external resolution and removes the unused shift defines to resolve the issue.
kernel/time/ntp.c +1 -3
@@ -402,9 +402,7 @@ int do_adjtimex(struct timex *txc)if (!(time_status &STA_NANO )) txc->offset /=NSEC_PER_USEC ; } - txc->freq =shift_right ((s32)(time_freq >>PPM_SCALE_INV_SHIFT ) * - (s64 )PPM_SCALE_INV , -NTP_SCALE_SHIFT ); + txc->freq =div_s64 (time_freq,PPM_SCALE ); txc->maxerror = time_maxerror; txc->esterror = time_esterror; txc->status = time_status;
include/linux/timex.h +0 -3
@@ -82,9 +82,6 @@*/ #define SHIFT_USEC 16/* frequency offset scale (shift) */ #define PPM_SCALE (NSEC_PER_USEC << (NTP_SCALE_SHIFT - SHIFT_USEC)) -#define PPM_SCALE_INV_SHIFT 20 -#define PPM_SCALE_INV ((1ll << (PPM_SCALE_INV_SHIFT + NTP_SCALE_SHIFT)) / \ - PPM_SCALE + 1)#define MAXPHASE 500000000l/* max phase error (ns) */ #define MAXFREQ 500000/* max frequency error (ns/s) */
Quoted 41 lines · On Tue, 19 Aug 2008 19:29:06 -0700 john stultz <[email protected]> wrote:
Martin Ziegler reported a bug in
adjtimex(), where odd positive frequencies are reduced by 1 while negative values are correct.See http://bugzilla.kernel.org/show_bug.cgi?id=11370 for details
The issue was introduced in the following commit that increased
time_freqto a 64bit value and increased its resolution:The problem is how we convert from our internal high res value back to userland. Instead of dividing, we shift, then mult, then shift again, causing the slight error seen in the bug.
I don't think
adjtimex()is a terribly hot path, so this patch simply usesdiv_s64()to convert back to the external resolution and removes the unused shift defines to resolve the issue.Signed-off-by John Stultz <[email protected]>Index:
2.6-git/kernel/time/ntp.c
===================================================================kernel/time/ntp.c +1
-3@@ -402,9 +402,7 @@ int do_adjtimex(struct timex *txc)if (!(time_status &STA_NANO )) txc->offset /=NSEC_PER_USEC ; } - txc->freq =shift_right ((s32)(time_freq >>PPM_SCALE_INV_SHIFT ) * - (s64 )PPM_SCALE_INV , -NTP_SCALE_SHIFT ); + txc->freq =div_s64 (time_freq,PPM_SCALE ); txc->maxerror = time_maxerror; txc->esterror = time_esterror; txc->status = time_status;Index:
2.6-git/include/linux/timex.h
===================================================================
include/linux/timex.h line 87
Quoted 9 lines
include/linux/timex.h +0
-3@@ -82,9 +82,6 @@*/ #define SHIFT_USEC 16/* frequency offset scale (shift) */ #define PPM_SCALE (NSEC_PER_USEC << (NTP_SCALE_SHIFT - SHIFT_USEC)) -#define PPM_SCALE_INV_SHIFT 20 -#define PPM_SCALE_INV ((1ll << (PPM_SCALE_INV_SHIFT + NTP_SCALE_SHIFT)) / \ - PPM_SCALE + 1)#define MAXPHASE 500000000l/* max phase error (ns) */ #define MAXFREQ 500000/* max frequency error (ns/s) */
Thanks, John. And Martin for the perfect bug report.
Based on Martin's observation that the bug has been there since 2.6.21 I marked this as needed in 2.6.25.x and in 2.6.26.x. However the patch doesn't apply at all to 2.6.25, so someone(tm) will have a bit of rework to do if we want to fix 2.6.25.x.
This fix caused Roman's ntp-fix-adj_offset_ss_read-bug-and-do_adjtimex-cleanup.patch (which I have queued for when Thomas wakes up) to toss a reject, but fixing that seemed pretty simple. I think. The scripts/kconfig/conf segfaults are being a bit of a PITA at present.
Hi,
Quoted 4 lines · Andrew Morton
Quoted 2 lines · On Tue, 19 Aug 2008 19:29:06 -0700 john stultz <[email protected]> wrote:
Martin Ziegler reported a bug in
adjtimex(), where odd positive frequencies are reduced by 1 while negative values are correct.
To put this in perspective one should also know that 1 here means an error of 2^-16ppm or less than 0.02nsec.
When I wrote this code, I noticed that the rounding isn't perfect, but OTOH the error is so small that it can't have any practical relevance. AFAIC the old code is fine as is.
bye, Roman
Hi,
Quoted 5 lines · Andrew Morton
This fix caused Roman's ntp-fix-adj_offset_ss_read-bug-and-do_adjtimex-cleanup.patch (which I have queued for when Thomas wakes up) to toss a reject, but fixing that seemed pretty simple. I think. The
scripts/kconfig/confsegfaults are being a bit of a PITA at present.
Below is a better patch on top of the above patch, note that this not a bug fix and thus not an issue for stable.
bye, Roman
[PATCH] improve adjtimex frequency rounding
Change PPM_SCALE_INV_SHIFT so that it doesn't throw away any input bits (19 is the amount of the factor 2 in PPM_SCALE), the output frequency can then be calculated back to its input value, as the inverse divide produce a slightly larger value, which is then correctly rounded by the final shift.
--- include/linux/timex.h | 2 +- kernel/time/ntp.c | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-)
include/linux/timex.h +1 -1
@@ -82,7 +82,7 @@*/ #define SHIFT_USEC 16/* frequency offset scale (shift) */ #define PPM_SCALE (NSEC_PER_USEC << (NTP_SCALE_SHIFT - SHIFT_USEC)) -#define PPM_SCALE_INV_SHIFT 20 +#define PPM_SCALE_INV_SHIFT 19#define PPM_SCALE_INV ((1ll << (PPM_SCALE_INV_SHIFT + NTP_SCALE_SHIFT)) / \ PPM_SCALE + 1)
kernel/time/ntp.c +2 -3
@@ -402,9 +402,8 @@ int do_adjtimex(struct timex *txc)if (!(time_status &STA_NANO )) txc->offset /=NSEC_PER_USEC ; } - txc->freq =shift_right ((s32)(time_freq >>PPM_SCALE_INV_SHIFT ) * - (s64 )PPM_SCALE_INV , -NTP_SCALE_SHIFT ); + txc->freq =shift_right ((time_freq >>PPM_SCALE_INV_SHIFT ) * + (s64 )PPM_SCALE_INV ,NTP_SCALE_SHIFT ); txc->maxerror = time_maxerror; txc->esterror = time_esterror; txc->status = time_status;