[RFC 0/7] KVM steal time implementation

[RFC 0/7] KVM steal time implementation

[Glauber Costa]

Hi,

In this series, I am proposing an steal time implementation
for KVM. It is an RFC, so don't be too harsh =]

There are two parts of it: the guest and host part.

The proposal for the guest part, is to just change the
common time accounting, and try to identify at that spot,
wether or not we should account any steal time. I considered
this idea less cumbersome that trying to cook a clockevents
implementation ourselves, since I see little value in it.
I am, however, pretty open to suggestions.

For the host<->guest communications, I am using a shared
page, in the same way as pvclock. Because of that, I am just
hijacking pvclock structure anyway. There is a 32-bit field
floating by, that gives us enough room for 8 years of steal
time (we use msec resolution).

The main idea is to timestamp our exit and entry through
sched notifiers, and export the value at pvclock updates.
This obviously have some disadvantages: by doing this we
are giving up futures ideas about only updating
this structure once, and even right now, won't work
on pinned-smp (since we don't update pvclock if we
haven't changed cpus.) 

But again, it is just an RFC, and I'd like to feel the
reception of the idea as a whole.

Have a nice review.

Glauber Costa (6):
  change headers preparing for steal time
  measure time out of guest
  change kernel accounting to include steal time
  kvm steal time implementation
  touch softlockup watchdog
  tell guest about steal time feature

Zachary Amsden (1):
  Implement getnsboottime kernel API

 arch/x86/include/asm/kvm_host.h    |    2 +
 arch/x86/include/asm/kvm_para.h    |    1 +
 arch/x86/include/asm/pvclock-abi.h |    4 ++-
 arch/x86/kernel/kvmclock.c         |   38 ++++++++++++++++++++++++++++++++++++
 arch/x86/kvm/x86.c                 |   15 ++++++++++++-
 include/linux/sched.h              |    1 +
 include/linux/time.h               |    1 +
 kernel/sched.c                     |   29 +++++++++++++++++++++++++++
 kernel/time/timekeeping.c          |   27 +++++++++++++++++++++++++
 9 files changed, 115 insertions(+), 3 deletions(-)

[RFC 1/7] Implement getnsboottime kernel API

[Glauber Costa]

From: Zachary Amsden <zamsden@redhat.com>

Add a kernel call to get the number of nanoseconds since boot.  This
is generally useful enough to make it a generic call.

Signed-off-by: Zachary Amsden <zamsden@redhat.com>
---
 include/linux/time.h      |    1 +
 kernel/time/timekeeping.c |   27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/include/linux/time.h b/include/linux/time.h
index ea3559f..5d04108 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -145,6 +145,7 @@ extern void getnstimeofday(struct timespec *tv);
 extern void getrawmonotonic(struct timespec *ts);
 extern void getboottime(struct timespec *ts);
 extern void monotonic_to_bootbased(struct timespec *ts);
+extern s64 getnsboottime(void);
 
 extern struct timespec timespec_trunc(struct timespec t, unsigned gran);
 extern int timekeeping_valid_for_hres(void);
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index caf8d4d..d250f0a 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -285,6 +285,33 @@ void ktime_get_ts(struct timespec *ts)
 }
 EXPORT_SYMBOL_GPL(ktime_get_ts);
 
+
+/**
+ * getnsboottime - get the bootbased clock in nsec format
+ *
+ * The function calculates the bootbased clock from the realtime
+ * clock and the wall_to_monotonic offset and stores the result
+ * in normalized timespec format in the variable pointed to by @ts.
+ */
+s64 getnsboottime(void)
+{
+	unsigned int seq;
+	s64 secs, nsecs;
+
+	WARN_ON(timekeeping_suspended);
+
+	do {
+		seq = read_seqbegin(&xtime_lock);
+		secs = xtime.tv_sec + wall_to_monotonic.tv_sec;
+		secs += total_sleep_time.tv_sec;
+		nsecs = xtime.tv_nsec + wall_to_monotonic.tv_nsec;
+		nsecs += total_sleep_time.tv_nsec + timekeeping_get_ns();
+
+	} while (read_seqretry(&xtime_lock, seq));
+	return nsecs + (secs * NSEC_PER_SEC);
+}
+EXPORT_SYMBOL_GPL(getnsboottime);
+
 /**
  * do_gettimeofday - Returns the time of day in a timeval
  * @tv:		pointer to the timeval to be set
-- 
1.6.2.2

[RFC 2/7] change headers preparing for steal time

[Glauber Costa]

This guest/host common patch prepares infrastructure for
the steal time implementation. Some constants are added,
and a name change happens in pvclock vcpu structure.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 arch/x86/include/asm/kvm_para.h    |    1 +
 arch/x86/include/asm/pvclock-abi.h |    4 +++-
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
index 05eba5e..1759c81 100644
--- a/arch/x86/include/asm/kvm_para.h
+++ b/arch/x86/include/asm/kvm_para.h
@@ -25,6 +25,7 @@
  * in pvclock structure. If no bits are set, all flags are ignored.
  */
 #define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT	24
+#define KVM_FEATURE_CLOCKSOURCE_STEAL_BIT 	25
 
 #define MSR_KVM_WALL_CLOCK  0x11
 #define MSR_KVM_SYSTEM_TIME 0x12
diff --git a/arch/x86/include/asm/pvclock-abi.h b/arch/x86/include/asm/pvclock-abi.h
index 35f2d19..417061b 100644
--- a/arch/x86/include/asm/pvclock-abi.h
+++ b/arch/x86/include/asm/pvclock-abi.h
@@ -24,7 +24,7 @@
 
 struct pvclock_vcpu_time_info {
 	u32   version;
-	u32   pad0;
+	u32   steal_time;
 	u64   tsc_timestamp;
 	u64   system_time;
 	u32   tsc_to_system_mul;
@@ -40,5 +40,7 @@ struct pvclock_wall_clock {
 } __attribute__((__packed__));
 
 #define PVCLOCK_TSC_STABLE_BIT	(1 << 0)
+#define PVCLOCK_STEAL_BIT   (2 << 0)
+
 #endif /* __ASSEMBLY__ */
 #endif /* _ASM_X86_PVCLOCK_ABI_H */
-- 
1.6.2.2

[RFC 3/7] measure time out of guest

[Glauber Costa]

By measuring time between a vcpu_put and a vcpu_load, we can
estimate how much time did the guest stay out of the cpu.
This is exported to the guest at every clock update.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |    2 ++
 arch/x86/kvm/x86.c              |   12 +++++++++++-
 2 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 502e53f..bc28aff 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -364,6 +364,8 @@ struct kvm_vcpu_arch {
 	u64 hv_vapic;
 
 	cpumask_var_t wbinvd_dirty_mask;
+	u64 time_out;
+	u64 last_time_out;
 };
 
 struct kvm_arch {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e7e3b50..680feaa 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -925,8 +925,9 @@ static void kvm_write_guest_time(struct kvm_vcpu *v)
 	vcpu->hv_clock.system_time = ts.tv_nsec +
 				     (NSEC_PER_SEC * (u64)ts.tv_sec) + v->kvm->arch.kvmclock_offset;
 
-	vcpu->hv_clock.flags = 0;
+	vcpu->hv_clock.flags = PVCLOCK_STEAL_BIT;
 
+	vcpu->hv_clock.steal_time = vcpu->time_out / 1000000;
 	/*
 	 * The interface expects us to write an even number signaling that the
 	 * update is finished. Since the guest won't see the intermediate
@@ -1798,6 +1799,8 @@ static bool need_emulate_wbinvd(struct kvm_vcpu *vcpu)
 
 void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 {
+	s64 now;
+
 	/* Address WBINVD may be executed by guest */
 	if (need_emulate_wbinvd(vcpu)) {
 		if (kvm_x86_ops->has_wbinvd_exit())
@@ -1815,12 +1818,19 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 		per_cpu(cpu_tsc_khz, cpu) = khz;
 	}
 	kvm_request_guest_time_update(vcpu);
+
+	now = getnsboottime();
+
+	if (vcpu->arch.last_time_out != 0)
+		vcpu->arch.time_out += now - vcpu->arch.last_time_out;
 }
 
 void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 {
 	kvm_x86_ops->vcpu_put(vcpu);
 	kvm_put_guest_fpu(vcpu);
+
+	vcpu->arch.last_time_out = getnsboottime();
 }
 
 static int is_efer_nx(void)
-- 
1.6.2.2

[RFC 4/7] change kernel accounting to include steal time

[Glauber Costa]

This patch proposes a common steal time implementation. When no
steal time is accounted, we just add a branch to the current
accounting code, that shouldn't add much overhead.

When we do want to register steal time, we proceed as following:
- if we would account user or system time in this tick, and there is
  out-of-cpu time registered, we skip it altogether, and account steal
  time only.
- if we would account user or system time in this tick, and we got the
  cpu for the whole slice, we proceed normaly.
- if we are idle in this tick, we flush out-of-cpu time to give it the
  chance to update whatever last-measure internal variable it may have.

This approach is simple, but proved to work well for my test scenarios.
in a UP guest on UP host, with a cpu-hog in both guest and host shows
~ 50 % steal time. steal time is also accounted proportionally, if
nice values are given to the host cpu-hog.

A cpu-hog in the host with no load in the guest, produces 0 % steal time,
with 100 % idle, as one would expect.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 include/linux/sched.h |    1 +
 kernel/sched.c        |   29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 0478888..e571ddd 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -312,6 +312,7 @@ long io_schedule_timeout(long timeout);
 extern void cpu_init (void);
 extern void trap_init(void);
 extern void update_process_times(int user);
+extern cputime_t (*hypervisor_steal_time)(void);
 extern void scheduler_tick(void);
 
 extern void sched_show_task(struct task_struct *p);
diff --git a/kernel/sched.c b/kernel/sched.c
index f52a880..9695c92 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -3157,6 +3157,16 @@ unsigned long long thread_group_sched_runtime(struct task_struct *p)
 	return ns;
 }
 
