RE: [PATCH] clocksource=tsc

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

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

After trying lots of things, I fell back to my known
working version of time.c and adapted it forward to
match tip.  The attached patch (on top of 18063) boots
clocksource=tsc and doesn't display the weirdness for
2-1/2 hours of testing (always showed up around 1 hour before).

It may not be elegant but it works and tip doesn't (with
clocksource=tsc).

Dan

> -----Original Message-----
> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> Sent: Wednesday, July 16, 2008 6:50 AM
> To: dan.magenheimer@oracle.com; Xen-Devel (E-mail)
> Cc: Dave Winchell
> Subject: Re: [PATCH] clocksource=tsc
> 
> 
> That's a weird set of symptoms. Perhaps dom0's sense of system time is
> diverging from Xen's? I don't see that CPUs can diverge, if 
> their TSCs are
> in sync, since we shouldn't be dynamically modifying the 
> per-CPU timestamps
> or scale factors.
> 
>  -- Keir
> 
> On 16/7/08 13:43, "Dan Magenheimer" 
> <dan.magenheimer@oracle.com> wrote:
> 
> > Well now I have to take that back.  It DOESN'T work yet.
> > I think I am experiencing "Weirdness can happen..."
> > when booting with clocksource=tsc... I was away from
> > the machine overnight but the symptoms I've seen before
> > are that the system becomes less snappy and eventually
> > grinds to a near-halt.
> >
> > Oddly, I can login most of the way on the console
> > and launch new xterm's in my VNC display, but I never
> > get a prompt, and I can't interrupt a process I left
> > running overnight in another xterm.  The time display
> > in gnome seems to have frozen about an hour after
> > I booted.  Pinging the machine works but ssh'ing to
> > it doesn't ("Connection closed")
> >
> >> -----Original Message-----
> >> From: Dan Magenheimer [mailto:dan.magenheimer@oracle.com]
> >> Sent: Tuesday, July 15, 2008 10:12 PM
> >> To: 'dan.magenheimer@oracle.com'; 'Keir Fraser'; 
> 'Xen-Devel (E-mail)'
> >> Cc: 'Dave Winchell'
> >> Subject: RE: [PATCH] clocksource=tsc
> >>
> >>
> >> OK, ignore that.  It looks like you have it all fixed
> >> in 18060.  I tried it and it now boots.  Thanks!
> >>
> >>> -----Original Message-----
> >>> From: Dan Magenheimer [mailto:dan.magenheimer@oracle.com]
> >>> Sent: Tuesday, July 15, 2008 7:16 PM
> >>> To: 'dan.magenheimer@oracle.com'; 'Keir Fraser'; 'Xen-Devel
> >> (E-mail)'
> >>> Cc: 'Dave Winchell'
> >>> Subject: RE: [PATCH] clocksource=tsc
> >>>
> >>>
> >>>>>> Returning to 32-bit read_counter(), and having NULL
> >>>>> read_counter when
> >>>>>> clocksource=tsc would be another possibility...
> >>>
> >>> Well I hacked on 18055 for awhile and just couldn't get it
> >>> to boot.  I think local_time_calibration() (and thus
> >>> init_percpu_time()) is necessary for boot, though I'm not really
> >>> sure why.  Possibly the "Weirdness can happen..." comment in
> >>> that routine?
> >>>
> >>> Anyway, this patch (on top of 18055) DOES work, returns to the
> >>> 32-bit read_counter, and re-enables local_time_calibration().
> >>> I'd suggest putting off more major surgery for another day.
> >>>
> >>> Thanks,
> >>> Dan
> >>>
> >>>> -----Original Message-----
> >>>> From: Dan Magenheimer [mailto:dan.magenheimer@oracle.com]
> >>>> Sent: Tuesday, July 15, 2008 10:04 AM
> >>>> To: dan.magenheimer@oracle.com; Keir Fraser; Xen-Devel (E-mail)
> >>>> Cc: Dave Winchell
> >>>> Subject: RE: [PATCH] clocksource=tsc
> >>>>
> >>>>
> >>>> Hmmm... 18055 also fails to boot on my machine.
> >>>>
> >>>> Could we perhaps fall back to my original patch and do
> >>>> cleanup later/separately?  I also want to try implementing
> >>>> an hpet64-based get_s_time() so will be working more
> >>>> in this code later... but want to get clocksource=tsc
> >>>> working now with minimal code impact given the freeze.
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Dan Magenheimer [mailto:dan.magenheimer@oracle.com]
> >>>>> Sent: Tuesday, July 15, 2008 9:46 AM
> >>>>> To: 'Keir Fraser'; 'Xen-Devel (E-mail)'
> >>>>> Cc: 'Dave Winchell'
> >>>>> Subject: RE: [PATCH] clocksource=tsc
> >>>>>
> >>>>>> Actually in this mode of operation we hardly need a platform
> >>>>>> timer *at all*.
> >>>>>> The idea is that we let the TSCs free-run, because we know
> >>>>>> they will behave.
> >>>>>> Returning to 32-bit read_counter(), and having NULL
> >>>>> read_counter when
> >>>>>> clocksource=tsc would be another possibility...
> >>>>>
> >>>>> That's essentially what the original tscstable.patch did, though
> >>>>> I was perhaps much uglier in the miscellaneous parts.
> >>>>>
> >>>>> Thanks,
> >>>>> Dan
> >>>>>
> >>>>
> >>>>
> >
> 
> 
>

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

