RE: [PATCH] rendezvous-based local time calibration WOW!

["Dan Magenheimer" <dan.magenheimer@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 3601 bytes --]

OK, how about this version.  The rendezvous only collects
the key per-cpu time data then sets up a per-cpu 1ms timer
to later update the timestamp record and vcpu system time,
so neither should have racing issues.

I've only run it for about an hour but still haven't seen
any skew over 600nsec so apparently it is the collection of
the key time data that must be closely synchronized (probably
to ensure the slope is correct) while exact synchronization
of setting the timestamp records is less important.

Note that I'm not positive I got the clocksource=tsc part
correct... but am interested in your opinion on whether
clocksource=tsc can now be eliminated anyway (as the
main reason I pushed for it was because of unacceptable
skew which with this patch appears to be fixed).

Signed-off-by: Dan Magenheimer <dan.magenheimer@oracle.com>

> -----Original Message-----
> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> Sent: Sunday, August 03, 2008 11:25 AM
> To: dan.magenheimer@oracle.com; Xen-Devel (E-mail)
> Cc: Ian Pratt; Dave Winchell
> Subject: Re: [PATCH] rendezvous-based local time calibration WOW!
> 
> 
> It's not safe to poke a new timestamp record from an interrupt handler
> (which is what the smp_call_function() callback functions 
> are). Users of the
> timestamp records (e.g., get_s_time) need 
> local_irq_save/restore() or an
> equivalent of the Linux seqlock. The latter is likely faster. 
> I'm dubious
> about update_vcpu_system_time() from an interrupt handler 
> too. It needs
> thought about how it might race with a context switch (change 
> of 'current')
> or if it interrupts an existing invocation of 
> update_vcpu_system_time().
> 
>  -- Keir
> 
> On 3/8/08 17:50, "Dan Magenheimer" <dan.magenheimer@oracle.com> wrote:
> 
> > The synchronization of local_time_calibration (l_t_c) via
> > round-to-nearest-epoch provided some improvement, but I was
> > still seeing skew up to 16usec and higher.  I measured the
> > temporal distance between the rounded-epoch vs when ltc
> > was actually running to ensure there wasn't some kind of
> > bug and found that l_t_c was running up to 150us after the
> > round-epoch and sometimes up to 50us before.  I guess this
> > is the granularity of setting a Xen timer.  While it seemed
> > that +/- 100us shouldn't cause that much skew, I finally
> > decided to try synchronization-via-rendezvous, as suggested
> > by Ian here:
> >
> > 
> http://lists.xensource.com/archives/html/xen-devel/2008-07/msg
01074.html
> http://lists.xensource.com/archives/html/xen-devel/2008-07/msg01080.html
>
> The result is phenomenal... using this approach (in attached
> patch), I have yet to see a skew exceed 1usec!!!  So this is
> about a 10-fold increase in accuracy vs the rounded-epoch
> method and about 20-fold over the one-epoch-from-NOW() method.
>
> The platform time is now read once for all processors rather
> than once per processor.  (Actually, it is read once again
> in platform_time_calibration()... by "inlining" that routine
> into master_local_time_calibration() that extra read can
> be -- and probably should be -- avoided too.)
>
> It may be too late to get this into 3.3.0 but, if so, please
> consider it asap for 3.3.1 rather than just xen-unstable/3.4.
>
> Dan
>
> ===================================
> Thanks... for the memory
> I really could use more / My throughput's on the floor
> The balloon is flat / My swap disk's fat / I've OOM's in store
> Overcommitted so much
> (with apologies to the late great Bob Hope)



