All of lore.kernel.org
 help / color / mirror / Atom feed
From: Paolo Bonzini <pbonzini@redhat.com>
To: Sean Christopherson <seanjc@google.com>,
	Oliver Upton <oupton@google.com>
Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu,
	Marc Zyngier <maz@kernel.org>, Peter Shier <pshier@google.com>,
	Jim Mattson <jmattson@google.com>,
	David Matlack <dmatlack@google.com>,
	Ricardo Koller <ricarkol@google.com>,
	Jing Zhang <jingzhangos@google.com>,
	Raghavendra Rao Anata <rananta@google.com>,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	Andrew Jones <drjones@redhat.com>, Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>
Subject: Re: [PATCH v7 5/6] KVM: x86: Refactor tsc synchronization code
Date: Fri, 24 Sep 2021 11:28:28 +0200	[thread overview]
Message-ID: <de3ff2de-da79-fd0c-c90b-f767414b0b69@redhat.com> (raw)
In-Reply-To: <YTEkRfTFyoh+HQyT@google.com>

On 02/09/21 21:21, Sean Christopherson wrote:
> 
>> +	if (!matched) {
>> +		...
>> +		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).

This is IMO another reason to unify tsc_write_lock and 
pvclock_gtod_sync_lock.  The chances of contention are pretty slim.  As 
soon as I sort out the next -rc3 pull request I'll send out my version 
of Oliver's patches.

Thanks,

Paolo


WARNING: multiple messages have this Message-ID (diff)
From: Paolo Bonzini <pbonzini@redhat.com>
To: Sean Christopherson <seanjc@google.com>,
	Oliver Upton <oupton@google.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	kvm@vger.kernel.org, Will Deacon <will@kernel.org>,
	Marc Zyngier <maz@kernel.org>, Peter Shier <pshier@google.com>,
	David Matlack <dmatlack@google.com>,
	kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org,
	Jim Mattson <jmattson@google.com>
Subject: Re: [PATCH v7 5/6] KVM: x86: Refactor tsc synchronization code
Date: Fri, 24 Sep 2021 11:28:28 +0200	[thread overview]
Message-ID: <de3ff2de-da79-fd0c-c90b-f767414b0b69@redhat.com> (raw)
In-Reply-To: <YTEkRfTFyoh+HQyT@google.com>

On 02/09/21 21:21, Sean Christopherson wrote:
> 
>> +	if (!matched) {
>> +		...
>> +		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).

This is IMO another reason to unify tsc_write_lock and 
pvclock_gtod_sync_lock.  The chances of contention are pretty slim.  As 
soon as I sort out the next -rc3 pull request I'll send out my version 
of Oliver's patches.

Thanks,

Paolo

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

WARNING: multiple messages have this Message-ID (diff)
From: Paolo Bonzini <pbonzini@redhat.com>
To: Sean Christopherson <seanjc@google.com>,
	Oliver Upton <oupton@google.com>
Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu,
	Marc Zyngier <maz@kernel.org>, Peter Shier <pshier@google.com>,
	Jim Mattson <jmattson@google.com>,
	David Matlack <dmatlack@google.com>,
	Ricardo Koller <ricarkol@google.com>,
	Jing Zhang <jingzhangos@google.com>,
	Raghavendra Rao Anata <rananta@google.com>,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	Andrew Jones <drjones@redhat.com>, Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>
Subject: Re: [PATCH v7 5/6] KVM: x86: Refactor tsc synchronization code
Date: Fri, 24 Sep 2021 11:28:28 +0200	[thread overview]
Message-ID: <de3ff2de-da79-fd0c-c90b-f767414b0b69@redhat.com> (raw)
In-Reply-To: <YTEkRfTFyoh+HQyT@google.com>

On 02/09/21 21:21, Sean Christopherson wrote:
> 
>> +	if (!matched) {
>> +		...
>> +		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).

This is IMO another reason to unify tsc_write_lock and 
pvclock_gtod_sync_lock.  The chances of contention are pretty slim.  As 
soon as I sort out the next -rc3 pull request I'll send out my version 
of Oliver's patches.

Thanks,

Paolo


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

  parent reply	other threads:[~2021-09-24  9:28 UTC|newest]