+cputime_t (*hypervisor_steal_time)(void) = NULL;
+
+static inline cputime_t get_steal_time_from_hypervisor(void)
+{
+	if (!hypervisor_steal_time)
+		return 0;
+	return hypervisor_steal_time();
+}
+
+
 /*
  * Account user cpu time to a process.
  * @p: the process that the cpu time gets accounted to
@@ -3169,6 +3179,12 @@ void account_user_time(struct task_struct *p, cputime_t cputime,
 	struct cpu_usage_stat *cpustat = &kstat_this_cpu.cpustat;
 	cputime64_t tmp;
 
+	tmp = get_steal_time_from_hypervisor();
+	if (tmp) {
+		account_steal_time(tmp);
+		return;
+	}
+
 	/* Add user time to process. */
 	p->utime = cputime_add(p->utime, cputime);
 	p->utimescaled = cputime_add(p->utimescaled, cputime_scaled);
@@ -3234,6 +3250,12 @@ void account_system_time(struct task_struct *p, int hardirq_offset,
 		return;
 	}
 
+	tmp = get_steal_time_from_hypervisor();
+	if (tmp) {
+		account_steal_time(tmp);
+		return;
+	}
+
 	/* Add system time to process. */
 	p->stime = cputime_add(p->stime, cputime);
 	p->stimescaled = cputime_add(p->stimescaled, cputime_scaled);
@@ -3276,6 +3298,13 @@ void account_idle_time(cputime_t cputime)
 	cputime64_t cputime64 = cputime_to_cputime64(cputime);
 	struct rq *rq = this_rq();
 
+	/*
+	 * if we're idle, we don't account it as steal time, since we did
+	 * not want to run anyway. We do call the steal function, however, to
+	 * give the guest the chance to flush its internal buffers
+	 */
+	get_steal_time_from_hypervisor();
+
 	if (atomic_read(&rq->nr_iowait) > 0)
 		cpustat->iowait = cputime64_add(cpustat->iowait, cputime64);
 	else
-- 
1.6.2.2

[RFC 5/7] kvm steal time implementation

[Glauber Costa]

This is the proposed kvm-side steal time implementation.
It is migration safe, as it checks flags at every read.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 arch/x86/kernel/kvmclock.c |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index eb9b76c..a1f4852 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -18,6 +18,8 @@
 
 #include <linux/clocksource.h>
 #include <linux/kvm_para.h>
+#include <linux/kernel_stat.h>
+#include <linux/sched.h>
 #include <asm/pvclock.h>
 #include <asm/msr.h>
 #include <asm/apic.h>
@@ -41,6 +43,7 @@ early_param("no-kvmclock", parse_no_kvmclock);
 
 /* The hypervisor will put information about time periodically here */
 static DEFINE_PER_CPU_SHARED_ALIGNED(struct pvclock_vcpu_time_info, hv_clock);
+static DEFINE_PER_CPU(u64, steal_info);
 static struct pvclock_wall_clock wall_clock;
 
 /*
@@ -82,6 +85,32 @@ static cycle_t kvm_clock_read(void)
 	return ret;
 }
 
+static DEFINE_PER_CPU(u64, steal_info);
+
+cputime_t kvm_get_steal_time(void)
+{
+	u64 delta = 0;
+	u64 *last_steal_info, this_steal_info;
+	struct pvclock_vcpu_time_info *src;
+
+	src = &get_cpu_var(hv_clock);
+	if (!(src->flags & PVCLOCK_STEAL_BIT))
+		goto out;
+
+	this_steal_info = src->steal_time;
+	put_cpu_var(hv_clock);
+
+	last_steal_info = &get_cpu_var(steal_info);
+
+	delta = this_steal_info - *last_steal_info;
+
+	*last_steal_info = this_steal_info;
+	put_cpu_var(steal_info);
+
+out:
+	return msecs_to_cputime(delta);
+}
+
 static cycle_t kvm_clock_get_cycles(struct clocksource *cs)
 {
 	return kvm_clock_read();
@@ -134,6 +163,8 @@ static int kvm_register_clock(char *txt)
 	printk(KERN_INFO "kvm-clock: cpu %d, msr %x:%x, %s\n",
 	       cpu, high, low, txt);
 
+	per_cpu(steal_info, cpu) = 0;
+
 	return native_write_msr_safe(msr_kvm_system_time, low, high);
 }
 
@@ -218,4 +249,8 @@ void __init kvmclock_init(void)
 
 	if (kvm_para_has_feature(KVM_FEATURE_CLOCKSOURCE_STABLE_BIT))
 		pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
+
+	
+	if (kvm_para_has_feature(KVM_FEATURE_CLOCKSOURCE_STEAL_BIT))
+		hypervisor_steal_time = kvm_get_steal_time;
 }
-- 
1.6.2.2

[RFC 6/7] touch softlockup watchdog

[Glauber Costa]

With a reliable steal time mechanism, we can tell if we're
out of the cpu for very long, differentiating from the case
that we simply got a real softlockup.

In the case we were out of cpu, the watchdog is fed, making
bogus softlockups disappear.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 arch/x86/kernel/kvmclock.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index a1f4852..1c496c8 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -104,6 +104,9 @@ cputime_t kvm_get_steal_time(void)
 
 	delta = this_steal_info - *last_steal_info;
 
+	if (delta > 1000UL)
+		touch_softlockup_watchdog();
+
 	*last_steal_info = this_steal_info;
 	put_cpu_var(steal_info);
 
-- 
1.6.2.2

[RFC 7/7] tell guest about steal time feature

[Glauber Costa]

Guest kernel will only activate steal time if the host exports it.
Warn the guest about it.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 arch/x86/kvm/x86.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 680feaa..9a20cd8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2101,7 +2101,8 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		entry->eax = (1 << KVM_FEATURE_CLOCKSOURCE) |
 			     (1 << KVM_FEATURE_NOP_IO_DELAY) |
 			     (1 << KVM_FEATURE_CLOCKSOURCE2) |
-			     (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
+			     (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT) |
+			     (1 << KVM_FEATURE_CLOCKSOURCE_STEAL_BIT);
 		entry->ebx = 0;
 		entry->ecx = 0;
 		entry->edx = 0;
-- 
1.6.2.2

Re: [RFC 4/7] change kernel accounting to include steal time

[Marcelo Tosatti]

On Wed, Aug 25, 2010 at 05:43:14PM -0400, Glauber Costa wrote:
> This patch proposes a common steal time implementation. When no
> steal time is accounted, we just add a branch to the current
> accounting code, that shouldn't add much overhead.
> 
> When we do want to register steal time, we proceed as following:
> - if we would account user or system time in this tick, and there is
>   out-of-cpu time registered, we skip it altogether, and account steal
>   time only.
> - if we would account user or system time in this tick, and we got the
>   cpu for the whole slice, we proceed normaly.
> - if we are idle in this tick, we flush out-of-cpu time to give it the
>   chance to update whatever last-measure internal variable it may have.

Problem of using sched notifiers is that you don't differentiate whether
the vcpu scheduled out by its own (via hlt emulation) or not.

Skipping accounting of user/system time whenever there's any stolen
time detected probably breaks u/s accounting on non-cpu-hog loads.

I suppose steal time should be accounted separately from u/s ticks, as
Xen does.

+   if (delta > 1000UL)
+               touch_softlockup_watchdog();
+

This will break authentic soft lockup detection whenever qemu processing
takes more than 1s.

> 
> This approach is simple, but proved to work well for my test scenarios.
> in a UP guest on UP host, with a cpu-hog in both guest and host shows
> ~ 50 % steal time. steal time is also accounted proportionally, if
> nice values are given to the host cpu-hog.
> 
> A cpu-hog in the host with no load in the guest, produces 0 % steal time,
> with 100 % idle, as one would expect.
> 
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  include/linux/sched.h |    1 +
>  kernel/sched.c        |   29 +++++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 0478888..e571ddd 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -312,6 +312,7 @@ long io_schedule_timeout(long timeout);
>  extern void cpu_init (void);
>  extern void trap_init(void);
>  extern void update_process_times(int user);
> +extern cputime_t (*hypervisor_steal_time)(void);
>  extern void scheduler_tick(void);
>  
>  extern void sched_show_task(struct task_struct *p);
> diff --git a/kernel/sched.c b/kernel/sched.c
> index f52a880..9695c92 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -3157,6 +3157,16 @@ unsigned long long thread_group_sched_runtime(struct task_struct *p)
>  	return ns;
>  }
>  
> +cputime_t (*hypervisor_steal_time)(void) = NULL;
> +
> +static inline cputime_t get_steal_time_from_hypervisor(void)
> +{
> +	if (!hypervisor_steal_time)
> +		return 0;
> +	return hypervisor_steal_time();
> +}
> +
> +
>  /*
>   * Account user cpu time to a process.
>   * @p: the process that the cpu time gets accounted to
> @@ -3169,6 +3179,12 @@ void account_user_time(struct task_struct *p, cputime_t cputime,
>  	struct cpu_usage_stat *cpustat = &kstat_this_cpu.cpustat;
>  	cputime64_t tmp;
>  
> +	tmp = get_steal_time_from_hypervisor();
> +	if (tmp) {
> +		account_steal_time(tmp);
> +		return;
> +	}
> +
>  	/* Add user time to process. */
>  	p->utime = cputime_add(p->utime, cputime);
>  	p->utimescaled = cputime_add(p->utimescaled, cputime_scaled);
> @@ -3234,6 +3250,12 @@ void account_system_time(struct task_struct *p, int hardirq_offset,
>  		return;
>  	}
>  
> +	tmp = get_steal_time_from_hypervisor();
> +	if (tmp) {
> +		account_steal_time(tmp);
> +		return;
> +	}
> +
>  	/* Add system time to process. */
>  	p->stime = cputime_add(p->stime, cputime);
>  	p->stimescaled = cputime_add(p->stimescaled, cputime_scaled);
> @@ -3276,6 +3298,13 @@ void account_idle_time(cputime_t cputime)
>  	cputime64_t cputime64 = cputime_to_cputime64(cputime);
>  	struct rq *rq = this_rq();
>  
> +	/*
> +	 * if we're idle, we don't account it as steal time, since we did
> +	 * not want to run anyway. We do call the steal function, however, to
> +	 * give the guest the chance to flush its internal buffers
> +	 */
> +	get_steal_time_from_hypervisor();
> +
>  	if (atomic_read(&rq->nr_iowait) > 0)
>  		cpustat->iowait = cputime64_add(cpustat->iowait, cputime64);
>  	else
> -- 
> 1.6.2.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/7] Implement getnsboottime kernel API

