| Message ID | 20141103211627.GA19724@amt.cnet (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Extending the context we have: > if (vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC) > if (!ka->use_master_clock) > do_request = 1; > > - if (!vcpus_matched && ka->use_master_clock) > + if (ka->use_master_clock) > do_request = 1; > > if (do_request) > kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu); The patch also makes the previous "if (!ka->use_master_clock)" redundant. If you enter the first "if", do_request will be 1 independent of ka->use_master_clock. So you should also drop that one, and possibly rewrite it simply like this: if ((vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC) || ka->use_master_clock) kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu); But this brings the question: what is vclock_mode in your case? If it is VCLOCK_TSC, are you sure that the bug is fixed because you modified the second "if", or could it be fixed also by removing instead the "if (!ka->use_master_clock)"? This would leave the optimization in the case "!vcpus_matched && ka->use_master_clock". Or is the optimization always invalid? A different way to state the same question: can you explain the resulting condition ((vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC) || ka->use_master_clock) ? Please add a comment to kvm_track_tsc_matching that clarifies this logic. Paolo -- 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
On Tue, Nov 04, 2014 at 06:10:34PM +0100, Paolo Bonzini wrote: > Extending the context we have: > > if (vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC) > > if (!ka->use_master_clock) > > do_request = 1; > > > > - if (!vcpus_matched && ka->use_master_clock) > > + if (ka->use_master_clock) > > do_request = 1; > > > > if (do_request) > > kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu); > > The patch also makes the previous "if (!ka->use_master_clock)" redundant. > If you enter the first "if", do_request will be 1 independent of > ka->use_master_clock. So you should also drop that one, and possibly > rewrite it simply like this: > > if ((vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC) || > ka->use_master_clock) > kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu); > > But this brings the question: what is vclock_mode in your case? If it > is VCLOCK_TSC, are you sure that the bug is fixed because you modified > the second "if", or could it be fixed also by removing instead the > "if (!ka->use_master_clock)"? This would leave the optimization in the > case "!vcpus_matched && ka->use_master_clock". Or is the optimization > always invalid? The bug is fixed by always updating masterclock values, when it is enabled. > A different way to state the same question: can you explain the > resulting condition > > ((vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC) || > ka->use_master_clock) > > ? Please add a comment to kvm_track_tsc_matching that clarifies this > logic. > > Paolo Sure, sending v2. -- 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
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 0033df3..f52a887 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1248,7 +1248,7 @@ void kvm_track_tsc_matching(struct kvm_vcpu *vcpu) if (!ka->use_master_clock) do_request = 1; - if (!vcpus_matched && ka->use_master_clock) + if (ka->use_master_clock) do_request = 1; if (do_request)
When the guest writes to the TSC, the masterclock TSC copy must be updated as well along with the TSC_OFFSET update, otherwise a negative tsc_timestamp is calculated at kvm_guest_time_update. Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> -- 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