Sched clock paravirt op fix.patch

PATCH In review Zachary Amsden 6 participants 9 messages Mar 2, 2007 – Mar 16, 2007

Subsystems (2)

virtio core PARAVIRT_OPS interface

lore

Share & tools

lore

Zachary Amsden

author

2007-03-02 02:54 UTC #0 lore raw

PATCH 2/9 Sched clock paravirt op fix.patch

The custom_sched_clock hook is broken. The result from sched_clock needs to be in nanoseconds, not in CPU cycles. The TSC is insufficient for this purpose, because TSC is poorly defined in a virtual environment, and mostly represents real world time instead of scheduled process time (which can be interrupted without notice when a virtual machine is descheduled).

To make the scheduler consistent, we must expose a different nature of time, that is scheduled time. So deprecate this custom_sched_clock hack and turn it into a paravirt-op, as it should have been all along. This allows the tsc.c code which converts cycles to nanoseconds to be shared by all paravirt-ops backends.

It is unfortunate to add a new paravirt-op, but this is a very distinct abstraction which is clearly different for all virtual machine implementations, and it gets rid of an ugly indirect function which I ashamedly admit I hacked in to try to get this to work earlier, and then even got in the wrong units.

Please apply.

Signed-off-by Zachary Amsden <[email protected]>

arch/i386/kernel/paravirt.c +2 -0

diff -r d58e6ddfdfa9 arch/i386/kernel/paravirt.c
@@ -32,6 +32,7 @@
 #include <asm/fixmap.h>
 #include <asm/apic.h>
 #include <asm/tlbflush.h>
+#include <asm/timer.h>
 
 /* nop stub */
 static void native_nop(void)