diff -r ea6a9793928d xen/arch/x86/time.c
--- a/xen/arch/x86/time.c	Tue Jul 15 18:00:13 2008 +0100
+++ b/xen/arch/x86/time.c	Thu Jul 17 14:34:24 2008 -0600
@@ -484,7 +484,7 @@ static int init_pmtimer(struct platform_
  * PLATFORM TIMER 5: TSC
  */
 
-#define platform_timer_is_tsc() (!strcmp(plt_src.name, "TSC"))
+#define platform_timer_is_tsc() (plt_src.counter_bits == 64)
 static u64 tsc_freq;
 
 static u64 read_tsc_count(void)
@@ -496,22 +496,7 @@ static u64 read_tsc_count(void)
 
 static int init_tsctimer(struct platform_timesource *pts)
 {
-    unsigned int cpu;
-
-    /*
-     * TODO: evaluate stability of TSC here, return 0 if not stable.
-     * For now we assume all TSCs are synchronised and hence can all share
-     * CPU 0's calibration values.
-     */
-    for_each_cpu ( cpu )
-    {
-        if ( cpu == 0 )
-            continue;
-        memcpy(&per_cpu(cpu_time, cpu),
-               &per_cpu(cpu_time, 0),
-               sizeof(struct cpu_time));
-    }
-
+    /* TODO: evaluate stability of TSC here, return 0 if not stable */
     pts->name = "TSC";
     pts->frequency = tsc_freq;
     pts->read_counter = read_tsc_count;
@@ -541,6 +526,7 @@ static void plt_overflow(void *unused)
 {
     u64 count;
 
+    ASSERT(plt_src.counter_bits <= 32);
     spin_lock(&platform_timer_lock);
     count = plt_src.read_counter();
     plt_stamp64 += (count - plt_stamp) & plt_mask;
@@ -575,6 +561,8 @@ static void platform_time_calibration(vo
     u64 count;
     s_time_t stamp;
 
+    if ( plt_src.counter_bits == 64 )
+        return;
     spin_lock(&platform_timer_lock);
     count = plt_stamp64 + ((plt_src.read_counter() - plt_stamp) & plt_mask);
     stamp = __read_platform_stime(count);
@@ -585,6 +573,8 @@ static void platform_time_calibration(vo
 
 static void resume_platform_timer(void)
 {
+    if ( plt_src.counter_bits == 64 )
+        return;
     /* No change in platform_stime across suspend/resume. */
     platform_timer_stamp = plt_stamp64;
     plt_stamp = plt_src.read_counter();
@@ -624,12 +614,21 @@ static void init_platform_timer(void)
 
     set_time_scale(&plt_scale, pts->frequency);
 
-    plt_overflow_period = scale_delta(
-        1ull << (pts->counter_bits-1), &plt_scale);
-    init_timer(&plt_overflow_timer, plt_overflow, NULL, 0);
-    plt_overflow(NULL);
+    if (pts->counter_bits != 64 )
+    {
+        plt_overflow_period = scale_delta(
+            1ull << (pts->counter_bits-1), &plt_scale);
+        init_timer(&plt_overflow_timer, plt_overflow, NULL, 0);
+        plt_overflow(NULL);
 
-    platform_timer_stamp = plt_stamp64;
+        platform_timer_stamp = plt_stamp64;
+    }
+    else
+    {
+        platform_timer_stamp =  stime_platform_stamp = 0;
+        plt_stamp = plt_stamp64 = 0;
+    }
+
 
     printk("Platform timer is %s %s\n",
            freq_string(pts->frequency), pts->name);
@@ -752,14 +751,19 @@ static unsigned long get_cmos_time(void)
 
 s_time_t get_s_time(void)
 {
-    struct cpu_time *t = &this_cpu(cpu_time);
+    struct cpu_time *t;
     u64 tsc, delta;
     s_time_t now;
 
     rdtscll(tsc);
-    delta = tsc - t->local_tsc_stamp;
-    now = t->stime_local_stamp + scale_delta(delta, &t->tsc_scale);
-
+    if (platform_timer_is_tsc() )
+        now = scale_delta(tsc, &plt_scale);
+    else
+    {
+        t = &this_cpu(cpu_time);
+        delta = tsc - t->local_tsc_stamp;
+        now = t->stime_local_stamp + scale_delta(delta, &t->tsc_scale);
+    }
     return now;
 }
 
@@ -821,10 +825,6 @@ int cpu_frequency_change(u64 freq)
 {
     struct cpu_time *t = &this_cpu(cpu_time);
     u64 curr_tsc;
-
-    /* Nothing to do if TSC is platform timer. Assume it is constant-rate. */
-    if ( platform_timer_is_tsc() )
-        return 0;
 
     /* Sanity check: CPU frequency allegedly dropping below 1MHz? */
     if ( freq < 1000000u )
@@ -1024,12 +1024,9 @@ void init_percpu_time(void)
     unsigned long flags;
     s_time_t now;
 
-    if ( platform_timer_is_tsc() )
-        return;
-
     local_irq_save(flags);
     rdtscll(t->local_tsc_stamp);
-    now = read_platform_stime();
+    now = ( !plt_src.read_counter ) ? 0 : read_platform_stime();
     local_irq_restore(flags);
 
     t->stime_master_stamp = now;
@@ -1047,10 +1044,10 @@ int __init init_xen_time(void)
 
     local_irq_disable();
 
+    init_percpu_time();
+
     stime_platform_stamp = 0;
     init_platform_timer();
-
-    init_percpu_time();
 
     /* check if TSC is invariant during deep C state
        this is a new feature introduced by Nehalem*/
@@ -1146,12 +1143,11 @@ int time_suspend(void)
 
 int time_resume(void)
 {
-    /*u64 tmp = */init_pit_and_calibrate_tsc();
+    u64 tmp = init_pit_and_calibrate_tsc();
 
     disable_pit_irq();
 
-    /* Disable this while calibrate_tsc_ap() also is skipped. */
-    /*set_time_scale(&this_cpu(cpu_time).tsc_scale, tmp);*/
+    set_time_scale(&this_cpu(cpu_time).tsc_scale, tmp);
 
     resume_platform_timer();
 

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

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