[-- Attachment #2: rendezcalib3.patch --]
[-- Type: application/octet-stream, Size: 5497 bytes --]

diff -r 8951c3b84e2a xen/arch/x86/time.c
--- a/xen/arch/x86/time.c	Fri Aug 01 09:54:54 2008 +0100
+++ b/xen/arch/x86/time.c	Mon Aug 04 09:10:14 2008 -0600
@@ -56,7 +56,6 @@ struct cpu_time {
     s_time_t stime_master_stamp;
     struct time_scale tsc_scale;
     u64 cstate_plt_count_stamp;
-    struct timer calibration_timer;
 };
 
 struct platform_timesource {
@@ -66,7 +65,18 @@ struct platform_timesource {
     int counter_bits;
 };
 
+struct curr_cpu_time {
+    s_time_t local_stime;
+    s_time_t master_stime;
+    u64 local_tsc;
+    struct timer calibration_timer;
+};
+
+struct timer master_calibration_timer;
+
 static DEFINE_PER_CPU(struct cpu_time, cpu_time);
+/* save time values obtained during irq for next timer */
+static DEFINE_PER_CPU(struct curr_cpu_time, curr_cpu_time);
 
 /* TSC is invariant on C state entry? */
 static bool_t tsc_invariant;
@@ -848,9 +858,11 @@ int cpu_frequency_change(u64 freq)
     local_irq_enable();
 
     /* A full epoch should pass before we check for deviation. */
-    set_timer(&t->calibration_timer, NOW() + EPOCH);
     if ( smp_processor_id() == 0 )
+    {
+        set_timer(&master_calibration_timer, NOW() + EPOCH);
         platform_time_calibration();
+    }
 
     return 0;
 }
@@ -879,6 +891,7 @@ static void local_time_calibration(void 
 static void local_time_calibration(void *unused)
 {
     struct cpu_time *t = &this_cpu(cpu_time);
+    struct curr_cpu_time *c = &this_cpu(curr_cpu_time);
 
     /*
      * System timestamps, extrapolated from local and master oscillators,
@@ -913,7 +926,7 @@ static void local_time_calibration(void 
     {
         make_tsctimer_record(); 
         update_vcpu_system_time(current);
-        set_timer(&t->calibration_timer, NOW() + MILLISECS(10*1000));
+        set_timer(&master_calibration_timer, NOW() + MILLISECS(10*1000));
         return;
     }
 
@@ -921,15 +934,9 @@ static void local_time_calibration(void 
     prev_local_stime  = t->stime_local_stamp;
     prev_master_stime = t->stime_master_stamp;
 
-    /*
-     * Disable IRQs to get 'instantaneous' current timestamps. We read platform
-     * time first, as we may be delayed when acquiring platform_timer_lock.
-     */
-    local_irq_disable();
-    curr_master_stime = read_platform_stime();
-    curr_local_stime  = get_s_time();
-    rdtscll(curr_tsc);
-    local_irq_enable();
+    curr_local_stime = c->local_stime;
+    curr_master_stime = c->master_stime;
+    curr_tsc = c->local_tsc;
 
 #if 0
     printk("PRE%d: tsc=%"PRIu64" stime=%"PRIu64" master=%"PRIu64"\n",
@@ -1021,16 +1028,63 @@ static void local_time_calibration(void 
 
     update_vcpu_system_time(current);
 
- out:
-    set_timer(&t->calibration_timer, NEXT_EPOCH(curr_local_stime));
+out:
+    if ( smp_processor_id() == 0 )
+    {
+        platform_time_calibration();
+        set_timer(&master_calibration_timer, NEXT_EPOCH(curr_local_stime));
+    }
+}
 
-    if ( smp_processor_id() == 0 )
-        platform_time_calibration();
+static cpumask_t local_time_calibrate_cpumask = CPU_MASK_NONE;
+s_time_t curr_master_stime;
+
+static void slave_time_calibration(void *unused)
+{
+    unsigned int cpu = smp_processor_id();
+    struct curr_cpu_time *c = &this_cpu(curr_cpu_time);
+
+    local_irq_disable();
+    while ( !cpu_isset(cpu, local_time_calibrate_cpumask) )
+        cpu_relax();
+    c->local_stime = get_s_time();
+    rdtscll(c->local_tsc);
+    c->master_stime = curr_master_stime;
+    cpu_clear(cpu, local_time_calibrate_cpumask);
+    set_timer(&c->calibration_timer, c->local_stime + MILLISECS(1));
+    local_irq_enable();
+}
+
+static void master_time_calibration(void *unused)
+{
+    unsigned int cpu = smp_processor_id();
+    struct curr_cpu_time *c = &this_cpu(curr_cpu_time);
+
+    if ( platform_timer_is_tsc() )
+    {
+        smp_call_function(slave_time_calibration, NULL, 0, 0);
+        make_tsctimer_record(); 
+        update_vcpu_system_time(current);
+        set_timer(&master_calibration_timer, NOW() + MILLISECS(10*1000));
+        return;
+    }
+
+    smp_call_function(slave_time_calibration, NULL, 0, 0);
+
+    local_irq_disable();
+    curr_master_stime = c->master_stime = read_platform_stime();
+    local_time_calibrate_cpumask = cpu_online_map;
+    c->local_stime  = get_s_time();
+    rdtscll(c->local_tsc);
+    cpu_clear(cpu, local_time_calibrate_cpumask);
+    set_timer(&c->calibration_timer, c->local_stime + MILLISECS(1));
+    local_irq_enable();
 }
 
 void init_percpu_time(void)
 {
     struct cpu_time *t = &this_cpu(cpu_time);
+    struct curr_cpu_time *c = &this_cpu(curr_cpu_time);
     unsigned long flags;
     s_time_t now;
 
@@ -1049,9 +1103,14 @@ void init_percpu_time(void)
     t->stime_local_stamp  = now;
 
  out:
-    init_timer(&t->calibration_timer, local_time_calibration,
+    if ( smp_processor_id() == 0 )
+    {
+        init_timer(&master_calibration_timer, master_time_calibration,
                NULL, smp_processor_id());
-    set_timer(&t->calibration_timer, NEXT_EPOCH(NOW()));
+        set_timer(&master_calibration_timer, NEXT_EPOCH(NOW()));
+    }
+    init_timer(&c->calibration_timer, local_time_calibration,
+               NULL, smp_processor_id());
 }
 
 /* Late init function (after all CPUs are booted). */
@@ -1170,7 +1229,7 @@ int time_suspend(void)
     }
 
     /* Better to cancel calibration timer for accuracy. */
-    kill_timer(&this_cpu(cpu_time).calibration_timer);
+    kill_timer(&master_calibration_timer);
 
     return 0;
 }

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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