@@ -520,6 +521,7 @@ struct paravirt_ops paravirt_ops = {
 	.write_msr = native_write_msr,
 	.read_tsc = native_read_tsc,
 	.read_pmc = native_read_pmc,
+	.get_scheduled_cycles = native_read_tsc,
 	.load_tr_desc = native_load_tr_desc,
 	.set_ldt = native_set_ldt,
 	.load_gdt = native_load_gdt,

arch/i386/kernel/tsc.c +2 -4

diff -r d58e6ddfdfa9 arch/i386/kernel/tsc.c
@@ -14,6 +14,7 @@
 #include <asm/delay.h>
 #include <asm/tsc.h>
 #include <asm/io.h>
+#include <asm/timer.h>
 
 #include "mach_timer.h"

@@ -108,9 +109,6 @@ unsigned long long sched_clock(void)
 {
 	unsigned long long this_offset;
 
-	if (unlikely(custom_sched_clock))
-		return (*custom_sched_clock)();
-
 	/*
 	 * Fall back to jiffies if there's no TSC available:
 	 */

@@ -119,7 +117,7 @@ unsigned long long sched_clock(void)
 		return (jiffies_64 - INITIAL_JIFFIES) * (1000000000 / HZ);
 
 	/* read the Time Stamp Counter: */
-	rdtscll(this_offset);
+	get_scheduled_cycles(this_offset);
 
 	/* return the value in ns */
 	return cycles_2_ns(this_offset);

arch/i386/kernel/vmi.c +1 -1

diff -r d58e6ddfdfa9 arch/i386/kernel/vmi.c
@@ -873,7 +873,7 @@ static inline int __init activate_vmi(vo
 		paravirt_ops.setup_boot_clock = vmi_timer_setup_boot_alarm;
 		paravirt_ops.setup_secondary_clock = vmi_timer_setup_secondary_alarm;
 #endif
-		custom_sched_clock = vmi_sched_clock;
+		paravirt_ops.get_scheduled_cycles = vmi_get_sched_cycles;
 	}
 	if (!disable_noidle)
 		para_fill(safe_halt, Halt);

arch/i386/kernel/vmitime.c +1 -1

diff -r d58e6ddfdfa9 arch/i386/kernel/vmitime.c
@@ -172,7 +172,7 @@ int vmi_set_wallclock(unsigned long now)
 	return -1;
 }
 
-unsigned long long vmi_sched_clock(void)
+unsigned long long vmi_get_sched_cycles(void)
 {
 	return read_available_cycles();
 }

include/asm-i386/paravirt.h +3 -0

diff -r d58e6ddfdfa9 include/asm-i386/paravirt.h
@@ -94,6 +94,7 @@ struct paravirt_ops
 
 	u64 (*read_tsc)(void);
 	u64 (*read_pmc)(void);
+ 	u64 (*get_scheduled_cycles)(void);
 
 	void (*load_tr_desc)(void);
 	void (*load_gdt)(const struct Xgt_desc_struct *);

@@ -273,6 +274,8 @@ static inline void halt(void)
 
 #define rdtscll(val) (val = paravirt_ops.read_tsc())
 
+#define get_scheduled_cycles(val) (val = paravirt_ops.get_scheduled_cycles())
+
 #define write_tsc(val1,val2) wrmsr(0x10, val1, val2)
 
 #define rdpmc(counter,low,high) do {				\

include/asm-i386/time.h +0 -1

diff -r d58e6ddfdfa9 include/asm-i386/time.h
@@ -30,7 +30,6 @@ static inline int native_set_wallclock(u
 
 #ifdef CONFIG_PARAVIRT
 #include <asm/paravirt.h>
-extern unsigned long long native_sched_clock(void);
 #else /* !CONFIG_PARAVIRT */
 
 #define get_wallclock() native_get_wallclock()

include/asm-i386/timer.h +7 -1

diff -r d58e6ddfdfa9 include/asm-i386/timer.h
@@ -4,13 +4,19 @@
 #include <linux/pm.h>
 
 #define TICK_SIZE (tick_nsec / 1000)
+
 void setup_pit_timer(void);
+unsigned long long native_sched_clock(void);
+
 /* Modifiers for buggy PIT handling */
 extern int pit_latch_buggy;
 extern int timer_ack;
 extern int no_timer_check;
-extern unsigned long long (*custom_sched_clock)(void);
 extern int no_sync_cmos_clock;
 extern int recalibrate_cpu_khz(void);
 
+#ifndef CONFIG_PARAVIRT
+#define get_scheduled_cycles(val) rdtscll(val)
 #endif
+
+#endif

include/asm-i386/vmi_time.h +1 -1

diff -r d58e6ddfdfa9 include/asm-i386/vmi_time.h
@@ -49,7 +49,7 @@ extern void __init vmi_time_init(void);
 extern void __init vmi_time_init(void);
 extern unsigned long vmi_get_wallclock(void);
 extern int vmi_set_wallclock(unsigned long now);
-extern unsigned long long vmi_sched_clock(void);
+extern unsigned long long vmi_get_sched_cycles(void);
 
 #ifdef CONFIG_X86_LOCAL_APIC
 extern void __init vmi_timer_setup_boot_alarm(void);

Andi Kleen

2007-03-13 14:01 UTC #1 lore raw

Quoted 12 lines · Zachary Amsden

The custom_sched_clock hook is broken. The result from sched_clock needs to be in nanoseconds, not in CPU cycles. The TSC is insufficient for this purpose, because TSC is poorly defined in a virtual environment, and mostly represents real world time instead of scheduled process time (which can be interrupted without notice when a virtual machine is descheduled).
To make the scheduler consistent, we must expose a different nature of time, that is scheduled time. So deprecate this custom_sched_clock hack and turn it into a paravirt-op, as it should have been all along. This allows the tsc.c code which converts cycles to nanoseconds to be shared by all paravirt-ops backends.

Quoted 6 lines

It is unfortunate to add a new paravirt-op, but this is a very distinct abstraction which is clearly different for all virtual machine implementations, and it gets rid of an ugly indirect function which I ashamedly admit I hacked in to try to get this to work earlier, and then even got in the wrong units.
Please apply.

I think it's better to remove this completely and not allow paravirt to hook into sched_clock. After all a hypervisor stealing time is no different from interrupts stealing time and we don't try to handle that either.

I will remove the custom hook.

-Andi

Jeremy Fitzhardinge

2007-03-13 15:26 UTC #2 lore raw

Quoted 19 lines · Andi Kleen

Quoted 17 lines · Zachary Amsden
The custom_sched_clock hook is broken. The result from sched_clock needs to be in nanoseconds, not in CPU cycles. The TSC is insufficient for this purpose, because TSC is poorly defined in a virtual environment, and mostly represents real world time instead of scheduled process time (which can be interrupted without notice when a virtual machine is descheduled).
To make the scheduler consistent, we must expose a different nature of time, that is scheduled time. So deprecate this custom_sched_clock hack and turn it into a paravirt-op, as it should have been all along. This allows the tsc.c code which converts cycles to nanoseconds to be shared by all paravirt-ops backends.
It is unfortunate to add a new paravirt-op, but this is a very distinct abstraction which is clearly different for all virtual machine implementations, and it gets rid of an ugly indirect function which I ashamedly admit I hacked in to try to get this to work earlier, and then even got in the wrong units.

Quoted 8 lines

Quoted 2 lines
Please apply.
I think it's better to remove this completely and not allow paravirt to hook into sched_clock. After all a hypervisor stealing time is no different from interrupts stealing time and we don't try to handle that either.

Well, its a matter of degree. If your busy vcpu is sharing a real cpu with 9 other busy vcpus, then you'll only see about 10% of the real cpu cycles. If we had legitimate loads in which 90% of the cpu were taken up by interrupt processing, then I think we definitely would try to account for that time properly.

SMM interrupts, in which the CPU is stolen from the kernel entirely, is a better analogy. But again, they're expected to be rare and insignificant. In a virtual machine, on the other hand, you could expect the majority of cpu time to be stolen by other things running on the same physical machine.

In other words, regardless of whether this particular pv_op lives or dies, we're going to need to have to deal with stolen time properly. I think this hook is reasonable and useful step towards doing that.

Chris Wright

2007-03-13 16:07 UTC #3 lore raw

Quoted 3 lines · * Jeremy Fitzhardinge ([email protected]) wrote:

In other words, regardless of whether this particular pv_op lives or dies, we're going to need to have to deal with stolen time properly. I think this hook is reasonable and useful step towards doing that.

Exactly. Normal interrupts we can handle. Having CPU completely disappear for unkown time periods we can't, and will need to.

thanks,
-chris

Andi Kleen

2007-03-13 16:16 UTC #4 lore raw

Quoted 7 lines · Chris Wright

Quoted 3 lines · * Jeremy Fitzhardinge ([email protected]) wrote:
In other words, regardless of whether this particular pv_op lives or dies, we're going to need to have to deal with stolen time properly. I think this hook is reasonable and useful step towards doing that.
Exactly. Normal interrupts we can handle. Having CPU completely disappear for unkown time periods we can't, and will need to.

But that is just what a interrupt is.

-Andi

Rik van Riel

2007-03-13 16:37 UTC #5 lore raw

Quoted 9 lines · Andi Kleen

Quoted 6 lines · Chris Wright
Quoted 3 lines · * Jeremy Fitzhardinge ([email protected]) wrote:
In other words, regardless of whether this particular pv_op lives or dies, we're going to need to have to deal with stolen time properly. I think this hook is reasonable and useful step towards doing that.
Exactly. Normal interrupts we can handle. Having CPU completely disappear for unkown time periods we can't, and will need to.
But that is just what a interrupt is.

Interrupts tend to be reasonably short though.

Steal time can be several hypervisor/host time slices long.

As an aside, normal interrupts *are* accounted for separately in /proc/stat, so why not steal time too?

Signature · 3 lines

Politics is the struggle between those who want to make their country
the best in the world, and those who believe it already is.  Each group
calls the other unpatriotic.

Andi Kleen

2007-03-13 20:56 UTC #6 lore raw

Quoted 6 lines · Rik van Riel

Quoted 5 lines · Andi Kleen
Quoted 4 lines · Chris Wright
Quoted 3 lines · * Jeremy Fitzhardinge ([email protected]) wrote:
In other words, regardless of whether this particular pv_op lives or dies, we're going to need to have to deal with stolen time properly. I think this hook is reasonable and useful step towards doing that.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
Exactly. Normal interrupts we can handle. Having CPU completely disappear for unkown time periods we can't, and will need to.
But that is just what a interrupt is.
Interrupts tend to be reasonably short though.

It depends -- under heavy network load you can spend a long time just processing interrupts.

-Andi

Jeremy Fitzhardinge

2007-03-13 21:05 UTC #7 lore raw

Quoted 2 lines · Andi Kleen

It depends -- under heavy network load you can spend a long time just processing interrupts.

Well, in that case you probably don't want to charge them to the process which happens to be running at the time.

Matt Mackall

2007-03-16 21:29 UTC #8 lore raw

Quoted 6 lines · Jeremy Fitzhardinge

Quoted 2 lines · Andi Kleen
It depends -- under heavy network load you can spend a long time just processing interrupts.
Well, in that case you probably don't want to charge them to the process which happens to be running at the time.

It's actually a good first-order approximation of the right thing to do, as it will generally correlate with the userspace process servicing that network load.

If not (for instance, with routing loads), then you'd basically expect the charge to get spread around evenly in proportion to an application's CPU usage.

The -rt kernel pushes most of the interrupt work off to threads, which of course follow the same scheduling and accounting rules as everything other thread.

Signature · 1 line

Mathematics is the supreme nostalgia of our time.