[Rik van Riel]

On 08/25/2010 05:43 PM, Glauber Costa wrote:
> From: Zachary Amsden<zamsden@redhat.com>
>
> Add a kernel call to get the number of nanoseconds since boot.  This
> is generally useful enough to make it a generic call.
>
> Signed-off-by: Zachary Amsden<zamsden@redhat.com>

Acked-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed

Re: [RFC 4/7] change kernel accounting to include steal time

[Glauber Costa]

On Thu, Aug 26, 2010 at 02:23:03PM -0300, Marcelo Tosatti wrote:
> On Wed, Aug 25, 2010 at 05:43:14PM -0400, Glauber Costa wrote:
> > This patch proposes a common steal time implementation. When no
> > steal time is accounted, we just add a branch to the current
> > accounting code, that shouldn't add much overhead.
> > 
> > When we do want to register steal time, we proceed as following:
> > - if we would account user or system time in this tick, and there is
> >   out-of-cpu time registered, we skip it altogether, and account steal
> >   time only.
> > - if we would account user or system time in this tick, and we got the
> >   cpu for the whole slice, we proceed normaly.
> > - if we are idle in this tick, we flush out-of-cpu time to give it the
> >   chance to update whatever last-measure internal variable it may have.
> 
> Problem of using sched notifiers is that you don't differentiate whether
> the vcpu scheduled out by its own (via hlt emulation) or not.
And we don't need to. If we're out because we want to, we're idle.
And so, we don't account steal time.

> Skipping accounting of user/system time whenever there's any stolen
> time detected probably breaks u/s accounting on non-cpu-hog loads.
I am willing to test some workloads you can suggest, but right now,
(yeah, I mostly used cpu-hogs), this scheme worked better.

Linux does statistical sampling for accounting anyway, so I don't see
it getting much worse.

> 
> I suppose steal time should be accounted separately from u/s ticks, as
> Xen does.
It requires us to hook somewhere else, which I deem as overcomplicated.
Do you have any suggestion on how to make it simple?

Furthermore, "doing separate", is equivalent of not skipping user/system,
if we really prefer to.


> +   if (delta > 1000UL)
> +               touch_softlockup_watchdog();
> +
> 
> This will break authentic soft lockup detection whenever qemu processing
> takes more than 1s.

This should be 10s. 1000UL is a typo.

> 
> > 
> > This approach is simple, but proved to work well for my test scenarios.
> > in a UP guest on UP host, with a cpu-hog in both guest and host shows
> > ~ 50 % steal time. steal time is also accounted proportionally, if
> > nice values are given to the host cpu-hog.
> > 
> > A cpu-hog in the host with no load in the guest, produces 0 % steal time,
> > with 100 % idle, as one would expect.
> > 
> > Signed-off-by: Glauber Costa <glommer@redhat.com>
> > ---
> >  include/linux/sched.h |    1 +
> >  kernel/sched.c        |   29 +++++++++++++++++++++++++++++
> >  2 files changed, 30 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 0478888..e571ddd 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -312,6 +312,7 @@ long io_schedule_timeout(long timeout);
> >  extern void cpu_init (void);
> >  extern void trap_init(void);
> >  extern void update_process_times(int user);
> > +extern cputime_t (*hypervisor_steal_time)(void);
> >  extern void scheduler_tick(void);
> >  
> >  extern void sched_show_task(struct task_struct *p);
> > diff --git a/kernel/sched.c b/kernel/sched.c
> > index f52a880..9695c92 100644
> > --- a/kernel/sched.c
> > +++ b/kernel/sched.c
> > @@ -3157,6 +3157,16 @@ unsigned long long thread_group_sched_runtime(struct task_struct *p)
> >  	return ns;
> >  }
> >  
> > +cputime_t (*hypervisor_steal_time)(void) = NULL;
> > +
> > +static inline cputime_t get_steal_time_from_hypervisor(void)
> > +{
> > +	if (!hypervisor_steal_time)
> > +		return 0;
> > +	return hypervisor_steal_time();
> > +}
> > +
> > +
> >  /*
> >   * Account user cpu time to a process.
> >   * @p: the process that the cpu time gets accounted to
> > @@ -3169,6 +3179,12 @@ void account_user_time(struct task_struct *p, cputime_t cputime,
> >  	struct cpu_usage_stat *cpustat = &kstat_this_cpu.cpustat;
> >  	cputime64_t tmp;
> >  
> > +	tmp = get_steal_time_from_hypervisor();
> > +	if (tmp) {
> > +		account_steal_time(tmp);
> > +		return;
> > +	}
> > +
> >  	/* Add user time to process. */
> >  	p->utime = cputime_add(p->utime, cputime);
> >  	p->utimescaled = cputime_add(p->utimescaled, cputime_scaled);
> > @@ -3234,6 +3250,12 @@ void account_system_time(struct task_struct *p, int hardirq_offset,
> >  		return;
> >  	}
> >  
> > +	tmp = get_steal_time_from_hypervisor();
> > +	if (tmp) {
> > +		account_steal_time(tmp);
> > +		return;
> > +	}
> > +
> >  	/* Add system time to process. */
> >  	p->stime = cputime_add(p->stime, cputime);
> >  	p->stimescaled = cputime_add(p->stimescaled, cputime_scaled);
> > @@ -3276,6 +3298,13 @@ void account_idle_time(cputime_t cputime)
> >  	cputime64_t cputime64 = cputime_to_cputime64(cputime);
> >  	struct rq *rq = this_rq();
> >  
> > +	/*
> > +	 * if we're idle, we don't account it as steal time, since we did
> > +	 * not want to run anyway. We do call the steal function, however, to
> > +	 * give the guest the chance to flush its internal buffers
> > +	 */
> > +	get_steal_time_from_hypervisor();
> > +
> >  	if (atomic_read(&rq->nr_iowait) > 0)
> >  		cpustat->iowait = cputime64_add(cpustat->iowait, cputime64);
> >  	else
> > -- 
> > 1.6.2.2
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 2/7] change headers preparing for steal time

[Zachary Amsden]

On 08/25/2010 11:43 AM, Glauber Costa wrote:
> This guest/host common patch prepares infrastructure for
> the steal time implementation. Some constants are added,
> and a name change happens in pvclock vcpu structure.
>
> Signed-off-by: Glauber Costa<glommer@redhat.com>
> ---
>   arch/x86/include/asm/kvm_para.h    |    1 +
>   arch/x86/include/asm/pvclock-abi.h |    4 +++-
>   2 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
> index 05eba5e..1759c81 100644
> --- a/arch/x86/include/asm/kvm_para.h
> +++ b/arch/x86/include/asm/kvm_para.h
> @@ -25,6 +25,7 @@
>    * in pvclock structure. If no bits are set, all flags are ignored.
>    */
>   #define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT	24
> +#define KVM_FEATURE_CLOCKSOURCE_STEAL_BIT 	25
>
>   #define MSR_KVM_WALL_CLOCK  0x11
>   #define MSR_KVM_SYSTEM_TIME 0x12
> diff --git a/arch/x86/include/asm/pvclock-abi.h b/arch/x86/include/asm/pvclock-abi.h
> index 35f2d19..417061b 100644
> --- a/arch/x86/include/asm/pvclock-abi.h
> +++ b/arch/x86/include/asm/pvclock-abi.h
> @@ -24,7 +24,7 @@
>
>   struct pvclock_vcpu_time_info {
>   	u32   version;
> -	u32   pad0;
> +	u32   steal_time;
>   	u64   tsc_timestamp;
>   	u64   system_time;
>   	u32   tsc_to_system_mul;
> @@ -40,5 +40,7 @@ struct pvclock_wall_clock {
>   } __attribute__((__packed__));
>
>   #define PVCLOCK_TSC_STABLE_BIT	(1<<  0)
> +#define PVCLOCK_STEAL_BIT   (2<<  0)
> +
>   #endif /* __ASSEMBLY__ */
>   #endif /* _ASM_X86_PVCLOCK_ABI_H */
>    


Will 32 bits be enough?

Re: [RFC 4/7] change kernel accounting to include steal time

[Marcelo Tosatti]

On Thu, Aug 26, 2010 at 05:28:56PM -0300, Glauber Costa wrote:
> On Thu, Aug 26, 2010 at 02:23:03PM -0300, Marcelo Tosatti wrote:
> > On Wed, Aug 25, 2010 at 05:43:14PM -0400, Glauber Costa wrote:
> > > This patch proposes a common steal time implementation. When no
> > > steal time is accounted, we just add a branch to the current
> > > accounting code, that shouldn't add much overhead.
> > > 
> > > When we do want to register steal time, we proceed as following:
> > > - if we would account user or system time in this tick, and there is
> > >   out-of-cpu time registered, we skip it altogether, and account steal
> > >   time only.
> > > - if we would account user or system time in this tick, and we got the
> > >   cpu for the whole slice, we proceed normaly.
> > > - if we are idle in this tick, we flush out-of-cpu time to give it the
> > >   chance to update whatever last-measure internal variable it may have.
> > 
> > Problem of using sched notifiers is that you don't differentiate whether
> > the vcpu scheduled out by its own (via hlt emulation) or not.
> And we don't need to. If we're out because we want to, we're idle.
> And so, we don't account steal time.

Think of the program below.

> > Skipping accounting of user/system time whenever there's any stolen
> > time detected probably breaks u/s accounting on non-cpu-hog loads.
> I am willing to test some workloads you can suggest, but right now,
> (yeah, I mostly used cpu-hogs), this scheme worked better.
> 
> Linux does statistical sampling for accounting anyway, so I don't see
> it getting much worse.

A "cpu hog" that sleeps 1us every 1ms.

> > I suppose steal time should be accounted separately from u/s ticks, as
> > Xen does.
> It requires us to hook somewhere else, which I deem as overcomplicated.
> Do you have any suggestion on how to make it simple?

Unfortunately no.

> Furthermore, "doing separate", is equivalent of not skipping user/system,
> if we really prefer to.
>
> > +   if (delta > 1000UL)
> > +               touch_softlockup_watchdog();
> > +
> > 
> > This will break authentic soft lockup detection whenever qemu processing
> > takes more than 1s.
> 
> This should be 10s. 1000UL is a typo.

Comment is still valid.

Re: [RFC 3/7] measure time out of guest

[Zachary Amsden]

On 08/25/2010 11:43 AM, Glauber Costa wrote:
> By measuring time between a vcpu_put and a vcpu_load, we can
> estimate how much time did the guest stay out of the cpu.
> This is exported to the guest at every clock update.
>
> Signed-off-by: Glauber Costa<glommer@redhat.com>
> ---
>   arch/x86/include/asm/kvm_host.h |    2 ++
>   arch/x86/kvm/x86.c              |   12 +++++++++++-
>   2 files changed, 13 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 502e53f..bc28aff 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -364,6 +364,8 @@ struct kvm_vcpu_arch {
>   	u64 hv_vapic;
>
>   	cpumask_var_t wbinvd_dirty_mask;
> +	u64 time_out;
> +	u64 last_time_out;
>   };
>
>   struct kvm_arch {
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e7e3b50..680feaa 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -925,8 +925,9 @@ static void kvm_write_guest_time(struct kvm_vcpu *v)
>   	vcpu->hv_clock.system_time = ts.tv_nsec +
>   				     (NSEC_PER_SEC * (u64)ts.tv_sec) + v->kvm->arch.kvmclock_offset;
>
> -	vcpu->hv_clock.flags = 0;
> +	vcpu->hv_clock.flags = PVCLOCK_STEAL_BIT;
>
> +	vcpu->hv_clock.steal_time = vcpu->time_out / 1000000;
>   	/*
>   	 * The interface expects us to write an even number signaling that the
>   	 * update is finished. Since the guest won't see the intermediate
> @@ -1798,6 +1799,8 @@ static bool need_emulate_wbinvd(struct kvm_vcpu *vcpu)
>
>   void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>   {
> +	s64 now;
> +
>   	/* Address WBINVD may be executed by guest */
>   	if (need_emulate_wbinvd(vcpu)) {
>   		if (kvm_x86_ops->has_wbinvd_exit())
> @@ -1815,12 +1818,19 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>   		per_cpu(cpu_tsc_khz, cpu) = khz;
>   	}
>   	kvm_request_guest_time_update(vcpu);
> +
> +	now = getnsboottime();
> +
> +	if (vcpu->arch.last_time_out != 0)
> +		vcpu->arch.time_out += now - vcpu->arch.last_time_out;
>    


>   }
>
>   void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>   {
>   	kvm_x86_ops->vcpu_put(vcpu);
>   	kvm_put_guest_fpu(vcpu);
> +
> +	vcpu->arch.last_time_out = getnsboottime();
>   }
>
>   static int is_efer_nx(void)
>    

I think I've added some of the same instrumentation in my series.

Re: [RFC 2/7] change headers preparing for steal time

[Rik van Riel]

On 08/26/2010 04:44 PM, Zachary Amsden wrote:
> On 08/25/2010 11:43 AM, Glauber Costa wrote:
>> This guest/host common patch prepares infrastructure for
>> the steal time implementation. Some constants are added,
>> and a name change happens in pvclock vcpu structure.
>>
>> Signed-off-by: Glauber Costa<glommer@redhat.com>
>> ---
>> arch/x86/include/asm/kvm_para.h | 1 +
>> arch/x86/include/asm/pvclock-abi.h | 4 +++-
>> 2 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_para.h
>> b/arch/x86/include/asm/kvm_para.h
>> index 05eba5e..1759c81 100644
>> --- a/arch/x86/include/asm/kvm_para.h
>> +++ b/arch/x86/include/asm/kvm_para.h
>> @@ -25,6 +25,7 @@
>> * in pvclock structure. If no bits are set, all flags are ignored.
>> */
>> #define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT 24
>> +#define KVM_FEATURE_CLOCKSOURCE_STEAL_BIT 25
>>
>> #define MSR_KVM_WALL_CLOCK 0x11
>> #define MSR_KVM_SYSTEM_TIME 0x12
>> diff --git a/arch/x86/include/asm/pvclock-abi.h
>> b/arch/x86/include/asm/pvclock-abi.h
>> index 35f2d19..417061b 100644
>> --- a/arch/x86/include/asm/pvclock-abi.h
>> +++ b/arch/x86/include/asm/pvclock-abi.h
>> @@ -24,7 +24,7 @@
>>
>> struct pvclock_vcpu_time_info {
>> u32 version;
>> - u32 pad0;
>> + u32 steal_time;
>> u64 tsc_timestamp;
>> u64 system_time;
>> u32 tsc_to_system_mul;
>> @@ -40,5 +40,7 @@ struct pvclock_wall_clock {
>> } __attribute__((__packed__));
>>
>> #define PVCLOCK_TSC_STABLE_BIT (1<< 0)
>> +#define PVCLOCK_STEAL_BIT (2<< 0)
>> +
>> #endif /* __ASSEMBLY__ */
>> #endif /* _ASM_X86_PVCLOCK_ABI_H */
>
>
> Will 32 bits be enough?

Good question.   Reading the rest of the code,
I suspect it won't be, but Glauber will know
better.

-- 
All rights reversed

Re: [RFC 4/7] change kernel accounting to include steal time

[Rik van Riel]

On 08/26/2010 04:47 PM, Marcelo Tosatti wrote:
> On Thu, Aug 26, 2010 at 05:28:56PM -0300, Glauber Costa wrote:
>> On Thu, Aug 26, 2010 at 02:23:03PM -0300, Marcelo Tosatti wrote:

>>> Skipping accounting of user/system time whenever there's any stolen
>>> time detected probably breaks u/s accounting on non-cpu-hog loads.

Steal time does not completely skip accounting of user/system
time.  Say that it has been 10ms since the last timer tick, we
are currently in user mode, and 4ms have been accounted as steal
time.

The steal time accounting code will then account 4ms as steal
time, and 6ms as user time.

It does not "skip accounting of user/system time" at all.

>> I am willing to test some workloads you can suggest, but right now,
>> (yeah, I mostly used cpu-hogs), this scheme worked better.
>>
>> Linux does statistical sampling for accounting anyway, so I don't see
>> it getting much worse.
>
> A "cpu hog" that sleeps 1us every 1ms.

This kind of program can be an issue with or without
steal time.

I don't see steal time make the situation any worse.

-- 
All rights reversed

Re: [RFC 4/7] change kernel accounting to include steal time

[Glauber Costa]

On Thu, Aug 26, 2010 at 05:47:12PM -0300, Marcelo Tosatti wrote:
> > Linux does statistical sampling for accounting anyway, so I don't see
> > it getting much worse.
> 
> A "cpu hog" that sleeps 1us every 1ms.
> 
Imagine a user program, that at periodic intervals, goes to the system.
Under the current scheme, it will be accounted as 100 % system.
This is all statistical anyway

Re: [RFC 4/7] change kernel accounting to include steal time

[Anthony Liguori]

On 08/26/2010 03:28 PM, Glauber Costa wrote:
>
>> +   if (delta>  1000UL)
>> +               touch_softlockup_watchdog();
>> +
>>
>> This will break authentic soft lockup detection whenever qemu processing
>> takes more than 1s.
>>      
> This should be 10s. 1000UL is a typo.
>    

I was wondering that when I first saw the patch..  10s is the default 
detection time but it's actually run time configurable so hard coding 
10s is not correct.

Regards,

Anthony Liguori

Re: [RFC 3/7] measure time out of guest

[Glauber Costa]

> >  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> >  {
> >  	kvm_x86_ops->vcpu_put(vcpu);
> >  	kvm_put_guest_fpu(vcpu);
> >+
> >+	vcpu->arch.last_time_out = getnsboottime();
> >  }
> >
> >  static int is_efer_nx(void)
> 
> I think I've added some of the same instrumentation in my series.
Yeah, I did picked up one of your patches verbatim, if you don't mind =]

Re: [RFC 2/7] change headers preparing for steal time

[Glauber Costa]

On Thu, Aug 26, 2010 at 05:04:02PM -0400, Rik van Riel wrote:
> On 08/26/2010 04:44 PM, Zachary Amsden wrote:
> >On 08/25/2010 11:43 AM, Glauber Costa wrote:
> >>This guest/host common patch prepares infrastructure for
> >>the steal time implementation. Some constants are added,
> >>and a name change happens in pvclock vcpu structure.
> >>
> >>Signed-off-by: Glauber Costa<glommer@redhat.com>
> >>---
> >>arch/x86/include/asm/kvm_para.h | 1 +
> >>arch/x86/include/asm/pvclock-abi.h | 4 +++-
> >>2 files changed, 4 insertions(+), 1 deletions(-)
> >>
> >>diff --git a/arch/x86/include/asm/kvm_para.h
> >>b/arch/x86/include/asm/kvm_para.h
> >>index 05eba5e..1759c81 100644
> >>--- a/arch/x86/include/asm/kvm_para.h
> >>+++ b/arch/x86/include/asm/kvm_para.h
> >>@@ -25,6 +25,7 @@
> >>* in pvclock structure. If no bits are set, all flags are ignored.
> >>*/
> >>#define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT 24
> >>+#define KVM_FEATURE_CLOCKSOURCE_STEAL_BIT 25
> >>
> >>#define MSR_KVM_WALL_CLOCK 0x11
> >>#define MSR_KVM_SYSTEM_TIME 0x12
> >>diff --git a/arch/x86/include/asm/pvclock-abi.h
> >>b/arch/x86/include/asm/pvclock-abi.h
> >>index 35f2d19..417061b 100644
> >>--- a/arch/x86/include/asm/pvclock-abi.h
> >>+++ b/arch/x86/include/asm/pvclock-abi.h
> >>@@ -24,7 +24,7 @@
> >>
> >>struct pvclock_vcpu_time_info {
> >>u32 version;
> >>- u32 pad0;
> >>+ u32 steal_time;
> >>u64 tsc_timestamp;
> >>u64 system_time;
> >>u32 tsc_to_system_mul;
> >>@@ -40,5 +40,7 @@ struct pvclock_wall_clock {
> >>} __attribute__((__packed__));
> >>
> >>#define PVCLOCK_TSC_STABLE_BIT (1<< 0)
> >>+#define PVCLOCK_STEAL_BIT (2<< 0)
> >>+
> >>#endif /* __ASSEMBLY__ */
> >>#endif /* _ASM_X86_PVCLOCK_ABI_H */
> >
> >
> >Will 32 bits be enough?
> 
> Good question.   Reading the rest of the code,
> I suspect it won't be, but Glauber will know
> better.

We are at msec resolution, not nanoseconds.
And I doubt we need more than that, since
we'll later on convert to jiffie granularity.

it gives us enough for about 50 days of steal
time.

Re: [RFC 4/7] change kernel accounting to include steal time

[Rik van Riel]

On 08/25/2010 05:43 PM, Glauber Costa wrote:
> This patch proposes a common steal time implementation. When no
> steal time is accounted, we just add a branch to the current
> accounting code, that shouldn't add much overhead.
>
> When we do want to register steal time, we proceed as following:
> - if we would account user or system time in this tick, and there is
>    out-of-cpu time registered, we skip it altogether, and account steal
>    time only.
> - if we would account user or system time in this tick, and we got the
>    cpu for the whole slice, we proceed normaly.
> - if we are idle in this tick, we flush out-of-cpu time to give it the
>    chance to update whatever last-measure internal variable it may have.
>
> This approach is simple, but proved to work well for my test scenarios.
> in a UP guest on UP host, with a cpu-hog in both guest and host shows
> ~ 50 % steal time. steal time is also accounted proportionally, if
> nice values are given to the host cpu-hog.
>
> A cpu-hog in the host with no load in the guest, produces 0 % steal time,
> with 100 % idle, as one would expect.
>
> Signed-off-by: Glauber Costa<glommer@redhat.com>
> ---
>   include/linux/sched.h |    1 +
>   kernel/sched.c        |   29 +++++++++++++++++++++++++++++
>   2 files changed, 30 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 0478888..e571ddd 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -312,6 +312,7 @@ long io_schedule_timeout(long timeout);
>   extern void cpu_init (void);
>   extern void trap_init(void);
>   extern void update_process_times(int user);
> +extern cputime_t (*hypervisor_steal_time)(void);
>   extern void scheduler_tick(void);
>
>   extern void sched_show_task(struct task_struct *p);
> diff --git a/kernel/sched.c b/kernel/sched.c
> index f52a880..9695c92 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -3157,6 +3157,16 @@ unsigned long long thread_group_sched_runtime(struct task_struct *p)
>   	return ns;
>   }
>
> +cputime_t (*hypervisor_steal_time)(void) = NULL;
> +
> +static inline cputime_t get_steal_time_from_hypervisor(void)
> +{
> +	if (!hypervisor_steal_time)
> +		return 0;
> +	return hypervisor_steal_time();
> +}
> +
> +
>   /*
>    * Account user cpu time to a process.
>    * @p: the process that the cpu time gets accounted to
> @@ -3169,6 +3179,12 @@ void account_user_time(struct task_struct *p, cputime_t cputime,
>   	struct cpu_usage_stat *cpustat =&kstat_this_cpu.cpustat;
>   	cputime64_t tmp;
>
> +	tmp = get_steal_time_from_hypervisor();
> +	if (tmp) {
> +		account_steal_time(tmp);
> +		return;
> +	}
> +
>   	/* Add user time to process. */
>   	p->utime = cputime_add(p->utime, cputime);
>   	p->utimescaled = cputime_add(p->utimescaled, cputime_scaled);

I see one problem here.

What if get_steal_time_from_hypervisor() returns a smaller
amount of time than "cputime"?

Would it be better to account tmp as stealtime, and the
difference (cputime - tmp) as user/sys/... time?

-- 
All rights reversed

Re: [RFC 4/7] change kernel accounting to include steal time

[Glauber Costa]

On Thu, Aug 26, 2010 at 05:19:23PM -0400, Rik van Riel wrote:
> On 08/25/2010 05:43 PM, Glauber Costa wrote:
> >This patch proposes a common steal time implementation. When no
> >steal time is accounted, we just add a branch to the current
> >accounting code, that shouldn't add much overhead.
> >
> >When we do want to register steal time, we proceed as following:
> >- if we would account user or system time in this tick, and there is
> >   out-of-cpu time registered, we skip it altogether, and account steal
> >   time only.
> >- if we would account user or system time in this tick, and we got the
> >   cpu for the whole slice, we proceed normaly.
> >- if we are idle in this tick, we flush out-of-cpu time to give it the
> >   chance to update whatever last-measure internal variable it may have.
> >
> >This approach is simple, but proved to work well for my test scenarios.
> >in a UP guest on UP host, with a cpu-hog in both guest and host shows
> >~ 50 % steal time. steal time is also accounted proportionally, if
> >nice values are given to the host cpu-hog.
> >
> >A cpu-hog in the host with no load in the guest, produces 0 % steal time,
> >with 100 % idle, as one would expect.
> >
> >Signed-off-by: Glauber Costa<glommer@redhat.com>
> >---
> >  include/linux/sched.h |    1 +
> >  kernel/sched.c        |   29 +++++++++++++++++++++++++++++
> >  2 files changed, 30 insertions(+), 0 deletions(-)
> >
> >diff --git a/include/linux/sched.h b/include/linux/sched.h
> >index 0478888..e571ddd 100644
> >--- a/include/linux/sched.h
> >+++ b/include/linux/sched.h
> >@@ -312,6 +312,7 @@ long io_schedule_timeout(long timeout);
> >  extern void cpu_init (void);
> >  extern void trap_init(void);
> >  extern void update_process_times(int user);
> >+extern cputime_t (*hypervisor_steal_time)(void);
> >  extern void scheduler_tick(void);
> >
> >  extern void sched_show_task(struct task_struct *p);
> >diff --git a/kernel/sched.c b/kernel/sched.c
> >index f52a880..9695c92 100644
> >--- a/kernel/sched.c
> >+++ b/kernel/sched.c
> >@@ -3157,6 +3157,16 @@ unsigned long long thread_group_sched_runtime(struct task_struct *p)
> >  	return ns;
> >  }
> >
> >+cputime_t (*hypervisor_steal_time)(void) = NULL;
> >+
> >+static inline cputime_t get_steal_time_from_hypervisor(void)
> >+{
> >+	if (!hypervisor_steal_time)
> >+		return 0;
> >+	return hypervisor_steal_time();
> >+}
> >+
> >+
> >  /*
> >   * Account user cpu time to a process.
> >   * @p: the process that the cpu time gets accounted to
> >@@ -3169,6 +3179,12 @@ void account_user_time(struct task_struct *p, cputime_t cputime,
> >  	struct cpu_usage_stat *cpustat =&kstat_this_cpu.cpustat;
> >  	cputime64_t tmp;
> >
> >+	tmp = get_steal_time_from_hypervisor();
> >+	if (tmp) {
> >+		account_steal_time(tmp);
> >+		return;
> >+	}
> >+
> >  	/* Add user time to process. */
> >  	p->utime = cputime_add(p->utime, cputime);
> >  	p->utimescaled = cputime_add(p->utimescaled, cputime_scaled);
> 
> I see one problem here.
> 
> What if get_steal_time_from_hypervisor() returns a smaller
> amount of time than "cputime"?
> 
> Would it be better to account tmp as stealtime, and the
> difference (cputime - tmp) as user/sys/... time?

There is also the case in which tmp is greater than cputime,
but not a multiple of it. In which case, I believe we should
account cputime - (tmp % cputime) as user/sys too.

Re: [RFC 4/7] change kernel accounting to include steal time

[Glauber Costa]

On Thu, Aug 26, 2010 at 04:14:47PM -0500, Anthony Liguori wrote:
> On 08/26/2010 03:28 PM, Glauber Costa wrote:
> >
> >>+   if (delta>  1000UL)
> >>+               touch_softlockup_watchdog();
> >>+
> >>
> >>This will break authentic soft lockup detection whenever qemu processing
> >>takes more than 1s.
> >This should be 10s. 1000UL is a typo.
> 
> I was wondering that when I first saw the patch..  10s is the
> default detection time but it's actually run time configurable so
> hard coding 10s is not correct.

Indeed, you are right.

Thanks

Re: [RFC 2/7] change headers preparing for steal time

[Rik van Riel]

On 08/26/2010 05:17 PM, Glauber Costa wrote:
> On Thu, Aug 26, 2010 at 05:04:02PM -0400, Rik van Riel wrote:
>> On 08/26/2010 04:44 PM, Zachary Amsden wrote:

>>> Will 32 bits be enough?
>>
>> Good question.   Reading the rest of the code,
>> I suspect it won't be, but Glauber will know
>> better.
>
> We are at msec resolution, not nanoseconds.
> And I doubt we need more than that, since
> we'll later on convert to jiffie granularity.
>
> it gives us enough for about 50 days of steal
> time.

I guess what really helps us here is that
kvm_get_steal_time() returns the delta.

This means it can be (it's using u64 at the
moment, but that is fixable) changed to deal
with overflows.

At that point, it should do the right thing
even if the 32 bit value contained nanoseconds.
And with microseconds, it would certainly be
plenty :)

