xen-devel
Re: [Xen-devel] Re: Fix for get_s_time()
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.
yes, this is the issue. What you suggest should be fine and I am trying
it now.
With the locking version (and a fix to a bug I introduced) I got .0012%
error
on an overnight run with hpet layered on get_s_time_mono(), which is the
max(prev, cur) layer on get_s_time we discussed.
-Dave
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
|
<Prev in Thread] |
Current Thread |
[Next in Thread>
|
- Re: [xen-devel] System time monotonicity, (continued)
- Re: [xen-devel] System time monotonicity, Keir Fraser
- Re: [xen-devel] System time monotonicity, Keir Fraser
- RE: [xen-devel] System time monotonicity, Dave Winchell
- Re: [xen-devel] System time monotonicity, Keir Fraser
- RE: [xen-devel] System time monotonicity, Dan Magenheimer
- Re: [xen-devel] System time monotonicity, Keir Fraser
- [Xen-devel] Fix for get_s_time(), Dave Winchell
- [Xen-devel] Re: Fix for get_s_time(), Keir Fraser
- Re: [Xen-devel] Re: Fix for get_s_time(), Keir Fraser
- Re: [Xen-devel] Re: Fix for get_s_time(), Dave Winchell
- Re: [Xen-devel] Re: Fix for get_s_time(),
Dave Winchell <=
- Re: [Xen-devel] Re: Fix for get_s_time(), Keir Fraser
- Re: [Xen-devel] Re: Fix for get_s_time(), Dave Winchell
- Re: [Xen-devel] Re: Fix for get_s_time(), Dave Winchell
- RE: [Xen-devel] Re: Fix for get_s_time(), Dan Magenheimer
- RE: [Xen-devel] Re: Fix for get_s_time(), Dave Winchell
- RE: [Xen-devel] Re: Fix for get_s_time(), Dan Magenheimer
- Re: [Xen-devel] Re: Fix for get_s_time(), Dave Winchell
- Re: [Xen-devel] Re: Fix for get_s_time(), Dave Winchell
- Re: [Xen-devel] Re: Fix for get_s_time(), Keir Fraser
- Re: [Xen-devel] Re: Fix for get_s_time(), Dave Winchell
|
|
|