Re: [PATCH] x86: unconditionally mark TSC unstable under Xen

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 07/14/2010 12:24 PM, Jed Smith wrote:
> Jeremy, Jan - what do you think?  Is this a bad move?  I feel like there
> is a consequence to this that I am unaware of, but it fixes my issue.
>   

Ah, well that's interesting.

There's a couple of comments:

   1. you can't do this with just a compile-time test, since the same
      kernel can also boot native
   2. nothing in a Xen PV domU environment should be using the tsc
      directly, so this shouldn't have an effect.  If something is using
      the tsc we should track it down.

I wonder, however, if you're getting the same result as Jan's suggestion
of making sched_clock unstable by making the tsc unstable.

In that case, this patch may help:

Subject: [PATCH] xen: disable xen_sched_clock by default

xen_sched_clock only counts unstolen time.  In principle this should
be useful to the Linux scheduler so that it knows how much time a process
actually consumed.  But in practice this doesn't work very well as the
scheduler expects the sched_clock time to be synchronized between
cpus.  It also uses sched_clock to measure the time a task spends
sleeping, in which case "unstolen time" isn't meaningful.

So just use plain xen_clocksource_read to return wallclock nanoseconds
for sched_clock.

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
index b83e119..6a09f01 100644
--- a/arch/x86/xen/Kconfig
+++ b/arch/x86/xen/Kconfig
@@ -29,6 +29,10 @@ config XEN_SAVE_RESTORE
        depends on XEN && PM
        default y
 
+config XEN_SCHED_CLOCK
+       bool
+       default n
+
 config XEN_DEBUG_FS
 	bool "Enable Xen debug and tuning parameters in debugfs"
 	depends on XEN && DEBUG_FS
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 4b57c0b..242a230 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -939,7 +939,11 @@ static const struct pv_time_ops xen_time_ops __initdata = {
 	.set_wallclock = xen_set_wallclock,
 	.get_wallclock = xen_get_wallclock,
 	.get_tsc_khz = xen_tsc_khz,
+#ifdef CONFIG_XEN_SCHED_CLOCK
 	.sched_clock = xen_sched_clock,
+#else
+	.sched_clock = xen_clocksource_read,
+#endif
 };
 
 static const struct pv_cpu_ops xen_cpu_ops __initdata = {
diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index 9d1f853..32dc3dc 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -154,6 +154,7 @@ static void do_stolen_accounting(void)
 	account_idle_ticks(ticks);
 }
 
+#ifdef CONFIG_XEN_SCHED_CLOCK
 /*
  * Xen sched_clock implementation.  Returns the number of unstolen
  * nanoseconds, which is nanoseconds the VCPU spent in RUNNING+BLOCKED
@@ -191,7 +192,7 @@ unsigned long long xen_sched_clock(void)
 
 	return ret;
 }
-
+#endif
 
 /* Get the TSC speed from Xen */
 unsigned long xen_tsc_khz(void)

Thanks,
	J