Hi Marcelo,
On Thu, Jun 29, 2017 at 5:59 PM, Marcelo Tosatti <mtosatti@xxxxxxxxxx> wrote:
> Hi Ladi,
>
> Can you disable masterclock if suspend-to-RAM is performed, in
> case backwards_tsc_observed was observed for any VM ?
> (which indicates its likely to happen again, backwards TSC).
Not quite sure I understand the question. High-level, I am not content
with the lasting effect of backwards_tsc_observed. If it's happened in
the past, it's likely to happen again, sure, but presumably the way we
handle it is correct, so it's not clear what remembering the flag buys
us. Having KVM behave differently by rmmod/insmod'ing it again, to
make it drop some host-wide state, is just ugly.
> Because a backwards TSC event, with masterclock in place, is likely
> to cause the guest to see time going backwards.
Got it. Hence the reasoning that guests whose time hasn't started
because they don't exist yet should be unaffected.
> What masterclock requires is that TSCs are:
>
> 1) In sync across CPUs and TSC clocksource is in use by the host.
> 2) kvmclock regions updated on VM-entry, after a suspend from RAM (in case it
> goes backwards): kvm_gen_update_masterclock.
>
> Is the first condition met?
Yes, the first condition is met. What happens after resume is that in
pvclock_update_vm_gtod_copy when assigning to ka->use_master_clock,
all conditions except for !backwards_tsc_observed are met.
And because ka->use_master_clock is false, vcpu->hv_clock.flags
doesn't have the PVCLOCK_TSC_STABLE_BIT bit set (per the logic in
kvm_guest_time_update) and compute_tsc_page_parameters completely
gives up on updating the TSC page.
The next thing to look into was going to be what's wrong with this
chain of events. TSC jumping backwards is something we should be able
to compensate for in the TSC page so it's not clear why don't do it.
Maybe I started from the wrong end :/
Thanks!
Ladi
> On Mon, Jun 26, 2017 at 09:56:43AM +0200, Ladi Prosek wrote:
>> The backwards_tsc_observed global introduced in commit 16a9602 is never
>> reset to false. If a VM happens to be running while the host is suspended
>> (a common source of the TSC jumping backwards), master clock will never
>> be enabled again for any VM. In contrast, if no VM is running while the
>> host is suspended, master clock is unaffected. This is inconsistent and
>> unnecessarily strict. Let's track the backwards_tsc_observed variable
>> separately and let each VM start with a clean slate.
>>
>> Real world impact: My Windows VMs get slower after my laptop undergoes a
>> suspend/resume cycle. The only way to get the perf back is unloading and
>> reloading the kvm module.
>>
>> Signed-off-by: Ladi Prosek <lprosek@xxxxxxxxxx>
>> ---
>> arch/x86/include/asm/kvm_host.h | 1 +
>> arch/x86/kvm/x86.c | 6 ++----
>> 2 files changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 695605e..d8259c3 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -799,6 +799,7 @@ struct kvm_arch {
>> int audit_point;
>> #endif
>>
>> + bool backwards_tsc_observed;
>> bool boot_vcpu_runs_old_kvmclock;
>> u32 bsp_vcpu_id;
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 87d3cb9..8586ec6 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -134,8 +134,6 @@ module_param(lapic_timer_advance_ns, uint, S_IRUGO | S_IWUSR);
>> static bool __read_mostly vector_hashing = true;
>> module_param(vector_hashing, bool, S_IRUGO);
>>
>> -static bool __read_mostly backwards_tsc_observed = false;
>> -
>> #define KVM_NR_SHARED_MSRS 16
>>
>> struct kvm_shared_msrs_global {
>> @@ -1719,7 +1717,7 @@ static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
>> &ka->master_cycle_now);
>>
>> ka->use_master_clock = host_tsc_clocksource && vcpus_matched
>> - && !backwards_tsc_observed
>> + && !ka->backwards_tsc_observed
>> && !ka->boot_vcpu_runs_old_kvmclock;
>>
>> if (ka->use_master_clock)
>> @@ -7827,8 +7825,8 @@ int kvm_arch_hardware_enable(void)
>> */
>> if (backwards_tsc) {
>> u64 delta_cyc = max_tsc - local_tsc;
>> - backwards_tsc_observed = true;
>> list_for_each_entry(kvm, &vm_list, vm_list) {
>> + kvm->arch.backwards_tsc_observed = true;
>> kvm_for_each_vcpu(i, vcpu, kvm) {
>> vcpu->arch.tsc_offset_adjustment += delta_cyc;
>> vcpu->arch.last_host_tsc = local_tsc;
>> --
>> 2.9.3
