From: Ulrich Obergfell <uobergfe@redhat.com>
Date: Tue, 20 Apr 2010 14:13:43 -0400
Subject: [x86_64] fix time drift due to faulty lost tick tracking
Message-id: <1024663177.442341271772823222.JavaMail.root@zmail07.collab.prod.int.phx2.redhat.com>
Patchwork-id: 24239
O-Subject: [RHEL5.6 PATCH BZ579711] time drift due to incorrect accounting of
	lost ticks if 'tick_divider' > 1
Bugzilla: 579711
RH-Acked-by: Glauber Costa <glommer@redhat.com>
RH-Acked-by: Rik van Riel <riel@redhat.com>
RH-Acked-by: Christopher Lalancette <clalance@redhat.com>
RH-Acked-by: Zachary Amsden <zamsden@redhat.com>
RH-Acked-by: Prarit Bhargava <prarit@redhat.com>

RH-Bugzilla: 579711
RH-Upstream-status: N/A

The implementation of 'tick_divider' in RHEL has introduced two notions
of ticks: 'real' ticks (IRQ0 timer interrupts) that occur at a rate of
HZ/tick_divider versus 'logical' ticks (jiffies) which mimic interrupts
at a rate of HZ.

do_timer_account_lost_ticks() does not scale lost real ticks to logical
ticks correctly. This can lead to time drifts in virtual machines that
use e.g. 'divider=10' on the kernel command line because the number of
lost ticks being accounted is too small.

do_timer_account_lost_ticks() handles VXTIME_HPET, VXTIME_PMTMR, and
VXTIME_TSC mode in an 'if (...) else if (...) else' statement. For each
mode, the variable 'lost' is computed in real ticks but only in case of
VXTIME_HPET it is being scaled to logical ticks. So, when we get to the
following section of code, 'lost' is still in _real_ ticks in case of
VXTIME_PMTMR mode and VXTIME_TSC mode.

        if (lost > (int)tick_divider - 1)  {
                handle_lost_ticks(lost, regs);
                jiffies += (u64)lost - (tick_divider - 1);
        }

However, even in case of VXTIME_HPET mode the lost tick accounting does
not work correctly. Let's consider an example with 'divider=10' and one
lost real tick. With VXTIME_HPET mode (where 'lost' is being scaled) the
above code would compute 'jiffies += 10 - (10 - 1)', i.e. it would only
add one lost logical tick instead of 10.

During testing with an experimental patch for the issues outlined above,
time drifts could still be observed with VXTIME_TSC mode. These drifts
were caused by the following code in do_timer_account_lost_ticks().

                /* True ticks of delay elapsed */
                delay *= tick_divider;

The content of the variable 'delay' is derived by latching the current
value of the PIT counter. The PIT counter is in units of the 'counter
period' (838ns), regardless of tick_divider. Hence, it does not appear
to be necessary to scale 'delay' by tick_divider.

The same issues also exist in RHEL 4.8 (BZ #579716).
Please review and please comment.

Regards,

Uli

proposed patch, based on kernel version 2.6.18-196.el5
------------------------------------------------------

Signed-off-by: Jarod Wilson <jarod@redhat.com>

diff --git a/arch/x86_64/kernel/time.c b/arch/x86_64/kernel/time.c
index 48cd2e1..9c9e88d 100644
--- a/arch/x86_64/kernel/time.c
+++ b/arch/x86_64/kernel/time.c
@@ -434,10 +434,7 @@ static void do_timer_account_lost_ticks(struct pt_regs *regs)
 		delay = inb_p(0x40);
 		delay |= inb(0x40) << 8;
 		spin_unlock(&i8253_lock);
-		/* We are in physical not logical ticks */
 		delay = LATCH - 1 - delay;
-		/* True ticks of delay elapsed */
-		delay *= tick_divider;
 	}
 
 	tsc = get_cycles_sync();
@@ -445,8 +442,6 @@ static void do_timer_account_lost_ticks(struct pt_regs *regs)
 	if (vxtime.mode == VXTIME_HPET) {
 		if (offset - vxtime.last > hpet_tick_real) {
 			lost = (offset - vxtime.last) / hpet_tick_real - 1;
-			/* Lost is now in real ticks but we want logical */
-			lost *= tick_divider;
 		}
 
 		monotonic_base += 
@@ -480,10 +475,11 @@ static void do_timer_account_lost_ticks(struct pt_regs *regs)
 			vxtime.last_tsc = tsc -
 				(((long) offset << NS_SCALE) / vxtime.tsc_quot) - 1;
 	}
-	/* SCALE: We expect tick_divider - 1 lost, ie 0 for normal behaviour */
-	if (lost > (int)tick_divider - 1)  {
+	if (lost > 0) {
+		/* Lost is now in real ticks but we want logical */
+		lost *= tick_divider;
 		handle_lost_ticks(lost, regs);
-		jiffies += (u64)lost - (tick_divider - 1);
+		jiffies += lost;
 	}
 
 	/* Do the timer stuff */