WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] Re: Fix for get_s_time()

To: Keir Fraser <keir.fraser@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel] Re: Fix for get_s_time()
From: Dave Winchell <dwinchell@xxxxxxxxxxxxxxx>
Date: Tue, 22 Apr 2008 10:09:20 -0400
Cc: Dave Winchell <dwinchell@xxxxxxxxxxxxxxx>, "Tian, Kevin" <kevin.tian@xxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Ian Pratt <Ian.Pratt@xxxxxxxxxxxxx>, "dan.magenheimer@xxxxxxxxxx" <dan.magenheimer@xxxxxxxxxx>
Delivery-date: Wed, 23 Apr 2008 06:19:50 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <C432DD6E.1706C%keir.fraser@xxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <C432DD6E.1706C%keir.fraser@xxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla Thunderbird 1.0.7-1.1.fc4 (X11/20050929)
Keir,

Well, I'm glad you didn't take the patch as my overnight test was
not successful. I'm afraid that I was overly confident.
I'll re-submit when I get to the bottom of this.

thanks,
Dave

Keir Fraser wrote:

Is the issue that there is a variable delay in waiting to acquire the
platform_timer_lock? If so, you can fix that by, for example, making
read_platform_stime() the *first* thing you do inside the
local_irq_disable/enable() block in local-time_calibration(). i.e., *before*
doing the rdtsc() and get_s_time(). This avoids a variable delay between the
TSC-based time estimates and the platform-timer-based time estimate.

If that fixes your issue I'll apply that in preference.

-- Keir

On 21/4/08 23:55, "Keir Fraser" <keir.fraser@xxxxxxxxxxxxx> wrote:

What's the race? All accesses to platform time are already done under the
platform_timer_lock as far as I can see.

-- Keir

On 21/4/08 21:32, "Dave Winchell" <dwinchell@xxxxxxxxxxxxxxx> wrote:

Keir,

In my work on layering hpet on get_s_time, I found a problem
in get_s_time and related code. Because of the problem I was getting
large jumps in the offset between local time and ntp time.
These jumps were on the order of many seconds.

The issue is the race between local_time_calibration() executing
on one processor and platform_time_calibration() on another.

I have included a patch which addresses the race in
local_time_calibration(), cpu_frequency_change(), and
init_percpu_time().

I'm giving you this ahead of the hpet work as it affects all users
of get_s_time().

I'm confident of the fix in local_time_calibration() as I had failures there
before the fix and no failures after. The other two I'm less confident
in, so check
my work closely there.

On the hpet over get_s_time() front, this fix allows me to get .0014% error.
This is very close to the error going to the hardware hpet each time.

Regards,
Dave


diff -r a38a41de0800 xen/arch/x86/time.c
--- a/xen/arch/x86/time.c Wed Apr 16 16:42:47 2008 +0100
+++ b/xen/arch/x86/time.c Mon Apr 21 15:19:37 2008 -0400
@@ -530,6 +530,16 @@ static s_time_t read_platform_stime(void

    return stime;
}
+static s_time_t read_platform_stime_locked(void)
+{
+    u64 count;
+    s_time_t stime;
+
+    count = plt_count64 + ((plt_src.read_counter() - plt_count) & plt_mask);
+    stime = __read_platform_stime(count);
+
+    return stime;
+}

static void platform_time_calibration(void)
{
@@ -749,6 +759,7 @@ int cpu_frequency_change(u64 freq)
{
    struct cpu_time *t = &this_cpu(cpu_time);
    u64 curr_tsc;
+    unsigned long flags;

    /* Sanity check: CPU frequency allegedly dropping below 1MHz? */
    if ( freq < 1000000u )
@@ -758,15 +769,15 @@ int cpu_frequency_change(u64 freq)
        return -EINVAL;
    }

-    local_irq_disable();
+    spin_lock_irqsave(&platform_timer_lock, flags);
    rdtscll(curr_tsc);
    t->local_tsc_stamp = curr_tsc;
-    t->stime_master_stamp = read_platform_stime();
+    t->stime_master_stamp = read_platform_stime_locked();
    /* TSC-extrapolated time may be bogus after frequency change. */
    /*t->stime_local_stamp = get_s_time();*/
    t->stime_local_stamp = t->stime_master_stamp;
    set_time_scale(&t->tsc_scale, freq);
-    local_irq_enable();
+    spin_unlock_irqrestore(&platform_timer_lock, flags);

    /* A full epoch should pass before we check for deviation. */
    set_timer(&t->calibration_timer, NOW() + EPOCH);
@@ -830,16 +841,18 @@ static void local_time_calibration(void
    /* The overall calibration scale multiplier. */
    u32 calibration_mul_frac;

+    unsigned long flags;
+
    prev_tsc          = t->local_tsc_stamp;
    prev_local_stime  = t->stime_local_stamp;
    prev_master_stime = t->stime_master_stamp;

    /* Disable IRQs to get 'instantaneous' current timestamps. */
-    local_irq_disable();
+    spin_lock_irqsave(&platform_timer_lock, flags);
    rdtscll(curr_tsc);
    curr_local_stime  = get_s_time();
-    curr_master_stime = read_platform_stime();
-    local_irq_enable();
+    curr_master_stime = read_platform_stime_locked();
+    spin_unlock_irqrestore(&platform_timer_lock, flags);

#if 0
    printk("PRE%d: tsc=%"PRIu64" stime=%"PRIu64" master=%"PRIu64"\n",
@@ -944,10 +957,10 @@ void init_percpu_time(void)
    unsigned long flags;
    s_time_t now;

-    local_irq_save(flags);
+    spin_lock_irqsave(&platform_timer_lock, flags);
    rdtscll(t->local_tsc_stamp);
-    now = !plt_src.read_counter ? 0 : read_platform_stime();
-    local_irq_restore(flags);
+    now = !plt_src.read_counter ? 0 : read_platform_stime_locked();
+    spin_unlock_irqrestore(&platform_timer_lock, flags);

    t->stime_master_stamp = now;
    t->stime_local_stamp  = now;

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel