Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1359389 > unrolled thread

Re: [PATCH 1/5] x86/kvm: On KVM re-enable (e.g. after suspend), update clocks

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/5] x86/kvm: On KVM re-enable (e.g. after suspend),  update clocks Radim Krcmar <rkrcmar@redhat.com> - 2016-03-16 23:10 +0100
    Re: [PATCH 1/5] x86/kvm: On KVM re-enable (e.g. after suspend),  update clocks Andy Lutomirski <luto@amacapital.net> - 2016-03-16 23:20 +0100
      Re: [PATCH 1/5] x86/kvm: On KVM re-enable (e.g. after suspend),  update clocks Radim Krcmar <rkrcmar@redhat.com> - 2016-03-17 00:00 +0100
        Re: [PATCH 1/5] x86/kvm: On KVM re-enable (e.g. after suspend),  update clocks Andy Lutomirski <luto@amacapital.net> - 2016-03-17 00:10 +0100
          Re: [PATCH 1/5] x86/kvm: On KVM re-enable (e.g. after suspend),  update clocks Radim Krcmar <rkrcmar@redhat.com> - 2016-03-17 16:20 +0100
            Re: [PATCH 1/5] x86/kvm: On KVM re-enable (e.g. after suspend),  update clocks Andy Lutomirski <luto@amacapital.net> - 2016-03-17 19:30 +0100
              Re: [PATCH 1/5] x86/kvm: On KVM re-enable (e.g. after suspend),  update clocks Radim Krcmar <rkrcmar@redhat.com> - 2016-03-17 21:00 +0100

#1359389 — Re: [PATCH 1/5] x86/kvm: On KVM re-enable (e.g. after suspend), update clocks

2015-12-09 15:12-0800, Andy Lutomirski:
> This gets rid of the "did TSC go backwards" logic and just updates
> all clocks.  It should work better (no more disabling of fast
> timing) and more reliably (all of the clocks are actually updated).
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> @@ -7369,88 +7366,22 @@ int kvm_arch_hardware_enable(void)
>  	list_for_each_entry(kvm, &vm_list, vm_list) {
>  		kvm_for_each_vcpu(i, vcpu, kvm) {
> +			if (vcpu->cpu == smp_processor_id()) {

(vmm_exclusive sets vcpu->cpu to -1, so KVM_REQ_MASTERCLOCK_UPDATE might
 not run, but vmm_exclusive probably doesn't work anyway.)

>  				kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
> +				kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE,
> +						vcpu);
>  			}

(Requesting KVM_REQ_MASTERCLOCK_UPDATE once per VM is enough.)

> -	if (backwards_tsc) {
> -		u64 delta_cyc = max_tsc - local_tsc;
> -		backwards_tsc_observed = true;
> -		list_for_each_entry(kvm, &vm_list, vm_list) {
> -			kvm_for_each_vcpu(i, vcpu, kvm) {
> -				vcpu->arch.tsc_offset_adjustment += delta_cyc;
> -				vcpu->arch.last_host_tsc = local_tsc;

tsc_offset_adjustment was set for

  	/* Apply any externally detected TSC adjustments (due to suspend) */
  	if (unlikely(vcpu->arch.tsc_offset_adjustment)) {
  		adjust_tsc_offset_host(vcpu, vcpu->arch.tsc_offset_adjustment);
  		vcpu->arch.tsc_offset_adjustment = 0;
  		kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
  	}

Guest TSC is going to jump backward with this patch, which would make
the guest think that a lot of cycles passed.  This has no bearing on
guest timekeeping, because the guest shouldn't be using raw TSC.
If we wanted to do something though, there are at least two options:
1) Fake that TSC continued at roughly its specified rate:  compute how
   many cycles could have elapsed while the CPU was suspended (using
   host time before/after suspend and guest TSC frequency) and adjust
   guest TSC.
2) Resume guest TSC at its last cycle before suspend.
   (Roughly what KVM does now.)

What are your opinions on TSC faking?

Thanks.


---
Btw. I'll be spending some days to decipher kvmclock, so I'd also fix
the masterclock+suspend issue, if you don't mind ... So far, I don't
even see a reason to update kvmclock on kvm_arch_hardware_enable().
Suspend is a condition that we want to handle, so kvm_resume would be a
better place, but we handle suspend only because TSC and timekeeping has
changed, so I think that the right place is in their event notifiers.

[toc] | [next] | [standalone]

#1359398

On Wed, Mar 16, 2016 at 3:06 PM, Radim Krcmar <rkrcmar@redhat.com> wrote:
> 2015-12-09 15:12-0800, Andy Lutomirski:
>> This gets rid of the "did TSC go backwards" logic and just updates
>> all clocks.  It should work better (no more disabling of fast
>> timing) and more reliably (all of the clocks are actually updated).
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> @@ -7369,88 +7366,22 @@ int kvm_arch_hardware_enable(void)
>>       list_for_each_entry(kvm, &vm_list, vm_list) {
>>               kvm_for_each_vcpu(i, vcpu, kvm) {
>> +                     if (vcpu->cpu == smp_processor_id()) {
>
> (vmm_exclusive sets vcpu->cpu to -1, so KVM_REQ_MASTERCLOCK_UPDATE might
>  not run, but vmm_exclusive probably doesn't work anyway.)
>
>>                               kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
>> +                             kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE,
>> +                                             vcpu);
>>                       }
>
> (Requesting KVM_REQ_MASTERCLOCK_UPDATE once per VM is enough.)
>
>> -     if (backwards_tsc) {
>> -             u64 delta_cyc = max_tsc - local_tsc;
>> -             backwards_tsc_observed = true;
>> -             list_for_each_entry(kvm, &vm_list, vm_list) {
>> -                     kvm_for_each_vcpu(i, vcpu, kvm) {
>> -                             vcpu->arch.tsc_offset_adjustment += delta_cyc;
>> -                             vcpu->arch.last_host_tsc = local_tsc;
>
> tsc_offset_adjustment was set for
>
>         /* Apply any externally detected TSC adjustments (due to suspend) */
>         if (unlikely(vcpu->arch.tsc_offset_adjustment)) {
>                 adjust_tsc_offset_host(vcpu, vcpu->arch.tsc_offset_adjustment);
>                 vcpu->arch.tsc_offset_adjustment = 0;
>                 kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
>         }
>
> Guest TSC is going to jump backward with this patch, which would make
> the guest think that a lot of cycles passed.  This has no bearing on
> guest timekeeping, because the guest shouldn't be using raw TSC.
> If we wanted to do something though, there are at least two options:
> 1) Fake that TSC continued at roughly its specified rate:  compute how
>    many cycles could have elapsed while the CPU was suspended (using
>    host time before/after suspend and guest TSC frequency) and adjust
>    guest TSC.
> 2) Resume guest TSC at its last cycle before suspend.
>    (Roughly what KVM does now.)
>
> What are your opinions on TSC faking?

I'd suggest restarting it wherever it left off, because it's simpler.
If there was a CLOCK_BOOT_RAW, you could try to track it, but I'm not
sure that such a thing exists.

FWIW, if you ever intend to support ART ("always running timer")
passthrough, this is going to be a giant clusterfsck.  Good luck.  I
haven't gotten a straight answer as to what hardware actually supports
that thing, so even testing isn't no easy.

>
> Thanks.
>
>
> ---
> Btw. I'll be spending some days to decipher kvmclock, so I'd also fix
> the masterclock+suspend issue, if you don't mind ... So far, I don't
> even see a reason to update kvmclock on kvm_arch_hardware_enable().
> Suspend is a condition that we want to handle, so kvm_resume would be a
> better place, but we handle suspend only because TSC and timekeeping has
> changed, so I think that the right place is in their event notifiers.

I'd be glad to try to review things.  Please cc me.

One of the Xen people pointed me at the MS Viridian spec for handling
TSC rate changes on migration to or from hosts that don't support TSC
scaling.  I wonder if KVM could use the same technique or even the
same API.

--Andy

[toc] | [prev] | [next] | [standalone]

#1359439

2016-03-16 15:15-0700, Andy Lutomirski:
> On Wed, Mar 16, 2016 at 3:06 PM, Radim Krcmar <rkrcmar@redhat.com> wrote:
>> Guest TSC is going to jump backward with this patch, which would make
>> the guest think that a lot of cycles passed.  This has no bearing on
>> guest timekeeping, because the guest shouldn't be using raw TSC.
>> If we wanted to do something though, there are at least two options:
>> 1) Fake that TSC continued at roughly its specified rate:  compute how
>>    many cycles could have elapsed while the CPU was suspended (using
>>    host time before/after suspend and guest TSC frequency) and adjust
>>    guest TSC.
>> 2) Resume guest TSC at its last cycle before suspend.
>>    (Roughly what KVM does now.)
>>
>> What are your opinions on TSC faking?
> 
> I'd suggest restarting it wherever it left off, because it's simpler.
> If there was a CLOCK_BOOT_RAW, you could try to track it, but I'm not
> sure that such a thing exists.

CLOCK_MONOTONIC_RAW can count in suspend, so CLOCK_BOOT_RAW would be a
conditional alias and it probably doesn't exist because of that.

> FWIW, if you ever intend to support ART ("always running timer")
> passthrough, this is going to be a giant clusterfsck.  Good luck.  I
> haven't gotten a straight answer as to what hardware actually supports
> that thing, so even testing isn't no easy.

Hm, AR TSC would be best handled by doing nothing ... dropping the
faking logic just became tempting.

>> ---
>> Btw. I'll be spending some days to decipher kvmclock, so I'd also fix
>> the masterclock+suspend issue, if you don't mind ... So far, I don't
>> even see a reason to update kvmclock on kvm_arch_hardware_enable().
>> Suspend is a condition that we want to handle, so kvm_resume would be a
>> better place, but we handle suspend only because TSC and timekeeping has
>> changed, so I think that the right place is in their event notifiers.
> 
> I'd be glad to try to review things.  Please cc me.

Ok.

> One of the Xen people pointed me at the MS Viridian spec for handling
> TSC rate changes on migration to or from hosts that don't support TSC
> scaling.  I wonder if KVM could use the same technique or even the
> same API.

The TSC frequency MSR is read-only in Xen, so I guess it's equivalent to
pvclock.  I'll take a deeper look, thanks for pointers.

[toc] | [prev] | [next] | [standalone]

#1359441

On Wed, Mar 16, 2016 at 3:59 PM, Radim Krcmar <rkrcmar@redhat.com> wrote:
> 2016-03-16 15:15-0700, Andy Lutomirski:
>> On Wed, Mar 16, 2016 at 3:06 PM, Radim Krcmar <rkrcmar@redhat.com> wrote:
>>> Guest TSC is going to jump backward with this patch, which would make
>>> the guest think that a lot of cycles passed.  This has no bearing on
>>> guest timekeeping, because the guest shouldn't be using raw TSC.
>>> If we wanted to do something though, there are at least two options:
>>> 1) Fake that TSC continued at roughly its specified rate:  compute how
>>>    many cycles could have elapsed while the CPU was suspended (using
>>>    host time before/after suspend and guest TSC frequency) and adjust
>>>    guest TSC.
>>> 2) Resume guest TSC at its last cycle before suspend.
>>>    (Roughly what KVM does now.)
>>>
>>> What are your opinions on TSC faking?
>>
>> I'd suggest restarting it wherever it left off, because it's simpler.
>> If there was a CLOCK_BOOT_RAW, you could try to track it, but I'm not
>> sure that such a thing exists.
>
> CLOCK_MONOTONIC_RAW can count in suspend, so CLOCK_BOOT_RAW would be a
> conditional alias and it probably doesn't exist because of that.
>
>> FWIW, if you ever intend to support ART ("always running timer")
>> passthrough, this is going to be a giant clusterfsck.  Good luck.  I
>> haven't gotten a straight answer as to what hardware actually supports
>> that thing, so even testing isn't no easy.
>
> Hm, AR TSC would be best handled by doing nothing ... dropping the
> faking logic just became tempting.

As it stands, ART is screwed if you adjust the VMCS's tsc offset.  But
I think it's also screwed if you migrate to a machine with a different
ratio of guest TSC ticks to host ART ticks or a different offset,
because the host isn't going to do the rdmsr every time it tries to
access the ART, so passing it through might require a paravirt
mechanism no matter what.

ISTM that, if KVM tries to keep the guest TSC monotonic across
migration, it should probably also keep it monotonic across host
suspend/resume.  After all, host suspend/resume is kind of like
migrating from the pre-suspend host to the post-resume host.  Maybe it
could even share code.

>
>>> ---
>>> Btw. I'll be spending some days to decipher kvmclock, so I'd also fix
>>> the masterclock+suspend issue, if you don't mind ... So far, I don't
>>> even see a reason to update kvmclock on kvm_arch_hardware_enable().
>>> Suspend is a condition that we want to handle, so kvm_resume would be a
>>> better place, but we handle suspend only because TSC and timekeeping has
>>> changed, so I think that the right place is in their event notifiers.
>>
>> I'd be glad to try to review things.  Please cc me.
>
> Ok.
>
>> One of the Xen people pointed me at the MS Viridian spec for handling
>> TSC rate changes on migration to or from hosts that don't support TSC
>> scaling.  I wonder if KVM could use the same technique or even the
>> same API.
>
> The TSC frequency MSR is read-only in Xen, so I guess it's equivalent to
> pvclock.  I'll take a deeper look, thanks for pointers.



-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [prev] | [next] | [standalone]

#1359942

2016-03-16 16:07-0700, Andy Lutomirski:
> On Wed, Mar 16, 2016 at 3:59 PM, Radim Krcmar <rkrcmar@redhat.com> wrote:
>> 2016-03-16 15:15-0700, Andy Lutomirski:
>>> FWIW, if you ever intend to support ART ("always running timer")
>>> passthrough, this is going to be a giant clusterfsck.  Good luck.  I
>>> haven't gotten a straight answer as to what hardware actually supports
>>> that thing, so even testing isn't no easy.
>>
>> Hm, AR TSC would be best handled by doing nothing ... dropping the
>> faking logic just became tempting.

ART is different from what I initially thought, it's the underlying
mechanism for invariant TSC and nothing more ...  we already forbid
migrations when the guest knows about invariant TSC, so we could do the
same and let ART be virtualized.  (Suspend has to be forbidden too.)

> As it stands, ART is screwed if you adjust the VMCS's tsc offset.  But

Luckily, assigning real hardware can prevent migration or suspend, so we
won't need to adjust the offset during runtime.  TSC is a generally
unmigratable device that just happens to live on the CPU.

(It would have been better to hide TSC capability from the guest and only
 use rdtsc for kvmclock if the guest wanted fancy features.)

> I think it's also screwed if you migrate to a machine with a different
> ratio of guest TSC ticks to host ART ticks or a different offset,
> because the host isn't going to do the rdmsr every time it tries to
> access the ART, so passing it through might require a paravirt
> mechanism no matter what.

It's almost certain that the other host will have a different offset,
which makes TSC unmigratable in software without even considering ART
or frequencies.  Well, KVM already emulates different TSC frequency, so
we could emulate ART without sinking much lower. :)

> ISTM that, if KVM tries to keep the guest TSC monotonic across
> migration, it should probably also keep it monotonic across host
> suspend/resume.

Yes, "Pausing" TSC during suspend or migration is one way of improving
the TSC estimate.  If we want to emulate ART, then the estimate is
noticeably lacking, because TSC and ART are defined by a simple
equation (SDM 2015-12, 17.14.4 Invariant Time-Keeping):
 TSC_Value = (ART_Value * CPUID.15H:EBX[31:0] )/ CPUID.15H:EAX[31:0] + K

where the guest thinks that CPUID and K are constant (between events
that the guest knows of), so we should give the best estimate of how
many TSC cycles have passed.  (The best estimate is still lacking.)

>                  After all, host suspend/resume is kind of like
> migrating from the pre-suspend host to the post-resume host.  Maybe it
> could even share code.

Hopefully ... host suspend/resume is driven by kernel and migration is
driven by userspace, which might complicate sharing.

[toc] | [prev] | [next] | [standalone]

#1360120

On Mar 17, 2016 8:10 AM, "Radim Krcmar" <rkrcmar@redhat.com> wrote:
>
> 2016-03-16 16:07-0700, Andy Lutomirski:
> > On Wed, Mar 16, 2016 at 3:59 PM, Radim Krcmar <rkrcmar@redhat.com> wrote:
> >> 2016-03-16 15:15-0700, Andy Lutomirski:
> >>> FWIW, if you ever intend to support ART ("always running timer")
> >>> passthrough, this is going to be a giant clusterfsck.  Good luck.  I
> >>> haven't gotten a straight answer as to what hardware actually supports
> >>> that thing, so even testing isn't no easy.
> >>
> >> Hm, AR TSC would be best handled by doing nothing ... dropping the
> >> faking logic just became tempting.
>
> ART is different from what I initially thought, it's the underlying
> mechanism for invariant TSC and nothing more ...  we already forbid
> migrations when the guest knows about invariant TSC, so we could do the
> same and let ART be virtualized.  (Suspend has to be forbidden too.)

It's more than that -- it's a TSC-like clock that can be read by PCIe devices.

>
> > As it stands, ART is screwed if you adjust the VMCS's tsc offset.  But
>
> Luckily, assigning real hardware can prevent migration or suspend, so we
> won't need to adjust the offset during runtime.  TSC is a generally
> unmigratable device that just happens to live on the CPU.
>
> (It would have been better to hide TSC capability from the guest and only
>  use rdtsc for kvmclock if the guest wanted fancy features.)
>

I think that, if KVM passes through an ART-supporting NIC, it might be
rather messy to try to avoid passing through TSC as well.  But maybe a
pvclock-like structure could expose the ART-kvmclock offset and scale.

> > I think it's also screwed if you migrate to a machine with a different
> > ratio of guest TSC ticks to host ART ticks or a different offset,
> > because the host isn't going to do the rdmsr every time it tries to
> > access the ART, so passing it through might require a paravirt
> > mechanism no matter what.
>
> It's almost certain that the other host will have a different offset,
> which makes TSC unmigratable in software without even considering ART
> or frequencies.  Well, KVM already emulates different TSC frequency, so
> we could emulate ART without sinking much lower. :)
>
> > ISTM that, if KVM tries to keep the guest TSC monotonic across
> > migration, it should probably also keep it monotonic across host
> > suspend/resume.
>
> Yes, "Pausing" TSC during suspend or migration is one way of improving
> the TSC estimate.  If we want to emulate ART, then the estimate is
> noticeably lacking, because TSC and ART are defined by a simple
> equation (SDM 2015-12, 17.14.4 Invariant Time-Keeping):
>  TSC_Value = (ART_Value * CPUID.15H:EBX[31:0] )/ CPUID.15H:EAX[31:0] + K
>
> where the guest thinks that CPUID and K are constant (between events
> that the guest knows of), so we should give the best estimate of how
> many TSC cycles have passed.  (The best estimate is still lacking.)
>
> >                  After all, host suspend/resume is kind of like
> > migrating from the pre-suspend host to the post-resume host.  Maybe it
> > could even share code.
>
> Hopefully ... host suspend/resume is driven by kernel and migration is
> driven by userspace, which might complicate sharing.

Good point.

--Andy

[toc] | [prev] | [next] | [standalone]

#1360150

2016-03-17 11:22-0700, Andy Lutomirski:
> On Mar 17, 2016 8:10 AM, "Radim Krcmar" <rkrcmar@redhat.com> wrote:
>> 2016-03-16 16:07-0700, Andy Lutomirski:
>>> On Wed, Mar 16, 2016 at 3:59 PM, Radim Krcmar <rkrcmar@redhat.com> wrote:
>>>> 2016-03-16 15:15-0700, Andy Lutomirski:
>>>>> FWIW, if you ever intend to support ART ("always running timer")
>>>>> passthrough, this is going to be a giant clusterfsck.  Good luck.  I
>>>>> haven't gotten a straight answer as to what hardware actually supports
>>>>> that thing, so even testing isn't no easy.
>>>>
>>>> Hm, AR TSC would be best handled by doing nothing ... dropping the
>>>> faking logic just became tempting.
>>
>> ART is different from what I initially thought, it's the underlying
>> mechanism for invariant TSC and nothing more ...  we already forbid
>> migrations when the guest knows about invariant TSC, so we could do the
>> same and let ART be virtualized.  (Suspend has to be forbidden too.)
> 
> It's more than that -- it's a TSC-like clock that can be read by PCIe devices.

So ART is for time synchronization within the machine.  Makes sense now.

>>> As it stands, ART is screwed if you adjust the VMCS's tsc offset.  But
>>
>> Luckily, assigning real hardware can prevent migration or suspend, so we
>> won't need to adjust the offset during runtime.  TSC is a generally
>> unmigratable device that just happens to live on the CPU.
>>
>> (It would have been better to hide TSC capability from the guest and only
>>  use rdtsc for kvmclock if the guest wanted fancy features.)
>>
> 
> I think that, if KVM passes through an ART-supporting NIC, it might be
> rather messy to try to avoid passing through TSC as well.

I agree.  Migrating a guest with ART-supporting NIC is going to be hard
or impossible, so there is no big drawback in exposing TSC.

If KVM adds host TSC_ADJUST and VMCS TSC-offset to guest TSC_ADJUST,
then ART-supporting NIC should use timestamps compatible with VCPUs.

>                                                            But maybe a
> pvclock-like structure could expose the ART-kvmclock offset and scale.

I think that getting ART from kvmclock would turn out to be horrible.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web