-- 
All rights reversed

Re: [RFC 5/7] kvm steal time implementation

[Rik van Riel]

On 08/25/2010 05:43 PM, Glauber Costa wrote:
> This is the proposed kvm-side steal time implementation.
> It is migration safe, as it checks flags at every read.
>
> Signed-off-by: Glauber Costa<glommer@redhat.com>
> ---
>   arch/x86/kernel/kvmclock.c |   35 +++++++++++++++++++++++++++++++++++
>   1 files changed, 35 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index eb9b76c..a1f4852 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -18,6 +18,8 @@
>
>   #include<linux/clocksource.h>
>   #include<linux/kvm_para.h>
> +#include<linux/kernel_stat.h>
> +#include<linux/sched.h>
>   #include<asm/pvclock.h>
>   #include<asm/msr.h>
>   #include<asm/apic.h>
> @@ -41,6 +43,7 @@ early_param("no-kvmclock", parse_no_kvmclock);
>
>   /* The hypervisor will put information about time periodically here */
>   static DEFINE_PER_CPU_SHARED_ALIGNED(struct pvclock_vcpu_time_info, hv_clock);
> +static DEFINE_PER_CPU(u64, steal_info);
>   static struct pvclock_wall_clock wall_clock;
>
>   /*
> @@ -82,6 +85,32 @@ static cycle_t kvm_clock_read(void)
>   	return ret;
>   }
>
> +static DEFINE_PER_CPU(u64, steal_info);
> +
> +cputime_t kvm_get_steal_time(void)
> +{
> +	u64 delta = 0;
> +	u64 *last_steal_info, this_steal_info;
> +	struct pvclock_vcpu_time_info *src;
> +
> +	src =&get_cpu_var(hv_clock);
> +	if (!(src->flags&  PVCLOCK_STEAL_BIT))
> +		goto out;
> +
> +	this_steal_info = src->steal_time;
> +	put_cpu_var(hv_clock);
> +
> +	last_steal_info =&get_cpu_var(steal_info);
> +
> +	delta = this_steal_info - *last_steal_info;
> +
> +	*last_steal_info = this_steal_info;
> +	put_cpu_var(steal_info);
> +
> +out:
> +	return msecs_to_cputime(delta);
> +}

Can this be changed to properly deal with overflow in
src->steal_time, the same way we deal with (eg jiffie)
overflow elsewhere in the kernel?

-- 
All rights reversed

Re: [RFC 5/7] kvm steal time implementation

[Glauber Costa]

On Thu, Aug 26, 2010 at 06:13:07PM -0400, Rik van Riel wrote:
> On 08/25/2010 05:43 PM, Glauber Costa wrote:
> >This is the proposed kvm-side steal time implementation.
> >It is migration safe, as it checks flags at every read.
> >
> >Signed-off-by: Glauber Costa<glommer@redhat.com>
> >---
> >  arch/x86/kernel/kvmclock.c |   35 +++++++++++++++++++++++++++++++++++
> >  1 files changed, 35 insertions(+), 0 deletions(-)
> >
> >diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> >index eb9b76c..a1f4852 100644
> >--- a/arch/x86/kernel/kvmclock.c
> >+++ b/arch/x86/kernel/kvmclock.c
> >@@ -18,6 +18,8 @@
> >
> >  #include<linux/clocksource.h>
> >  #include<linux/kvm_para.h>
> >+#include<linux/kernel_stat.h>
> >+#include<linux/sched.h>
> >  #include<asm/pvclock.h>
> >  #include<asm/msr.h>
> >  #include<asm/apic.h>
> >@@ -41,6 +43,7 @@ early_param("no-kvmclock", parse_no_kvmclock);
> >
> >  /* The hypervisor will put information about time periodically here */
> >  static DEFINE_PER_CPU_SHARED_ALIGNED(struct pvclock_vcpu_time_info, hv_clock);
> >+static DEFINE_PER_CPU(u64, steal_info);
> >  static struct pvclock_wall_clock wall_clock;
> >
> >  /*
> >@@ -82,6 +85,32 @@ static cycle_t kvm_clock_read(void)
> >  	return ret;
> >  }
> >
> >+static DEFINE_PER_CPU(u64, steal_info);
> >+
> >+cputime_t kvm_get_steal_time(void)
> >+{
> >+	u64 delta = 0;
> >+	u64 *last_steal_info, this_steal_info;
> >+	struct pvclock_vcpu_time_info *src;
> >+
> >+	src =&get_cpu_var(hv_clock);
> >+	if (!(src->flags&  PVCLOCK_STEAL_BIT))
> >+		goto out;
> >+
> >+	this_steal_info = src->steal_time;
> >+	put_cpu_var(hv_clock);
> >+
> >+	last_steal_info =&get_cpu_var(steal_info);
> >+
> >+	delta = this_steal_info - *last_steal_info;
> >+
> >+	*last_steal_info = this_steal_info;
> >+	put_cpu_var(steal_info);
> >+
> >+out:
> >+	return msecs_to_cputime(delta);
> >+}
> 
> Can this be changed to properly deal with overflow in
> src->steal_time, the same way we deal with (eg jiffie)
> overflow elsewhere in the kernel?
I believe so.

Re: [RFC 4/7] change kernel accounting to include steal time

[Marcelo Tosatti]

On Thu, Aug 26, 2010 at 06:40:36PM -0300, Glauber Costa wrote:
> On Thu, Aug 26, 2010 at 04:14:47PM -0500, Anthony Liguori wrote:
> > On 08/26/2010 03:28 PM, Glauber Costa wrote:
> > >
> > >>+   if (delta>  1000UL)
> > >>+               touch_softlockup_watchdog();
> > >>+
> > >>
> > >>This will break authentic soft lockup detection whenever qemu processing
> > >>takes more than 1s.
> > >This should be 10s. 1000UL is a typo.
> > 
> > I was wondering that when I first saw the patch..  10s is the
> > default detection time but it's actually run time configurable so
> > hard coding 10s is not correct.

This is not what i'm referring to. The code above will disable
softlockup detection in case of a vcpu blocked in qemu for longer than
softlockup threshold, which is a legitimate case.

> 
> Indeed, you are right.
> 
> Thanks

Re: [RFC 4/7] change kernel accounting to include steal time

[Glauber Costa]

On Thu, Aug 26, 2010 at 08:12:40PM -0300, Marcelo Tosatti wrote:
> On Thu, Aug 26, 2010 at 06:40:36PM -0300, Glauber Costa wrote:
> > On Thu, Aug 26, 2010 at 04:14:47PM -0500, Anthony Liguori wrote:
> > > On 08/26/2010 03:28 PM, Glauber Costa wrote:
> > > >
> > > >>+   if (delta>  1000UL)
> > > >>+               touch_softlockup_watchdog();
> > > >>+
> > > >>
> > > >>This will break authentic soft lockup detection whenever qemu processing
> > > >>takes more than 1s.
> > > >This should be 10s. 1000UL is a typo.
> > > 
> > > I was wondering that when I first saw the patch..  10s is the
> > > default detection time but it's actually run time configurable so
> > > hard coding 10s is not correct.
> 
> This is not what i'm referring to. The code above will disable
> softlockup detection in case of a vcpu blocked in qemu for longer than
> softlockup threshold, which is a legitimate case.
This is equivalent to a hardware so broken it can't even send
an NMI. Not sure we should worry too much about it.

Re: [RFC 4/7] change kernel accounting to include steal time

[Marcelo Tosatti]

On Thu, Aug 26, 2010 at 09:33:02PM -0300, Glauber Costa wrote:
> On Thu, Aug 26, 2010 at 08:12:40PM -0300, Marcelo Tosatti wrote:
> > On Thu, Aug 26, 2010 at 06:40:36PM -0300, Glauber Costa wrote:
> > > On Thu, Aug 26, 2010 at 04:14:47PM -0500, Anthony Liguori wrote:
> > > > On 08/26/2010 03:28 PM, Glauber Costa wrote:
> > > > >
> > > > >>+   if (delta>  1000UL)
> > > > >>+               touch_softlockup_watchdog();
> > > > >>+
> > > > >>
> > > > >>This will break authentic soft lockup detection whenever qemu processing
> > > > >>takes more than 1s.
> > > > >This should be 10s. 1000UL is a typo.
> > > > 
> > > > I was wondering that when I first saw the patch..  10s is the
> > > > default detection time but it's actually run time configurable so
> > > > hard coding 10s is not correct.
> > 
> > This is not what i'm referring to. The code above will disable
> > softlockup detection in case of a vcpu blocked in qemu for longer than
> > softlockup threshold, which is a legitimate case.
> This is equivalent to a hardware so broken it can't even send
> an NMI. Not sure we should worry too much about it.

Well, take the virtio-net issues for example. You'd disable reporting
for those.

Re: [RFC 2/7] change headers preparing for steal time

[Avi Kivity]

  On 08/26/2010 12:43 AM, Glauber Costa wrote:
> This guest/host common patch prepares infrastructure for
> the steal time implementation. Some constants are added,
> and a name change happens in pvclock vcpu structure.
>
> Signed-off-by: Glauber Costa<glommer@redhat.com>
> ---
>   arch/x86/include/asm/kvm_para.h    |    1 +
>   arch/x86/include/asm/pvclock-abi.h |    4 +++-
>   2 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
> index 05eba5e..1759c81 100644
> --- a/arch/x86/include/asm/kvm_para.h
> +++ b/arch/x86/include/asm/kvm_para.h
> @@ -25,6 +25,7 @@
>    * in pvclock structure. If no bits are set, all flags are ignored.
>    */
>   #define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT	24
> +#define KVM_FEATURE_CLOCKSOURCE_STEAL_BIT 	25
>
>   #define MSR_KVM_WALL_CLOCK  0x11
>   #define MSR_KVM_SYSTEM_TIME 0x12
> diff --git a/arch/x86/include/asm/pvclock-abi.h b/arch/x86/include/asm/pvclock-abi.h
> index 35f2d19..417061b 100644
> --- a/arch/x86/include/asm/pvclock-abi.h
> +++ b/arch/x86/include/asm/pvclock-abi.h
> @@ -24,7 +24,7 @@
>
>   struct pvclock_vcpu_time_info {
>   	u32   version;
> -	u32   pad0;
> +	u32   steal_time;
>   	u64   tsc_timestamp;
>   	u64   system_time;
>   	u32   tsc_to_system_mul;
> @@ -40,5 +40,7 @@ struct pvclock_wall_clock {
>   } __attribute__((__packed__));
>
>   #define PVCLOCK_TSC_STABLE_BIT	(1<<  0)
> +#define PVCLOCK_STEAL_BIT   (2<<  0)
> +
>   #endif /* __ASSEMBLY__ */
>   #endif /* _ASM_X86_PVCLOCK_ABI_H */

We could extend pvclock to 64 bytes (unfortunately we didn't reserve 
bits 0-5 like we did with others) and use an nsec field.

We can declare bit 1 to be a 'the address is 64 byte aligned and bits 
2-5 are now feature bits', or we can add a new msr.  Pretty complicated 
but it's good to have everything using the same units.

-- 
error compiling committee.c: too many arguments to function

Re: [RFC 3/7] measure time out of guest

[Avi Kivity]

  On 08/26/2010 12:43 AM, Glauber Costa wrote:
> By measuring time between a vcpu_put and a vcpu_load, we can
> estimate how much time did the guest stay out of the cpu.
> This is exported to the guest at every clock update.
>
> Signed-off-by: Glauber Costa<glommer@redhat.com>
> ---
>   arch/x86/include/asm/kvm_host.h |    2 ++
>   arch/x86/kvm/x86.c              |   12 +++++++++++-
>   2 files changed, 13 insertions(+), 1 deletions(-)
>

Could be put in common code, this is useful to at least s390.

-- 
error compiling committee.c: too many arguments to function

Re: [RFC 4/7] change kernel accounting to include steal time

[Avi Kivity]

  On 08/26/2010 12:43 AM, Glauber Costa wrote:
> This patch proposes a common steal time implementation. When no
> steal time is accounted, we just add a branch to the current
> accounting code, that shouldn't add much overhead.
>
> When we do want to register steal time, we proceed as following:
> - if we would account user or system time in this tick, and there is
>    out-of-cpu time registered, we skip it altogether, and account steal
>    time only.
> - if we would account user or system time in this tick, and we got the
>    cpu for the whole slice, we proceed normaly.
> - if we are idle in this tick, we flush out-of-cpu time to give it the
>    chance to update whatever last-measure internal variable it may have.
>
> This approach is simple, but proved to work well for my test scenarios.
> in a UP guest on UP host, with a cpu-hog in both guest and host shows
> ~ 50 % steal time. steal time is also accounted proportionally, if
> nice values are given to the host cpu-hog.
>
> A cpu-hog in the host with no load in the guest, produces 0 % steal time,
> with 100 % idle, as one would expect.
>

The scheduler people and lkml need to be copied on this patch.

Since s390 does steal time (I think?), can this code be shared?

-- 
error compiling committee.c: too many arguments to function

Re: [RFC 4/7] change kernel accounting to include steal time

[Rik van Riel]

On 08/29/2010 05:59 AM, Avi Kivity wrote:

> The scheduler people and lkml need to be copied on this patch.

Good idea for the second version of the series.

> Since s390 does steal time (I think?), can this code be shared?

That part already is shared.  Glauber's patches reuse the same
code that s390 and Xen use.

-- 
All rights reversed

Re: [RFC 4/7] change kernel accounting to include steal time

[Avi Kivity]

  On 08/29/2010 06:13 PM, Rik van Riel wrote:
>
>> Since s390 does steal time (I think?), can this code be shared?
>
> That part already is shared.  Glauber's patches reuse the same
> code that s390 and Xen use.
>

Why can't we use the same approach as s390 and ppc (calling 
account_steal_time from the timer interrupt)?  or perhaps those should 
be moved into the scheduler proper?

Didn't find the Xen hooks.

-- 
error compiling committee.c: too many arguments to function

Re: [RFC 4/7] change kernel accounting to include steal time

[Rik van Riel]

On 08/29/2010 11:25 AM, Avi Kivity wrote:
> On 08/29/2010 06:13 PM, Rik van Riel wrote:
>>
>>> Since s390 does steal time (I think?), can this code be shared?
>>
>> That part already is shared. Glauber's patches reuse the same
>> code that s390 and Xen use.
>>
>
> Why can't we use the same approach as s390 and ppc (calling
> account_steal_time from the timer interrupt)? or perhaps those should be
> moved into the scheduler proper?

Now that is a good question.

> Didn't find the Xen hooks.

I suspect Xen lost them in the paravirt ops port...

-- 
All rights reversed

Re: [RFC 4/7] change kernel accounting to include steal time

[Avi Kivity]

  On 08/29/2010 06:42 PM, Rik van Riel wrote:
>> Didn't find the Xen hooks.
>
> I suspect Xen lost them in the paravirt ops port...
>

Glauber: please copy Jeremy on this patchset so we can coordinate things.

-- 
error compiling committee.c: too many arguments to function

Re: [RFC 4/7] change kernel accounting to include steal time

[Glauber Costa]

On Sun, Aug 29, 2010 at 12:59:36PM +0300, Avi Kivity wrote:
>  On 08/26/2010 12:43 AM, Glauber Costa wrote:
> >This patch proposes a common steal time implementation. When no
> >steal time is accounted, we just add a branch to the current
> >accounting code, that shouldn't add much overhead.
> >
> >When we do want to register steal time, we proceed as following:
> >- if we would account user or system time in this tick, and there is
> >   out-of-cpu time registered, we skip it altogether, and account steal
> >   time only.
> >- if we would account user or system time in this tick, and we got the
> >   cpu for the whole slice, we proceed normaly.
> >- if we are idle in this tick, we flush out-of-cpu time to give it the
> >   chance to update whatever last-measure internal variable it may have.
> >
> >This approach is simple, but proved to work well for my test scenarios.
> >in a UP guest on UP host, with a cpu-hog in both guest and host shows
> >~ 50 % steal time. steal time is also accounted proportionally, if
> >nice values are given to the host cpu-hog.
> >
> >A cpu-hog in the host with no load in the guest, produces 0 % steal time,
> >with 100 % idle, as one would expect.
> >
> 
> The scheduler people and lkml need to be copied on this patch.
> 
> Since s390 does steal time (I think?), can this code be shared?
AFAIK, s390 enables CONFIG_VIRT_CPU_ACCOUNTING, so all timings
comes from the hypervisor, and statistical sampling is not involved.

We could do that, if our hardware had any method to say precisely
how much time we spent in each state, which I don't think we do.
So in a summary, s390 is in a totally different ifdef side.

Who should we copy at the scheduler side?

Re: [RFC 2/7] change headers preparing for steal time

[Glauber Costa]

On Sun, Aug 29, 2010 at 12:51:40PM +0300, Avi Kivity wrote:
>  On 08/26/2010 12:43 AM, Glauber Costa wrote:
> >This guest/host common patch prepares infrastructure for
> >the steal time implementation. Some constants are added,
> >and a name change happens in pvclock vcpu structure.
> >
> >Signed-off-by: Glauber Costa<glommer@redhat.com>
> >---
> >  arch/x86/include/asm/kvm_para.h    |    1 +
> >  arch/x86/include/asm/pvclock-abi.h |    4 +++-
> >  2 files changed, 4 insertions(+), 1 deletions(-)
> >
> >diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
> >index 05eba5e..1759c81 100644
> >--- a/arch/x86/include/asm/kvm_para.h
> >+++ b/arch/x86/include/asm/kvm_para.h
> >@@ -25,6 +25,7 @@
> >   * in pvclock structure. If no bits are set, all flags are ignored.
> >   */
> >  #define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT	24
> >+#define KVM_FEATURE_CLOCKSOURCE_STEAL_BIT 	25
> >
> >  #define MSR_KVM_WALL_CLOCK  0x11
> >  #define MSR_KVM_SYSTEM_TIME 0x12
> >diff --git a/arch/x86/include/asm/pvclock-abi.h b/arch/x86/include/asm/pvclock-abi.h
> >index 35f2d19..417061b 100644
> >--- a/arch/x86/include/asm/pvclock-abi.h
> >+++ b/arch/x86/include/asm/pvclock-abi.h
> >@@ -24,7 +24,7 @@
> >
> >  struct pvclock_vcpu_time_info {
> >  	u32   version;
> >-	u32   pad0;
> >+	u32   steal_time;
> >  	u64   tsc_timestamp;
> >  	u64   system_time;
> >  	u32   tsc_to_system_mul;
> >@@ -40,5 +40,7 @@ struct pvclock_wall_clock {
> >  } __attribute__((__packed__));
> >
> >  #define PVCLOCK_TSC_STABLE_BIT	(1<<  0)
> >+#define PVCLOCK_STEAL_BIT   (2<<  0)
> >+
> >  #endif /* __ASSEMBLY__ */
> >  #endif /* _ASM_X86_PVCLOCK_ABI_H */
> 
> We could extend pvclock to 64 bytes (unfortunately we didn't reserve
> bits 0-5 like we did with others) and use an nsec field.
Yes we could, but what for?
We're in a jiffie resolution on the other side anyway.

Re: [RFC 2/7] change headers preparing for steal time

[Avi Kivity]

  On 08/30/2010 03:44 PM, Glauber Costa wrote:
> @@ -40,5 +40,7 @@ struct pvclock_wall_clock {
>>>   } __attribute__((__packed__));
>>>
>>>   #define PVCLOCK_TSC_STABLE_BIT	(1<<   0)
>>> +#define PVCLOCK_STEAL_BIT   (2<<   0)
>>> +
>>>   #endif /* __ASSEMBLY__ */
>>>   #endif /* _ASM_X86_PVCLOCK_ABI_H */
>> We could extend pvclock to 64 bytes (unfortunately we didn't reserve
>> bits 0-5 like we did with others) and use an nsec field.
> Yes we could, but what for?
> We're in a jiffie resolution on the other side anyway.

"the other side" keeps changing (and is potentially not just Linux).  We 
need to be prepared for the future.

We'll regret having an interface that matches Linux at a random in time 
but nothing else.



-- 
error compiling committee.c: too many arguments to function

Re: [RFC 4/7] change kernel accounting to include steal time

[Avi Kivity]

  On 08/30/2010 03:42 PM, Glauber Costa wrote:
> On Sun, Aug 29, 2010 at 12:59:36PM +0300, Avi Kivity wrote:
>>   On 08/26/2010 12:43 AM, Glauber Costa wrote:
>>> This patch proposes a common steal time implementation. When no
>>> steal time is accounted, we just add a branch to the current
>>> accounting code, that shouldn't add much overhead.
>>>
>>> When we do want to register steal time, we proceed as following:
>>> - if we would account user or system time in this tick, and there is
>>>    out-of-cpu time registered, we skip it altogether, and account steal
>>>    time only.
>>> - if we would account user or system time in this tick, and we got the
>>>    cpu for the whole slice, we proceed normaly.
>>> - if we are idle in this tick, we flush out-of-cpu time to give it the
>>>    chance to update whatever last-measure internal variable it may have.
>>>
>>> This approach is simple, but proved to work well for my test scenarios.
>>> in a UP guest on UP host, with a cpu-hog in both guest and host shows
>>> ~ 50 % steal time. steal time is also accounted proportionally, if
>>> nice values are given to the host cpu-hog.
>>>
>>> A cpu-hog in the host with no load in the guest, produces 0 % steal time,
>>> with 100 % idle, as one would expect.
>>>
>> The scheduler people and lkml need to be copied on this patch.
>>
>> Since s390 does steal time (I think?), can this code be shared?
> AFAIK, s390 enables CONFIG_VIRT_CPU_ACCOUNTING, so all timings
> comes from the hypervisor, and statistical sampling is not involved.

Ok.  I see ppc does something similar as well (taking care of 
user/kernel transitions itself).

> We could do that, if our hardware had any method to say precisely
> how much time we spent in each state, which I don't think we do.

We don't, though I'm sure everyone is wondering why we can't have cheap 
accurate global clocks on x86.

> So in a summary, s390 is in a totally different ifdef side.

Yes.

> Who should we copy at the scheduler side?

 From MAINTAINERS:
   Ingo Molnar <mingo@elte.hu>
   Peter Zijlstra <peterz@infradead.org>

-- 
error compiling committee.c: too many arguments to function