Thread overview: 61+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-16  0:11 [PATCH v7 0/6] KVM: x86: Add idempotent controls for migrating system counter state Oliver Upton
2021-08-16  0:11 ` Oliver Upton
2021-08-16  0:11 ` Oliver Upton
2021-08-16  0:11 ` [PATCH v7 1/6] KVM: x86: Fix potential race in KVM_GET_CLOCK Oliver Upton
2021-08-16  0:11   ` Oliver Upton
2021-08-16  0:11   ` Oliver Upton
2021-08-19 18:24   ` Marcelo Tosatti
2021-08-19 18:24     ` Marcelo Tosatti
2021-08-19 18:24     ` Marcelo Tosatti
2021-08-20 18:22     ` Oliver Upton
2021-08-20 18:22       ` Oliver Upton
2021-08-20 18:22       ` Oliver Upton
2021-08-16  0:11 ` [PATCH v7 2/6] KVM: x86: Create helper methods for KVM_{GET,SET}_CLOCK ioctls Oliver Upton
2021-08-16  0:11   ` Oliver Upton
2021-08-16  0:11   ` Oliver Upton
2021-08-16  0:11 ` [PATCH v7 3/6] KVM: x86: Report host tsc and realtime values in KVM_GET_CLOCK Oliver Upton
2021-08-16  0:11   ` Oliver Upton
2021-08-16  0:11   ` Oliver Upton
2021-08-20 12:46   ` Marcelo Tosatti
2021-08-20 12:46     ` Marcelo Tosatti
2021-08-20 12:46     ` Marcelo Tosatti
2021-09-24  8:30     ` Paolo Bonzini
2021-09-24  8:30       ` Paolo Bonzini
2021-09-24  8:30       ` Paolo Bonzini
2021-08-16  0:11 ` [PATCH v7 4/6] KVM: x86: Take the pvclock sync lock behind the tsc_write_lock Oliver Upton
2021-08-16  0:11   ` Oliver Upton
2021-08-16  0:11   ` Oliver Upton
2021-09-02 19:22   ` Sean Christopherson
2021-09-02 19:22     ` Sean Christopherson
2021-09-02 19:22     ` Sean Christopherson
2021-08-16  0:11 ` [PATCH v7 5/6] KVM: x86: Refactor tsc synchronization code Oliver Upton
2021-08-16  0:11   ` Oliver Upton
2021-08-16  0:11   ` Oliver Upton
2021-09-02 19:21   ` Sean Christopherson
2021-09-02 19:21     ` Sean Christopherson
2021-09-02 19:21     ` Sean Christopherson
2021-09-02 19:41     ` Oliver Upton
2021-09-02 19:41       ` Oliver Upton
2021-09-02 19:41       ` Oliver Upton
2021-09-24  9:28     ` Paolo Bonzini [this message]
2021-09-24  9:28       ` Paolo Bonzini
2021-09-24  9:28       ` Paolo Bonzini
2021-08-16  0:11 ` [PATCH v7 6/6] KVM: x86: Expose TSC offset controls to userspace Oliver Upton
2021-08-16  0:11   ` Oliver Upton
2021-08-16  0:11   ` Oliver Upton
2021-08-23 20:56   ` Oliver Upton
2021-08-23 20:56     ` Oliver Upton
2021-08-23 20:56     ` Oliver Upton
2021-08-26 12:48     ` Marcelo Tosatti
2021-08-26 12:48       ` Marcelo Tosatti
2021-08-26 12:48       ` Marcelo Tosatti
2021-08-26 20:27       ` Oliver Upton
2021-08-26 20:27         ` Oliver Upton
2021-08-26 20:27         ` Oliver Upton
2021-08-29  8:25         ` Paolo Bonzini
2021-09-02 19:23 ` [PATCH v7 0/6] KVM: x86: Add idempotent controls for migrating system counter state Sean Christopherson
2021-09-02 19:23   ` Sean Christopherson
2021-09-02 19:23   ` Sean Christopherson
2021-09-02 19:45   ` Oliver Upton
2021-09-02 19:45     ` Oliver Upton
2021-09-02 19:45     ` Oliver Upton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=de3ff2de-da79-fd0c-c90b-f767414b0b69@redhat.com \
    --to=pbonzini@redhat.com \
    --cc=alexandru.elisei@arm.com \
    --cc=catalin.marinas@arm.com \
    --cc=dmatlack@google.com \
    --cc=drjones@redhat.com \
    --cc=james.morse@arm.com \
    --cc=jingzhangos@google.com \
    --cc=jmattson@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=maz@kernel.org \
    --cc=oupton@google.com \
    --cc=pshier@google.com \
    --cc=rananta@google.com \
    --cc=ricarkol@google.com \
    --cc=seanjc@google.com \
    --cc=suzuki.poulose@arm.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.