Re: [PATCH v7 5/6] KVM: x86: Refactor tsc synchronization code

On Thu, Sep 2, 2021 at 12:21 PM Sean Christopherson [off-list ref] wrote:

On Mon, Aug 16, 2021, Oliver Upton wrote:

quoted

Refactor kvm_synchronize_tsc to make a new function that allows callers
to specify TSC parameters (offset, value, nanoseconds, etc.) explicitly
for the sake of participating in TSC synchronization.

Signed-off-by: Oliver Upton <redacted>
---
+     struct kvm *kvm = vcpu->kvm;
+     bool already_matched;
+
+     lockdep_assert_held(&kvm->arch.tsc_write_lock);
+
+     already_matched =
+            (vcpu->arch.this_tsc_generation == kvm->arch.cur_tsc_generation);
+

...

quoted

+     if (!matched) {
+             /*
+              * We split periods of matched TSC writes into generations.
+              * For each generation, we track the original measured
+              * nanosecond time, offset, and write, so if TSCs are in
+              * sync, we can match exact offset, and if not, we can match
+              * exact software computation in compute_guest_tsc()
+              *
+              * These values are tracked in kvm->arch.cur_xxx variables.
+              */
+             kvm->arch.cur_tsc_generation++;
+             kvm->arch.cur_tsc_nsec = ns;
+             kvm->arch.cur_tsc_write = tsc;
+             kvm->arch.cur_tsc_offset = offset;
+
+             spin_lock(&kvm->arch.pvclock_gtod_sync_lock);
+             kvm->arch.nr_vcpus_matched_tsc = 0;
+     } else if (!already_matched) {
+             spin_lock(&kvm->arch.pvclock_gtod_sync_lock);
+             kvm->arch.nr_vcpus_matched_tsc++;
+     }
+
+     kvm_track_tsc_matching(vcpu);
+     spin_unlock(&kvm->arch.pvclock_gtod_sync_lock);

This unlock is imbalanced if matched and already_matched are both true.  It's not
immediately obvious that that _can't_ happen, and if it truly can't happen then
conditionally locking is pointless (because it's not actually conditional).

The previous code took the lock unconditionally, I don't see a strong argument
to change that, e.g. holding it for a few extra cycles while kvm->arch.cur_tsc_*
are updated is unlikely to be noticable.

We may have gone full circle here :-) You had said it was confusing to
hold the lock when updating kvm->arch.cur_tsc_* a while back. I do
still agree with that sentiment, but the conditional locking is odd.

If you really want to delay taking the locking, you could do

        if (!matched) {
                kvm->arch.cur_tsc_generation++;
                kvm->arch.cur_tsc_nsec = ns;
                kvm->arch.cur_tsc_write = data;
                kvm->arch.cur_tsc_offset = offset;
        }

        spin_lock(&kvm->arch.pvclock_gtod_sync_lock);
        if (!matched)
                kvm->arch.nr_vcpus_matched_tsc = 0;
        else if (!already_matched)
                kvm->arch.nr_vcpus_matched_tsc++;
        spin_unlock(&kvm->arch.pvclock_gtod_sync_lock);

This seems the most readable, making it clear what is guarded and what
is not. I'll probably go this route.

--
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel