On Fri, Aug 23, 2024 at 2:25 PM Chao Gao <chao.gao@xxxxxxxxx> wrote: > > On Fri, Aug 23, 2024 at 01:17:31PM +0900, Suleiman Souhlal wrote: > >On Wed, Aug 21, 2024 at 3:31 PM Chao Gao <chao.gao@xxxxxxxxx> wrote: > >> > >> On Tue, Aug 20, 2024 at 01:35:42PM +0900, Suleiman Souhlal wrote: > >> >When the host resumes from a suspend, the guest thinks any task > >> >that was running during the suspend ran for a long time, even though > >> >the effective run time was much shorter, which can end up having > >> >negative effects with scheduling. This can be particularly noticeable > >> >if the guest task was RT, as it can end up getting throttled for a > >> >long time. > >> > > >> >To mitigate this issue, we include the time that the host was > >> >suspended in steal time, which lets the guest subtract the duration from > >> >the tasks' runtime. > >> > > >> >Note that the case of a suspend happening during a VM migration > >> >might not be accounted. > >> > > >> >Signed-off-by: Suleiman Souhlal <suleiman@xxxxxxxxxx> > >> >--- > >> > arch/x86/include/asm/kvm_host.h | 1 + > >> > arch/x86/kvm/x86.c | 11 ++++++++++- > >> > 2 files changed, 11 insertions(+), 1 deletion(-) > >> > > >> >diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > >> >index 4a68cb3eba78f8..728798decb6d12 100644 > >> >--- a/arch/x86/include/asm/kvm_host.h > >> >+++ b/arch/x86/include/asm/kvm_host.h > >> >@@ -898,6 +898,7 @@ struct kvm_vcpu_arch { > >> > u8 preempted; > >> > u64 msr_val; > >> > u64 last_steal; > >> >+ u64 last_suspend_ns; > >> > struct gfn_to_hva_cache cache; > >> > } st; > >> > > >> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > >> >index 70219e4069874a..104f3d318026fa 100644 > >> >--- a/arch/x86/kvm/x86.c > >> >+++ b/arch/x86/kvm/x86.c > >> >@@ -3654,7 +3654,7 @@ static void record_steal_time(struct kvm_vcpu *vcpu) > >> > struct kvm_steal_time __user *st; > >> > struct kvm_memslots *slots; > >> > gpa_t gpa = vcpu->arch.st.msr_val & KVM_STEAL_VALID_BITS; > >> >- u64 steal; > >> >+ u64 steal, suspend_ns; > >> > u32 version; > >> > > >> > if (kvm_xen_msr_enabled(vcpu->kvm)) { > >> >@@ -3735,6 +3735,14 @@ static void record_steal_time(struct kvm_vcpu *vcpu) > >> > steal += current->sched_info.run_delay - > >> > vcpu->arch.st.last_steal; > >> > vcpu->arch.st.last_steal = current->sched_info.run_delay; > >> >+ /* > >> >+ * Include the time that the host was suspended in steal time. > >> >+ * Note that the case of a suspend happening during a VM migration > >> >+ * might not be accounted. > >> >+ */ > >> >+ suspend_ns = kvm_total_suspend_ns(); > >> >+ steal += suspend_ns - vcpu->arch.st.last_suspend_ns; > >> >+ vcpu->arch.st.last_suspend_ns = suspend_ns; > >> > >> The document in patch 3 states: > >> > >> Time during which the vcpu is idle, will not be reported as steal time > >> > >> I'm wondering if all host suspend time should be reported as steal time, > >> or if the suspend time during a vCPU halt should be excluded. > > > >I think the statement about idle time not being reported as steal isn't > >completely accurate, so I'm not sure if it's worth the extra complexity. > > > >> > >> > unsafe_put_user(steal, &st->steal, out); > >> > > >> > version += 1; > >> >@@ -12280,6 +12288,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) > >> > > >> > vcpu->arch.arch_capabilities = kvm_get_arch_capabilities(); > >> > vcpu->arch.msr_platform_info = MSR_PLATFORM_INFO_CPUID_FAULT; > >> >+ vcpu->arch.st.last_suspend_ns = kvm_total_suspend_ns(); > >> > >> is this necessary? I doubt this because KVM doesn't capture > >> current->sched_info.run_delay here. > > > >Isn't run_delay being captured by the scheduler at all time? > > I meant KVM doesn't do: > > vcpu->arch.st.last_steal = current->sched_info.run_delay; > > at vCPU creation time. I think for run_delay it's different because run_delay is a time difference. It's something that gets added to steal, not relative to the previous steal value.