clocksource=tsc

PATCH In review Dan Magenheimer 3 participants 28 messages Jul 12, 2008 – Jul 18, 2008

Share & tools

Dan Magenheimer

author

2008-07-12 21:38 UTC #0 lore raw

PATCH 1/1 clocksource=tsc

Attached patch adds clocksource=tsc boot option that uses TSC as clocksource. This option should only be used on machines where TSC is known to be synchronized across all processors. A future TODO is to dynamically determine if this is the case.

TSC may "beat" with another clocksource, resulting in cross-processor Xen system time skew. This skew can be visible in PV guests and can appear as "time is stopped" in hvm guests. On some systems, this patch can reduce skew by 30x or more.

Signed-off-by Dan Magenheimer <[email protected]>

===================================
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)

xen/arch/x86/time.c +60 ~~-14~~

diff -r bd97e45e073a xen/arch/x86/time.c
@@ -452,6 +452,24 @@ static int init_pmtimer(struct platform_
 }
 
 /************************************************************
+ * PLATFORM TIMER 4: TSC
+ */
+
+static bool_t clocksource_is_tsc = 0;
+static u64 tsc_freq;
+
+static int init_tsctimer(struct platform_timesource *pts)
+{
+    /* TODO: evaluate stability of TSC here, return 0 if not stable */
+    pts->name = "TSC";
+    pts->frequency = tsc_freq;
+    pts->read_counter = 0;  /* unused as of now */
+    pts->counter_bits = 64;
+    clocksource_is_tsc = 1;
+    return 1;
+}
+
+/************************************************************
  * GENERIC PLATFORM TIMER INFRASTRUCTURE
  */

@@ -483,16 +501,28 @@ static void plt_overflow(void *unused)
 
 static s_time_t __read_platform_stime(u64 platform_time)
 {
-    u64 diff = platform_time - platform_timer_stamp;
+    u64 diff, tsc;
+
+    if ( clocksource_is_tsc )
+    {
+        rdtscll(tsc);
+        return scale_delta(tsc, &plt_scale);
+    }
+    diff = platform_time - platform_timer_stamp;
     ASSERT(spin_is_locked(&platform_timer_lock));
     return (stime_platform_stamp + scale_delta(diff, &plt_scale));
 }
 
 static s_time_t read_platform_stime(void)
 {
-    u64 count;
+    u64 count, tsc;
     s_time_t stime;
 
+    if ( clocksource_is_tsc )
+    {
+        rdtscll(tsc);
+        return scale_delta(tsc, &plt_scale);
+    }
     spin_lock(&platform_timer_lock);
     count = plt_stamp64 + ((plt_src.read_counter() - plt_stamp) & plt_mask);
     stime = __read_platform_stime(count);

@@ -506,6 +536,8 @@ static void platform_time_calibration(vo
     u64 count;
     s_time_t stamp;
 
+    if ( clocksource_is_tsc )
+        return;
     spin_lock(&platform_timer_lock);
     count = plt_stamp64 + ((plt_src.read_counter() - plt_stamp) & plt_mask);
     stamp = __read_platform_stime(count);

@@ -516,6 +548,8 @@ static void platform_time_calibration(vo
 
 static void resume_platform_timer(void)
 {
+    if ( clocksource_is_tsc )
+        return;
     /* No change in platform_stime across suspend/resume. */
     platform_timer_stamp = plt_stamp64;
     plt_stamp = plt_src.read_counter();

@@ -536,6 +570,8 @@ static void init_platform_timer(void)
             rc = init_cyclone(pts);
         else if ( !strcmp(opt_clocksource, "acpi") )
             rc = init_pmtimer(pts);
+        else if ( !strcmp(opt_clocksource, "tsc") )
+            rc = init_tsctimer(pts);
 
         if ( rc <= 0 )
             printk("WARNING: %s clocksource '%s'.\n",

@@ -549,16 +585,20 @@ static void init_platform_timer(void)
          !init_pmtimer(pts) )
         init_pit(pts);
 
-    plt_mask = (u32)~0u >> (32 - pts->counter_bits);
-
     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_mask = (u32)~0u >> (32 - pts->counter_bits);
 
-    platform_timer_stamp = plt_stamp64;
+        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;
+    }
+
 
     printk("Platform timer is %s %s\n",
            freq_string(pts->frequency), pts->name);

@@ -580,7 +620,8 @@ void cstate_restore_tsc(void)
     u32    plt_count_delta;
     u64    tsc_delta;
 
-    if (!tsc_invariant){
+    if ( !tsc_invariant && !clocksource_is_tsc )
+    {
         t = &this_cpu(cpu_time);
 
         /* if platform counter overflow happens, interrupt will bring CPU from

@@ -687,14 +728,18 @@ 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 ( clocksource_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;
 }

@@ -996,6 +1041,7 @@ void __init early_time_init(void)
 {
     u64 tmp = init_pit_and_calibrate_tsc();
 
+    tsc_freq = tmp;
     set_time_scale(&this_cpu(cpu_time).tsc_scale, tmp);
 
     do_div(tmp, 1000);

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Dan Magenheimer

author

2008-07-13 03:59 UTC #1 lore raw

I meant to add... although this appears to be new functionality and thus too late for 3.3, I'd like to argue that it is really a bug fix: Let's say there are good-tsc machines and bad-tsc machines. Prior to cset 17716 ("hvm: Build guest timers on monotonic system time") all guest platform timer reads were built directly on top of physical TSC. This was a big problem for bad-tsc SMP machines, but fine for good-tsc machines. Cset 17716 made things better for bad-tsc machines, but actually made them worse for good-tsc SMP machines (because of the inter-processor skew).

So users with good-tsc machines moving from a pre-3.3 release to 3.3 may see time "stopped" in 3.3 when there was no problem
with pre-3.3. The clocksource=tsc patch partially
fixes this "regression" for good-tsc machines... at least it provides a boot option to fix it.

Thanks,
Dan

Quoted 20 lines

-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Saturday, July 12, 2008 3:38 PM
To: Xen-Devel (E-mail); Keir Fraser; [email protected]
Cc: Dave Winchell
Subject: [PATCH] clocksource=tsc
Attached patch adds clocksource=tsc boot option that uses TSC as clocksource. This option should only be used on machines where TSC is known to be synchronized across all processors. A future TODO is to dynamically determine if this is the case.
TSC may "beat" with another clocksource, resulting in cross-processor Xen system time skew. This skew can be visible in PV guests and can appear as "time is stopped" in hvm guests. On some systems, this patch can reduce skew by 30x or more.

Quoted 9 lines

Signed-off-by Dan Magenheimer <[email protected]>
===================================
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)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Keir Fraser

2008-07-14 09:24 UTC #2 lore raw

In reply to Dan Magenheimer

I'm sympathetic to the patch, but it looks like an ugly hack right now. Most of the clocksource_is_tsc special cases will fall away if you, e.g., have platform_timer_stamp = stime_platform_stamp = local_tsc_stamp = stime_local_stamp = 0; and provide a plt->read_counter() function; and accept that for now you will unnecessarily take the platform-timer spinlock; etc. Also it is platform timer 5, not 4!

Clean it up and it'll go in. Since it should clearly only affect behaviour when the new clocksource is explicitly selected, I'll take this patch further into 3.3 freeze than usual -- it's more important to me that it's clean.

Thanks,
Keir

Quoted 20 lines · Dan Magenheimer

Attached patch adds clocksource=tsc boot option that uses TSC as clocksource. This option should only be used on machines where TSC is known to be synchronized across all processors. A future TODO is to dynamically determine if this is the case.
TSC may "beat" with another clocksource, resulting in cross-processor Xen system time skew. This skew can be visible in PV guests and can appear as "time is stopped" in hvm guests. On some systems, this patch can reduce skew by 30x or more.
Signed-off-by Dan Magenheimer <[email protected]>
===================================
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)

Dan Magenheimer

author

2008-07-14 17:59 UTC #3 lore raw

Quoted 5 lines

I'm sympathetic to the patch, but it looks like an ugly hack right now.
Since it should clearly only affect behaviour when the new clocksource is explicitly selected...

Actually the ugliness is because the read_counter function and plt_mask are both u32 and I worried that changing them to a u64 would be too invasive.

So here's two possible replacement patches, one with those switched to u64 and the other with separate 32- and 64-bit read_counter functions. Both are boot-tested with and without clocksource=tsc.

Dan

Quoted 25 lines

-----Original Message-----
From: Keir Fraser [mailto:[email protected]]
Sent: Monday, July 14, 2008 3:24 AM
To: [email protected]; Xen-Devel (E-mail)
Cc: Dave Winchell
Subject: Re: [PATCH] clocksource=tsc
I'm sympathetic to the patch, but it looks like an ugly hack right now. Most
of the clocksource_is_tsc special cases will fall away if you, e.g., have
platform_timer_stamp = stime_platform_stamp = local_tsc_stamp = stime_local_stamp = 0; and provide a plt->read_counter() function; and accept that for now you will unnecessarily take the platform-timer spinlock;
etc. Also it is platform timer 5, not 4!
Clean it up and it'll go in. Since it should clearly only affect behaviour
when the new clocksource is explicitly selected, I'll take this patch further into 3.3 freeze than usual -- it's more important to me that it's
clean.

Quoted 29 lines

Thanks,
Keir
Quoted 20 lines · Dan Magenheimer
Attached patch adds clocksource=tsc boot option that uses TSC as clocksource. This option should only be used on machines where TSC is known to be synchronized across all processors. A future TODO is to dynamically determine if this is the case.
TSC may "beat" with another clocksource, resulting in cross-processor Xen system time skew. This skew can be visible in PV guests and can appear as "time is stopped" in hvm guests. On some systems, this patch can reduce skew by 30x or more.
Signed-off-by Dan Magenheimer <[email protected]>
===================================
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)

xen/arch/x86/time.c +73 ~~-26~~

diff -r bd97e45e073a xen/arch/x86/time.c
@@ -54,14 +54,14 @@ struct cpu_time {
     s_time_t stime_local_stamp;
     s_time_t stime_master_stamp;
     struct time_scale tsc_scale;
-    u32 cstate_plt_count_stamp;
+    u64 cstate_plt_count_stamp;
     struct timer calibration_timer;
 };
 
 struct platform_timesource {
     char *name;
     u64 frequency;
-    u32 (*read_counter)(void);
+    u64 (*read_counter)(void);
     int counter_bits;
 };

@@ -311,7 +311,7 @@ static char *freq_string(u64 freq)
  * PLATFORM TIMER 1: PROGRAMMABLE INTERVAL TIMER (LEGACY PIT)
  */
 
-static u32 read_pit_count(void)
+static u64 read_pit_count(void)
 {
     u16 count16;
     u32 count32;

@@ -327,7 +327,7 @@ static u32 read_pit_count(void)
 
     spin_unlock_irqrestore(&pit_lock, flags);
 
-    return count32;
+    return (u64)count32;
 }
 
 static void init_pit(struct platform_timesource *pts)

@@ -343,9 +343,9 @@ static void init_pit(struct platform_tim
  * PLATFORM TIMER 2: HIGH PRECISION EVENT TIMER (HPET)
  */
 
-static u32 read_hpet_count(void)
+static u64 read_hpet_count(void)
 {
-    return hpet_read32(HPET_COUNTER);
+    return (u64)hpet_read32(HPET_COUNTER);
 }
 
 static int init_hpet(struct platform_timesource *pts)

@@ -383,9 +383,9 @@ int use_cyclone;
 /* Cyclone MPMC0 register. */
 static volatile u32 *cyclone_timer;
 
-static u32 read_cyclone_count(void)
+static u64 read_cyclone_count(void)
 {
-    return *cyclone_timer;
+    return (u64)*cyclone_timer;
 }
 
 static volatile u32 *map_cyclone_reg(unsigned long regaddr)

@@ -433,9 +433,9 @@ u32 pmtmr_ioport;
 /* ACPI PM timer ticks at 3.579545 MHz. */
 #define ACPI_PM_FREQUENCY 3579545
 
-static u32 read_pmtimer_count(void)
+static u64 read_pmtimer_count(void)
 {
-    return inl(pmtmr_ioport);
+    return (u64)inl(pmtmr_ioport);
 }
 
 static int init_pmtimer(struct platform_timesource *pts)

@@ -452,15 +452,38 @@ static int init_pmtimer(struct platform_
 }
 
 /************************************************************
+ * PLATFORM TIMER 5: TSC
+ */
+
+static u64 tsc_freq;
+
+static u64 read_tsc_count(void)
+{
+        u64 tsc;
+	rdtscll(tsc);
+        return tsc;
+}
+
+static int init_tsctimer(struct platform_timesource *pts)
+{
+    /* TODO: evaluate stability of TSC here, return 0 if not stable */
+    pts->name = "TSC";
+    pts->frequency = tsc_freq;
+    pts->read_counter = read_tsc_count;
+    pts->counter_bits = 64;
+    return 1;
+}
+
+/************************************************************
  * GENERIC PLATFORM TIMER INFRASTRUCTURE
  */
 
 static struct platform_timesource plt_src; /* details of chosen timesource  */
-static u32 plt_mask;             /* hardware-width mask                     */
+static u64 plt_mask;             /* hardware-width mask                     */
 static u64 plt_overflow_period;  /* ns between calls to plt_overflow()      */
 static struct time_scale plt_scale; /* scale: platform counter -> nanosecs  */
 
-/* Protected by platform_timer_lock. */
+/* Protected by platform_timer_lock.  Must be zero for 64-bit clocksources */
 static DEFINE_SPINLOCK(platform_timer_lock);
 static s_time_t stime_platform_stamp; /* System time at below platform time */
 static u64 platform_timer_stamp;      /* Platform time at above system time */

@@ -472,8 +495,9 @@ static void plt_overflow(void *unused)
 {
     u32 count;
 
+    ASSERT(plt_src.counter_bits <= 32);
     spin_lock(&platform_timer_lock);
-    count = plt_src.read_counter();
+    count = (u32)plt_src.read_counter();
     plt_stamp64 += (count - plt_stamp) & plt_mask;
     plt_stamp = count;
     spin_unlock(&platform_timer_lock);

@@ -506,6 +530,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);

@@ -516,6 +542,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();

@@ -536,6 +564,8 @@ static void init_platform_timer(void)
             rc = init_cyclone(pts);
         else if ( !strcmp(opt_clocksource, "acpi") )
             rc = init_pmtimer(pts);
+        else if ( !strcmp(opt_clocksource, "tsc") )
+            rc = init_tsctimer(pts);
 
         if ( rc <= 0 )
             printk("WARNING: %s clocksource '%s'.\n",

@@ -549,16 +579,26 @@ static void init_platform_timer(void)
          !init_pmtimer(pts) )
         init_pit(pts);
 
-    plt_mask = (u32)~0u >> (32 - pts->counter_bits);
-
     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_mask = (u32)~0u >> (32 - pts->counter_bits);
 
-    platform_timer_stamp = plt_stamp64;
+        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;
+    }
+    else
+    {
+        plt_mask = -1LL;
+        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);

@@ -577,10 +617,11 @@ void cstate_restore_tsc(void)
 void cstate_restore_tsc(void)
 {
     struct cpu_time *t;
-    u32    plt_count_delta;
+    u64    plt_count_delta;
     u64    tsc_delta;
 
-    if (!tsc_invariant){
+    if ( !tsc_invariant )
+    {
         t = &this_cpu(cpu_time);
 
         /* if platform counter overflow happens, interrupt will bring CPU from

@@ -687,14 +728,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 ( plt_src.counter_bits == 64 )
+        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;
 }

@@ -996,6 +1042,7 @@ void __init early_time_init(void)
 {
     u64 tmp = init_pit_and_calibrate_tsc();
 
+    tsc_freq = tmp;
     set_time_scale(&this_cpu(cpu_time).tsc_scale, tmp);
 
     do_div(tmp, 1000);

xen/arch/x86/time.c +88 ~~-29~~

257 lines not on this page · raw message

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Tian, Kevin

2008-07-15 00:35 UTC #4 lore raw

Quoted 13 lines

From: Dan Magenheimer
Sent: 2008年7月15日 2:00
Quoted 5 lines
I'm sympathetic to the patch, but it looks like an ugly hack right now.
Since it should clearly only affect behaviour when the new clocksource is explicitly selected...
Actually the ugliness is because the read_counter function and plt_mask are both u32 and I worried that changing them to a u64 would be too invasive.

Quoted 6 lines

So here's two possible replacement patches, one with those switched to u64 and the other with separate 32- and 64-bit read_counter functions. Both are boot-tested with and without clocksource=tsc.
Dan

Boot option is good enough for now, but finally I guess you may need some type of dynamic clocksource switch mechanism, just like what Linux does today. User may be not sure of the specific platform. Some features may affect TSC stabilitity, e.g. Px may have freq change and Cx may have TSC stop on some platform. On those path a mark_tsc_unstable step is required to switch current clocksource to other available platfrom timer on the fly. So besides the boot option, Xen itself needs to code such condition checks for TSC stability. :-)

Thanks,
Kevin

Dan Magenheimer

author

2008-07-17 23:47 UTC #5 lore raw

Quoted 9 lines

Boot option is good enough for now, but finally I guess you may need some type of dynamic clocksource switch mechanism, just like what Linux does today. User may be not sure of the specific platform. Some features may affect TSC stabilitity, e.g. Px may have freq change and Cx may have TSC stop on some platform. On those path a mark_tsc_unstable step is required to switch current clocksource to other available platfrom timer on the fly. So besides the boot option, Xen itself needs to code such condition checks for TSC stability. :-)

Yes, agreed.

I think there are "good tsc" machines where TSC will never skew, "bad tsc" machines where the skew is apparent at boot, and "grey tsc" machines where there is skew but the skew happens to be small at boot but may grow to be bad post-boot possibly due to Px/Cx. In order to handle all of these here's the algorithm that I'm thinking of:

1) Use processor bits (borrowing code from a recent Linux version) to determine whether a system is likely to be "good tsc" or "bad tsc". Set the tsc_invariant global variable accordingly.
2) When synchronize_tsc_bp()/ap() dynamically evaluates skew, change tsc_invariant if appropriate.
3) If tsc_invariant is set when clocksource is being selected, tsc should be the default clocksource, unless overridden by clocksource= on the boot line OR a new boot parameter "notsc".
4) Write a pair of routines equivalent to synchronize_tsc_bp/ap() but which
just returns whether or not TSCs are sync'ed. Call this routine whenever a processor exits from Cx/Px and also on a decaying counter, e.g. 1 second after boot, then 2 seconds after that, then 4 seconds after that, etc. If skew is detected, change the clocksource to the next best and printk the change.
5) I don't know if it is currently "safe" to change clocksources after the initial selection in init_platform_timer() so this may take some work.

Comments?

Thanks,
Dan

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Keir Fraser

2008-07-15 13:05 UTC #6 lore raw

In reply to Dan Magenheimer

Quoted 6 lines · Dan Magenheimer

Quoted 5 lines
I'm sympathetic to the patch, but it looks like an ugly hack right now.
Since it should clearly only affect behaviour when the new clocksource is explicitly selected...

Quoted 8 lines

Actually the ugliness is because the read_counter function and plt_mask are both u32 and I worried that changing them to a u64 would be too invasive.
So here's two possible replacement patches, one with those switched to u64 and the other with separate 32- and 64-bit read_counter functions. Both are boot-tested with and without clocksource=tsc.

Changeset 18055 is what I had in mind. I split out the read_counter-is-64-bit portion separately. The changeset needs a bit of testing and it may need a tweak or two.

-- Keir

Dan Magenheimer

author

2008-07-15 14:44 UTC #7 lore raw

Quoted 4 lines

Changeset 18055 is what I had in mind. I split out the read_counter-is-64-bit portion separately. The changeset needs a bit of
testing and it may need a tweak or two.

Hmmm... One thing I was trying to do with the special casing in get_s_time() was to avoid using a dynamically changing scaling (t->tsc_scale). If I'm not mistaken, t->tsc_scale is recalculated every EPOCH and thus is a potential source of stime jitter. Seems unnecessary just to make the code look cleaner.

One other nit: Especially if the above is changed, do you really prefer a strcmp vs a global variable (or checking for size == 64) for determining if the tsc is the platform timer?

Thanks,
Dan

Keir Fraser

2008-07-15 15:08 UTC #8 lore raw

Quoted 6 lines · Dan Magenheimer

Hmmm... One thing I was trying to do with the special casing in get_s_time() was to avoid using a dynamically changing scaling (t->tsc_scale). If I'm not mistaken, t->tsc_scale is recalculated every EPOCH and thus is a potential source of stime jitter. Seems unnecessary just to make the code look cleaner.

My patch disables the per-epoch calibration.

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...

Quoted 4 lines

One other nit: Especially if the above is changed, do you really prefer a strcmp vs a global variable (or checking for size == 64) for determining if the tsc is the platform timer?

It's hidden in a macro and it's called very rarely. So I think it's fine.

-- Keir

Dan Magenheimer

author

2008-07-15 15:46 UTC #9 lore raw

Quoted 8 lines

Quoted 6 lines
Hmmm... One thing I was trying to do with the special casing in get_s_time() was to avoid using a dynamically changing scaling (t->tsc_scale). If I'm not mistaken, t->tsc_scale is recalculated every EPOCH and thus is a potential source of stime jitter. Seems unnecessary just to make the code look cleaner.
My patch disables the per-epoch calibration.

OK, then I think it is the calls to calibrate_tsc_ap() that result in different values of tsc_scale (potentially a different one for every processor depending on the precision of the calibration). I still think it would be clearer and safer to always use a fixed tsc_scale value.

Quoted 6 lines

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

Dan Magenheimer

author

2008-07-15 16:04 UTC #10 lore raw

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.

Quoted 21 lines

-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 9:46 AM
To: 'Keir Fraser'; 'Xen-Devel (E-mail)'
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 5 lines
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
Quoted 1 line
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

Dan Magenheimer

author

2008-07-16 01:15 UTC #11 lore raw

Quoted 3 lines

Quoted 3 lines
Quoted 1 line
Returning to 32-bit read_counter(), and having NULL
read_counter when
Quoted 1 line
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

Quoted 8 lines

-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 10:04 AM
To: [email protected]; Keir Fraser; Xen-Devel (E-mail)
Cc: Dave Winchell
Subject: RE: [PATCH] clocksource=tsc

Quoted 31 lines

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.
Quoted 21 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 9:46 AM
To: 'Keir Fraser'; 'Xen-Devel (E-mail)'
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 5 lines
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
Quoted 1 line
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

xen/arch/x86/time.c +54 ~~-38~~

diff -r 675fb031df88 xen/arch/x86/time.c
@@ -54,14 +54,14 @@ struct cpu_time {
     s_time_t stime_local_stamp;
     s_time_t stime_master_stamp;
     struct time_scale tsc_scale;
-    u64 cstate_plt_count_stamp;
+    u32 cstate_plt_count_stamp;
     struct timer calibration_timer;
 };
 
 struct platform_timesource {
     char *name;
     u64 frequency;
-    u64 (*read_counter)(void);
+    u32 (*read_counter)(void);
     int counter_bits;
 };

@@ -340,7 +340,7 @@ static char *freq_string(u64 freq)
  * PLATFORM TIMER 1: PROGRAMMABLE INTERVAL TIMER (LEGACY PIT)
  */
 
-static u64 read_pit_count(void)
+static u32 read_pit_count(void)
 {
     u16 count16;
     u32 count32;

@@ -372,7 +372,7 @@ static void init_pit(struct platform_tim
  * PLATFORM TIMER 2: HIGH PRECISION EVENT TIMER (HPET)
  */
 
-static u64 read_hpet_count(void)
+static u32 read_hpet_count(void)
 {
     return hpet_read32(HPET_COUNTER);
 }

@@ -412,7 +412,7 @@ int use_cyclone;
 /* Cyclone MPMC0 register. */
 static volatile u32 *cyclone_timer;
 
-static u64 read_cyclone_count(void)
+static u32 read_cyclone_count(void)
 {
     return *cyclone_timer;
 }

@@ -462,7 +462,7 @@ u32 pmtmr_ioport;
 /* ACPI PM timer ticks at 3.579545 MHz. */
 #define ACPI_PM_FREQUENCY 3579545
 
-static u64 read_pmtimer_count(void)
+static u32 read_pmtimer_count(void)
 {
     return inl(pmtmr_ioport);
 }

@@ -484,22 +484,15 @@ 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)
-{
-    u64 tsc;
-    rdtscll(tsc);
-    return tsc;
-}
 
 static int init_tsctimer(struct platform_timesource *pts)
 {
     /* TODO: evaluate stability of TSC here, return 0 if not stable. */
     pts->name = "TSC";
     pts->frequency = tsc_freq;
-    pts->read_counter = read_tsc_count;
+    pts->read_counter = 0;  /* unused as of now */
     pts->counter_bits = 64;
     return 1;
 }

@@ -518,13 +511,14 @@ static s_time_t stime_platform_stamp; /*
 static s_time_t stime_platform_stamp; /* System time at below platform time */
 static u64 platform_timer_stamp;      /* Platform time at above system time */
 static u64 plt_stamp64;          /* 64-bit platform counter stamp           */
-static u64 plt_stamp;            /* hardware-width platform counter stamp   */
+static u32 plt_stamp;            /* hardware-width platform counter stamp   */
 static struct timer plt_overflow_timer;
 
 static void plt_overflow(void *unused)
 {
-    u64 count;
+    u32 count;
 
+    ASSERT(plt_src.counter_bits != 64);
     spin_lock(&platform_timer_lock);
     count = plt_src.read_counter();
     plt_stamp64 += (count - plt_stamp) & plt_mask;

@@ -536,16 +530,28 @@ static void plt_overflow(void *unused)
 
 static s_time_t __read_platform_stime(u64 platform_time)
 {
-    u64 diff = platform_time - platform_timer_stamp;
+    u64 diff, tsc;
+
+    if ( platform_timer_is_tsc() )
+    {
+        rdtscll(tsc);
+        return scale_delta(tsc, &plt_scale);
+    }
+    diff = platform_time - platform_timer_stamp;
     ASSERT(spin_is_locked(&platform_timer_lock));
     return (stime_platform_stamp + scale_delta(diff, &plt_scale));
 }
 
 static s_time_t read_platform_stime(void)
 {
-    u64 count;
+    u64 count, tsc;
     s_time_t stime;
 
+    if ( platform_timer_is_tsc() )
+    {
+        rdtscll(tsc);
+        return scale_delta(tsc, &plt_scale);
+    }
     spin_lock(&platform_timer_lock);
     count = plt_stamp64 + ((plt_src.read_counter() - plt_stamp) & plt_mask);
     stime = __read_platform_stime(count);

@@ -559,6 +565,8 @@ static void platform_time_calibration(vo
     u64 count;
     s_time_t stamp;
 
+    if ( platform_timer_is_tsc() )
+        return;
     spin_lock(&platform_timer_lock);
     count = plt_stamp64 + ((plt_src.read_counter() - plt_stamp) & plt_mask);
     stamp = __read_platform_stime(count);

@@ -569,6 +577,8 @@ static void platform_time_calibration(vo
 
 static void resume_platform_timer(void)
 {
+    if ( platform_timer_is_tsc() )
+        return;
     /* No change in platform_stime across suspend/resume. */
     platform_timer_stamp = plt_stamp64;
     plt_stamp = plt_src.read_counter();

@@ -604,16 +614,20 @@ static void init_platform_timer(void)
          !init_pmtimer(pts) )
         init_pit(pts);
 
-    plt_mask = (u64)~0ull >> (64 - pts->counter_bits);
-
     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 ( !platform_timer_is_tsc() )
+    {
+        plt_mask = (u32)~0u >> (32 - pts->counter_bits);
 
-    platform_timer_stamp = plt_stamp64;
+        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;
+    }
+
 
     printk("Platform timer is %s %s\n",
            freq_string(pts->frequency), pts->name);

@@ -633,9 +647,10 @@ void cstate_restore_tsc(void)
 void cstate_restore_tsc(void)
 {
     struct cpu_time *t = &this_cpu(cpu_time);
-    u64 plt_count_delta, tsc_delta;
+    u32    plt_count_delta;
+    u64    tsc_delta;
 
-    if ( tsc_invariant )
+    if ( tsc_invariant || platform_timer_is_tsc() )
         return;
 
     plt_count_delta = (plt_src.read_counter() -

@@ -736,14 +751,18 @@ 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;
 }

@@ -1008,12 +1027,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;

@@ -1031,10 +1047,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*/

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Dan Magenheimer

author

2008-07-16 04:11 UTC #12 lore raw

OK, ignore that. It looks like you have it all fixed in 18060. I tried it and it now boots. Thanks!

Quoted 64 lines

-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 7:16 PM
To: '[email protected]'; 'Keir Fraser'; 'Xen-Devel (E-mail)'
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Returning to 32-bit read_counter(), and having NULL
read_counter when
Quoted 1 line
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
Quoted 39 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 10:04 AM
To: [email protected]; 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.
Quoted 21 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 9:46 AM
To: 'Keir Fraser'; 'Xen-Devel (E-mail)'
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 5 lines
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
Quoted 1 line
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

Dan Magenheimer

author

2008-07-16 12:43 UTC #13 lore raw

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")

Quoted 76 lines

-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 10:12 PM
To: '[email protected]'; '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!
Quoted 4 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 7:16 PM
To: '[email protected]'; 'Keir Fraser'; 'Xen-Devel
(E-mail)'
Quoted 60 lines
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Returning to 32-bit read_counter(), and having NULL
read_counter when
Quoted 1 line
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
Quoted 39 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 10:04 AM
To: [email protected]; 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.
Quoted 21 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 9:46 AM
To: 'Keir Fraser'; 'Xen-Devel (E-mail)'
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 5 lines
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
Quoted 1 line
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

Keir Fraser

2008-07-16 12:49 UTC #14 lore raw

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

Quoted 92 lines · Dan Magenheimer

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")
Quoted 76 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 10:12 PM
To: '[email protected]'; '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!
Quoted 4 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 7:16 PM
To: '[email protected]'; 'Keir Fraser'; 'Xen-Devel
(E-mail)'
Quoted 60 lines
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Returning to 32-bit read_counter(), and having NULL
read_counter when
Quoted 1 line
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
Quoted 39 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 10:04 AM
To: [email protected]; 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.
Quoted 21 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 9:46 AM
To: 'Keir Fraser'; 'Xen-Devel (E-mail)'
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 5 lines
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
Quoted 1 line
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

Dan Magenheimer

author

2008-07-16 13:43 UTC #15 lore raw

I'm thinking the problem may only occur when dom0 is running ntpd. Maybe "setting" the platform time only changes one CPU, or sets the platform time differently than system time?

Quoted 116 lines

-----Original Message-----
From: Keir Fraser [mailto:[email protected]]
Sent: Wednesday, July 16, 2008 6:50 AM
To: [email protected]; 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
Quoted 7 lines · Dan Magenheimer
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.
Quoted 12 lines
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")
Quoted 4 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 10:12 PM
To: '[email protected]'; 'Keir Fraser';
'Xen-Devel (E-mail)'
Quoted 73 lines
Quoted 72 lines
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!
Quoted 4 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 7:16 PM
To: '[email protected]'; 'Keir Fraser'; 'Xen-Devel
(E-mail)'
Quoted 60 lines
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Returning to 32-bit read_counter(), and having NULL
read_counter when
Quoted 1 line
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
Quoted 39 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 10:04 AM
To: [email protected]; 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.
Quoted 21 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 9:46 AM
To: 'Keir Fraser'; 'Xen-Devel (E-mail)'
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 5 lines
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
Quoted 1 line
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

Dan Magenheimer

author

2008-07-16 15:42 UTC #16 lore raw

Nope, its not ntpd. Everything seemed fine for at least 15-20 minutes, then I went away for awhile and when I checked back an hour later, same weird symptoms as before.

I also tried running with nosmp on the xen boot line (and confirmed that only one processor was started). Same results... after about an hour. So it doesn't appear to be a result of cross-CPU time issues.

Quoted 42 lines

From: Dan Magenheimer [mailto:[email protected]]
I'm thinking the problem may only occur when dom0 is running ntpd. Maybe "setting" the platform time only changes one CPU, or sets the platform time differently than system time?
Quoted 9 lines
-----Original Message-----
From: Keir Fraser [mailto:[email protected]]
Sent: Wednesday, July 16, 2008 6:50 AM
To: [email protected]; 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
Quoted 25 lines
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
Quoted 14 lines · Dan Magenheimer
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")

Keir Fraser

2008-07-16 19:32 UTC #17 lore raw

My best guess is still that Xen's calculation of time is diverging from dom0 kernel's. For some reason that is unclear, since of course the calibration info should be statically determined at boot and never change from there on.

-- Keir

Quoted 54 lines · Dan Magenheimer

Nope, its not ntpd. Everything seemed fine for at least 15-20 minutes, then I went away for awhile and when I checked back an hour later, same weird symptoms as before.
I also tried running with nosmp on the xen boot line (and confirmed that only one processor was started). Same results... after about an hour. So it doesn't appear to be a result of cross-CPU time issues.
Quoted 42 lines
From: Dan Magenheimer [mailto:[email protected]]
I'm thinking the problem may only occur when dom0 is running ntpd. Maybe "setting" the platform time only changes one CPU, or sets the platform time differently than system time?
Quoted 9 lines
-----Original Message-----
From: Keir Fraser [mailto:[email protected]]
Sent: Wednesday, July 16, 2008 6:50 AM
To: [email protected]; 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
Quoted 25 lines
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
Quoted 14 lines · Dan Magenheimer
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")

Dan Magenheimer

author

2008-07-17 23:05 UTC #18 lore raw

In reply to Keir Fraser

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

Quoted 35 lines

-----Original Message-----
From: Keir Fraser [mailto:[email protected]]
Sent: Wednesday, July 16, 2008 6:50 AM
To: [email protected]; 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
Quoted 15 lines · Dan Magenheimer
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")

Quoted 81 lines

Quoted 4 lines
Quoted 4 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 10:12 PM
To: '[email protected]'; 'Keir Fraser';
'Xen-Devel (E-mail)'
Quoted 73 lines
Quoted 72 lines
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!
Quoted 4 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 7:16 PM
To: '[email protected]'; 'Keir Fraser'; 'Xen-Devel
(E-mail)'
Quoted 60 lines
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Returning to 32-bit read_counter(), and having NULL
read_counter when
Quoted 1 line
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
Quoted 39 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 10:04 AM
To: [email protected]; 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.
Quoted 21 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 9:46 AM
To: 'Keir Fraser'; 'Xen-Devel (E-mail)'
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 5 lines
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
Quoted 1 line
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

xen/arch/x86/time.c +35 ~~-39~~

diff -r ea6a9793928d xen/arch/x86/time.c
@@ -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();

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Keir Fraser

2008-07-18 07:24 UTC #19 lore raw

You shouldn't have to modify get_s_time(). Guests will still be running the old algorithm (sys_stamp + (now_tsc - stamp_tsc) * scale_tsc), so existing get_s_time() ought to work. In fact it must work.

-- Keir

Quoted 127 lines · Dan Magenheimer

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
Quoted 116 lines
-----Original Message-----
From: Keir Fraser [mailto:[email protected]]
Sent: Wednesday, July 16, 2008 6:50 AM
To: [email protected]; 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
Quoted 7 lines · Dan Magenheimer
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.
Quoted 12 lines
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")
Quoted 4 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 10:12 PM
To: '[email protected]'; 'Keir Fraser';
'Xen-Devel (E-mail)'
Quoted 73 lines
Quoted 72 lines
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!
Quoted 4 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 7:16 PM
To: '[email protected]'; 'Keir Fraser'; 'Xen-Devel
(E-mail)'
Quoted 60 lines
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Returning to 32-bit read_counter(), and having NULL
read_counter when
Quoted 1 line
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
Quoted 39 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 10:04 AM
To: [email protected]; 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.
Quoted 21 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 9:46 AM
To: 'Keir Fraser'; 'Xen-Devel (E-mail)'
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 5 lines
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
Quoted 1 line
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

Keir Fraser

2008-07-18 11:01 UTC #20 lore raw

Okay, I'm pretty sure the reason your patch works is that system time as calculated in the guest is still being continually calibrated by local_time_calibration() in Xen. You do not turn off that calibration function and hence it will continually update the cpu_time structure with new timestamps and scale factors. Is this really intended?

The reason mine doesn't work is that the time record exposed to the guest *never* changes. The calculation in the guest is thus effectively:
sys_time = TSC * scale_factor

The problem is that this is sensitive to small errors in the scale factor! And unfortunately we compute a new scale factor, to convert to us, as scale_factor_ns/1000. The resulting scale factor is obviously less accurate, and leads to an accumulating absolute error as the TSC value gets large.

So, when all is said and done, what are we actually trying to achieve here and how should it really work? As far as I can see we cannot avoid sending an updated time record to guests periodically. But running local_time_calibration() only for guests and not for Xen system time seems pretty weird to me.

Perhaps we should push this all off until after 3.3?

-- Keir

Quoted 142 lines · Keir Fraser

You shouldn't have to modify get_s_time(). Guests will still be running the old algorithm (sys_stamp + (now_tsc - stamp_tsc) * scale_tsc), so existing get_s_time() ought to work. In fact it must work.
-- Keir
Quoted 6 lines · Dan Magenheimer
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).
Quoted 121 lines
It may not be elegant but it works and tip doesn't (with clocksource=tsc).
Dan
Quoted 116 lines
-----Original Message-----
From: Keir Fraser [mailto:[email protected]]
Sent: Wednesday, July 16, 2008 6:50 AM
To: [email protected]; 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
Quoted 7 lines · Dan Magenheimer
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.
Quoted 12 lines
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")
Quoted 4 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 10:12 PM
To: '[email protected]'; 'Keir Fraser';
'Xen-Devel (E-mail)'
Quoted 73 lines
Quoted 72 lines
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!
Quoted 4 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 7:16 PM
To: '[email protected]'; 'Keir Fraser'; 'Xen-Devel
(E-mail)'
Quoted 60 lines
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Returning to 32-bit read_counter(), and having NULL
read_counter when
Quoted 1 line
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
Quoted 39 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 10:04 AM
To: [email protected]; 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.
Quoted 21 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 9:46 AM
To: 'Keir Fraser'; 'Xen-Devel (E-mail)'
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 5 lines
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
Quoted 1 line
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
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Keir Fraser

2008-07-18 11:10 UTC #21 lore raw

Ah, I changed my mind. I now think it's due to overflow of a 32-bit usecs variable in Linux's do_gettimeofday(). Cause is still the same though -- the guest assuming that the timestamp record from Xen will never be 'very' stale (in this case, older than 1h12m).

-- Keir

Quoted 171 lines · Keir Fraser

Okay, I'm pretty sure the reason your patch works is that system time as calculated in the guest is still being continually calibrated by local_time_calibration() in Xen. You do not turn off that calibration function and hence it will continually update the cpu_time structure with new timestamps and scale factors. Is this really intended?
The reason mine doesn't work is that the time record exposed to the guest *never* changes. The calculation in the guest is thus effectively:
sys_time = TSC * scale_factor
The problem is that this is sensitive to small errors in the scale factor! And unfortunately we compute a new scale factor, to convert to us, as scale_factor_ns/1000. The resulting scale factor is obviously less accurate, and leads to an accumulating absolute error as the TSC value gets large.
So, when all is said and done, what are we actually trying to achieve here and how should it really work? As far as I can see we cannot avoid sending an updated time record to guests periodically. But running local_time_calibration() only for guests and not for Xen system time seems pretty weird to me.
Perhaps we should push this all off until after 3.3?
-- Keir
Quoted 142 lines · Keir Fraser
You shouldn't have to modify get_s_time(). Guests will still be running the old algorithm (sys_stamp + (now_tsc - stamp_tsc) * scale_tsc), so existing get_s_time() ought to work. In fact it must work.
-- Keir
Quoted 6 lines · Dan Magenheimer
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).
Quoted 121 lines
It may not be elegant but it works and tip doesn't (with clocksource=tsc).
Dan
Quoted 116 lines
-----Original Message-----
From: Keir Fraser [mailto:[email protected]]
Sent: Wednesday, July 16, 2008 6:50 AM
To: [email protected]; 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
Quoted 7 lines · Dan Magenheimer
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.
Quoted 12 lines
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")
Quoted 4 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 10:12 PM
To: '[email protected]'; 'Keir Fraser';
'Xen-Devel (E-mail)'
Quoted 73 lines
Quoted 72 lines
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!
Quoted 4 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 7:16 PM
To: '[email protected]'; 'Keir Fraser'; 'Xen-Devel
(E-mail)'
Quoted 60 lines
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Returning to 32-bit read_counter(), and having NULL
read_counter when
Quoted 1 line
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
Quoted 39 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 10:04 AM
To: [email protected]; 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.
Quoted 21 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 9:46 AM
To: 'Keir Fraser'; 'Xen-Devel (E-mail)'
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 5 lines
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
Quoted 1 line
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
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Dan Magenheimer

author

2008-07-18 14:19 UTC #22 lore raw

OK, I see. So:

If we have a 64-bit precise, monotonic, relatively constant rate (i.e. "ideal") timesource, Xen system time should use it directly. Call this the "new" algorithm. If we do NOT have such a timesource, Xen system time MUST use the (sys_stamp+(now_tsc-stamp_tsc)*scale_tsc) method... the "old" algorithm.

Unfortunately, the old algorithm is hard-baked into PV domains. But, to maximize precision in HV domains, we would like to use the new algorithm whenever there is an ideal timesource. In fact, there's no reason why Xen itself shouldn't use an ideal timesource if available.

So perhaps the compromise which I've (admittedly accidentally) crafted is the right answer for now. And the right answer for later is to update the PV time mechanisms (in a backwards compatible way of course) to determine if an ideal timesource is available and use it if it is.

Quoted 213 lines

-----Original Message-----
From: Keir Fraser [mailto:[email protected]]
Sent: Friday, July 18, 2008 5:10 AM
To: Keir Fraser; [email protected]; Xen-Devel (E-mail)
Cc: Dave Winchell
Subject: Re: [Xen-devel] Re: [PATCH] clocksource=tsc
Ah, I changed my mind. I now think it's due to overflow of a 32-bit usecs
variable in Linux's do_gettimeofday(). Cause is still the same though -- the
guest assuming that the timestamp record from Xen will never be 'very' stale
(in this case, older than 1h12m).
-- Keir
Quoted 1 line · Keir Fraser
Okay, I'm pretty sure the reason your patch works is that
system time as
Quoted 2 lines
calculated in the guest is still being continually calibrated by local_time_calibration() in Xen. You do not turn off that
calibration
Quoted 1 line
function and hence it will continually update the cpu_time
structure with
Quoted 3 lines
new timestamps and scale factors. Is this really intended?
The reason mine doesn't work is that the time record
exposed to the guest
Quoted 4 lines
*never* changes. The calculation in the guest is thus effectively:
sys_time = TSC * scale_factor
The problem is that this is sensitive to small errors in
the scale factor!
Quoted 1 line
And unfortunately we compute a new scale factor, to convert
to us, as
Quoted 1 line
scale_factor_ns/1000. The resulting scale factor is
obviously less accurate,
Quoted 1 line
and leads to an accumulating absolute error as the TSC
value gets large.
Quoted 2 lines
So, when all is said and done, what are we actually trying
to achieve here
Quoted 1 line
and how should it really work? As far as I can see we
cannot avoid sending
Quoted 2 lines
an updated time record to guests periodically. But running local_time_calibration() only for guests and not for Xen
system time seems
Quoted 9 lines
pretty weird to me.
Perhaps we should push this all off until after 3.3?
-- Keir
Quoted 1 line · Keir Fraser
You shouldn't have to modify get_s_time(). Guests will
still be running the
Quoted 1 line
Quoted 1 line
old algorithm (sys_stamp + (now_tsc - stamp_tsc) *
scale_tsc), so existing
Quoted 5 lines
Quoted 5 lines
get_s_time() ought to work. In fact it must work.
-- Keir
On 18/7/08 00:05, "Dan Magenheimer"
Quoted 20 lines · <[email protected]> wrote:
Quoted 20 lines
Quoted 20 lines
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
Quoted 8 lines
-----Original Message-----
From: Keir Fraser [mailto:[email protected]]
Sent: Wednesday, July 16, 2008 6:50 AM
To: [email protected]; Xen-Devel (E-mail)
Cc: Dave Winchell
Subject: Re: [PATCH] clocksource=tsc
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
That's a weird set of symptoms. Perhaps dom0's sense of
system time is
Quoted 47 lines
Quoted 47 lines
Quoted 47 lines
Quoted 47 lines
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
Quoted 7 lines · Dan Magenheimer
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.
Quoted 12 lines
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")
Quoted 4 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 10:12 PM
To: '[email protected]'; 'Keir Fraser';
'Xen-Devel (E-mail)'
Quoted 16 lines
Quoted 16 lines
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!
Quoted 4 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 7:16 PM
To: '[email protected]'; 'Keir Fraser'; 'Xen-Devel
(E-mail)'
Quoted 4 lines
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Returning to 32-bit read_counter(), and having NULL
read_counter when
Quoted 1 line
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
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
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.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Thanks,
Dan
Quoted 4 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 10:04 AM
To: [email protected]; Keir Fraser;
Xen-Devel (E-mail)
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
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.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
-----Original Message-----
From: Dan Magenheimer [mailto:[email protected]]
Sent: Tuesday, July 15, 2008 9:46 AM
To: 'Keir Fraser'; 'Xen-Devel (E-mail)'
Cc: 'Dave Winchell'
Subject: RE: [PATCH] clocksource=tsc
Quoted 1 line
Actually in this mode of operation we hardly need
a platform
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
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
Quoted 1 line
clocksource=tsc would be another possibility...
That's essentially what the original
tscstable.patch did, though
Quoted 20 lines
Quoted 18 lines
Quoted 11 lines
Quoted 11 lines
Quoted 8 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
I was perhaps much uglier in the miscellaneous parts.
Thanks,
Dan
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Keir Fraser

2008-07-18 14:29 UTC #23 lore raw

Quoted 5 lines · Dan Magenheimer

So perhaps the compromise which I've (admittedly accidentally) crafted is the right answer for now. And the right answer for later is to update the PV time mechanisms (in a backwards compatible way of course) to determine if an ideal timesource is available and use it if it is.

Well, maybe, although I don't see we have any guarantee that 'platform system time' and Xen's get_s_time won't diverge in absolute terms as time passes. The scale factors each use are calculated somewhat independently. Actually I think it may get it right just now, but it looks rather fragile and it stores up trouble for systems with long uptimes if you get it wrong. There's no particular reason not to generate fresh time records every few seconds and use those both in Xen and in guests, using the existing compute-system-time algorithm.

-- Keir

Dan Magenheimer

author

2008-07-18 14:56 UTC #24 lore raw

Quoted 6 lines

Quoted 5 lines
So perhaps the compromise which I've (admittedly accidentally) crafted is the right answer for now. And the right answer for later is to update the PV time mechanisms (in a backwards compatible way of course) to determine if an ideal timesource is available and use it if it is.

Quoted 13 lines

Well, maybe, although I don't see we have any guarantee that 'platform system time' and Xen's get_s_time won't diverge in absolute terms as time
passes. The scale factors each use are calculated somewhat independently.
Actually I think it may get it right just now, but it looks rather fragile
and it stores up trouble for systems with long uptimes if you get it wrong.
There's no particular reason not to generate fresh time records every few
seconds and use those both in Xen and in guests, using the existing compute-system-time algorithm.

It appears that, for clocksource=tsc, as long as both read_platform_stime() and get_s_time() are returning scaled-tsc there can be no divergence.

The issue with generating fresh time records every few seconds is that it unnecessarily introduces jitter into an otherwise ideal timesource.

Dan

Keir Fraser

2008-07-18 15:00 UTC #25 lore raw

Quoted 7 lines · Dan Magenheimer

It appears that, for clocksource=tsc, as long as both read_platform_stime() and get_s_time() are returning scaled-tsc there can be no divergence.
The issue with generating fresh time records every few seconds is that it unnecessarily introduces jitter into an otherwise ideal timesource.

I don't think this is necessarily true. If we write code to generate accurate time records specifically for clocksource=tsc then we should easily get accuracy down to a couple of parts per billion. This is certainly a more pragmatic solution than extending the guest time interfaces. I am at least coming round to the fact that the changes required in Xen's time.c are going to have to be a bit more drastic than I first hoped.

-- Keir

Dan Magenheimer

author

2008-07-18 16:51 UTC #26 lore raw

Quoted 11 lines

I don't think this is necessarily true. If we write code to generate accurate time records specifically for clocksource=tsc then we should easily
get accuracy down to a couple of parts per billion. This is certainly a more
pragmatic solution than extending the guest time interfaces. I am at least
coming round to the fact that the changes required in Xen's time.c are going
to have to be a bit more drastic than I first hoped.

Quoted 1 line

-- Keir

I guess you are correct as long as "generate a time record" doesn't include recomputing the scaling factor every second (which is I think what introduces the jitter... see more on this below).

However, I'm not sure why you perceive the aesthetics to be so bad to put "if (ideal_clocksource)" in get_s_time() and a few other places in time.c... or for that matter in the PV guest code.

Your existing algorithm is a very cool and elegant way to optimize time handling for all "legacy" time sources. The more I understand about it, the more I am impressed! But it is very difficult to understand and for most "modern" hardware it unnecessarily obfuscates time handling. So IMHO I much prefer the "if (ideal_clocksource)... else ..." approach rather than shoehorning the ideal to look like the legacy. And that applies for the PV guest code as well.

On a related note:

Quoted 7 lines

The problem is that this is sensitive to small errors in the scale factor!
And unfortunately we compute a new scale factor, to convert to us, as scale_factor_ns/1000. The resulting scale factor is obviously less accurate,
and leads to an accumulating absolute error as the TSC value gets large.

Yes I noticed that the mul_frac+scale_delta code gives different answers than muldiv64, though they have essentially the same API. I didn't do much testing but they differed after about 5 digits and muldiv64 does a hardware divide so I suspect it is more accurate (though also probably slower). It might be worth looking at this to squeeze out more precision.

Thanks for all your time and attention on this. As Xen supports more time-sensitive workloads (such as databases on enterprise systems and games on PCs), I think it will pay off!

Dan

Keir Fraser

2008-07-18 19:28 UTC #27 lore raw

Quoted 9 lines · Dan Magenheimer

I guess you are correct as long as "generate a time record" doesn't include recomputing the scaling factor every second (which is I think what introduces the jitter... see more on this below).
However, I'm not sure why you perceive the aesthetics to be so bad to put "if (ideal_clocksource)" in get_s_time() and a few other places in time.c... or for that matter in the PV guest code.

Extra special case are ugly. And then if they extend into the guests then you also have to worry about new guests on old Xen and old guests on new Xen (i.e., compatibility both ways). Which is not a good way to go when there is an easy way to hide this new implementation from guests. Indeed we would only update the TSC and system-time stamps -- the scale factor would never change.

-- Keir