Received-SPF: pass (google.com: domain of linux-kernel+bounces-54236-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 6 Feb 2024 10:38:06 +0800
From: Yan Zhao <yan.y.zhao@intel.com>
To: Sean Christopherson <seanjc@google.com>
CC: Paolo Bonzini <pbonzini@redhat.com>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, Kai Huang <kai.huang@intel.com>, Yuan Yao
	<yuan.yao@linux.intel.com>, Xu Yilun <yilun.xu@linux.intel.com>
Subject: Re: [PATCH v3] KVM: x86/mmu: Retry fault before acquiring mmu_lock
 if mapping is changing
Message-ID: <ZcGbjstPnwVpR3Jw@yzhao56-desk.sh.intel.com>
Reply-To: Yan Zhao <yan.y.zhao@intel.com>
References: <20240203003518.387220-1-seanjc@google.com>
 <ZcAoZ/uZqJHFNfLC@yzhao56-desk.sh.intel.com>
 <ZcEolLQYSlLEVslN@google.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZcEolLQYSlLEVslN@google.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?H0aWigfFC3t4X3CeztGo6JJOQCemQAOh8iaN8oKE5RGZfuy7wNp4a7FCHebI?=
 =?us-ascii?Q?yGmNfw/3qC+J75FTnBhr6FzOJbjNZjdJw7PKYWrE+DESQoBWCqKHqxsGBDPP?=
 =?us-ascii?Q?wX7mS4Qoz8b1gV3TLoa7dFfi3LEO0XXBHTFek/+b/uJeLhhuJZkfcZ6hhPB+?=
 =?us-ascii?Q?3TKYEFu1iE75MIpc3rg9GTPLcQeLZUYrykUox2pwbkjiAOy3Qn7tSHiVK7GA?=
 =?us-ascii?Q?Y1BqUALPEVhQ6MGPNREtFnG2dG523AamFuS/T5mT93503l3J23vfenk96VtK?=
 =?us-ascii?Q?k6v+LH8UCg/2ZsuR3jjtn/GPNfKmB2920uIswmU3kESdNI0jcbe29cgedDpi?=
 =?us-ascii?Q?N7i7cAx15qicDIK+gRPBwqSCK6qNTUuVZUBP0f0SXaZrUhlDycsZioXaebXL?=
 =?us-ascii?Q?AigeIUqv0gjQMBFfJbnH1POr5GGdsgcvnRyghfl7KvOtCCkqujGwkHUm32uF?=
 =?us-ascii?Q?QCfIYeTz4BswzEb7OA3od/HtK4W6mURvyFYVzKeMhffG7KZnRAGPDR8qRkP5?=
 =?us-ascii?Q?uoLKBVkCDBjE75Qj3ZYdCDNS0hIhmE2Ns7l/0qka8uT9MATK31Z5h3QgPVi1?=
 =?us-ascii?Q?KeGmflJA1+avE8d6YvqMsbgzIqR41PICkFyzqLJN4zrpwYVjxw9vQbdeikxG?=
 =?us-ascii?Q?FY2WpKZ/lc0mKFw/3bdFi/Xxc9PPRfp1lVJxCgAGqhDOCbSH37FQJI+74n8l?=
 =?us-ascii?Q?jVEuBk3N1WXnx5iT0hct1/Rb5/ynIwytKBz11TgvDphX6FZvFjObIq3uK2Gm?=
 =?us-ascii?Q?4UmFCdo7GFmiE6oYuVgNuwH4m1UndSEvqs0+5wmXerSJpNTmjSDfpu+zVoYw?=
 =?us-ascii?Q?7RW0SUhMN6393AGxwTIZC+7H6TBJjJZfN3knFYF0vD42rci+2MbtL+Ftr9Au?=
 =?us-ascii?Q?cJlwwIoijL5O90Lu4kCaGOXUv5iJh39JldtbUq12lyOf8RKnMVRD936XkdAE?=
 =?us-ascii?Q?Dso7Q5HQ6mBanFREkMSX1Zv/wlY5KJu/oi1drrHWmKTrwbZvFoWC30ZfkFdi?=
 =?us-ascii?Q?4uOhIsHVF9wxwMqKOj3Ha1y9Zq6mITMH6H6zCNPUoalI2iZW/2Q8l5vS24Np?=
 =?us-ascii?Q?QahVMS5m3Vb6TUQLi4dgZmovv4NLHul88uN16lMkGiFv1MOP/1ak4l8r2K5a?=
 =?us-ascii?Q?cO0oJwap/FgRK5vCuJURmDn+vVrLjmQAcD7Vsj+O928RhqSER/0e883gOUZ4?=
 =?us-ascii?Q?Fc21NY3bZV88QqVMXeewaLrb88pwpLOMyTg8AaVYnxuQuTAi4dDvXJTaCh8+?=
 =?us-ascii?Q?779ToH3bolJUys5fg8gESxn2I6pqKxuSiETFWmBhijue7QFGOuuAmAaU3su5?=
 =?us-ascii?Q?b82fgac5lgMo8w2MAvcN8jrAGda95nonGe84d3DCq3PKY7SdYkSzpJ16jDgM?=
 =?us-ascii?Q?AbgFJhKs8wn9djI/jHF/ylowblcZGEOIlPs0reH9nTGofd8vXQR6QNfxZQjF?=
 =?us-ascii?Q?Fks88fAAGG5boOctfLD2UDi9XA7+sU10PmRDarQhrg6uj/z0oIbcDRATlU5j?=
 =?us-ascii?Q?H0o+lFvshQCFmgFiCBMSODM8eQ2xOOmVXjHPrCKPzHGyz1DDiy1EvqG+2mkQ?=
 =?us-ascii?Q?rDV4JXzayoaJwgQvuzfT9yGk/Wt5npuyCfrbQF08?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 18497376-e06a-4807-ab9b-08dc26c0c733
X-MS-Exchange-CrossTenant-AuthSource: DS7PR11MB5966.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Feb 2024 03:07:39.8332
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Gq96Cpi1NLoAv80CdvsXW7k5VtSUfSSgnQHtHkzMlcjrP2LEilpRIRwxkw3IJEUWXwKh5emLJvDwuJIA+g+e7g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB8095
X-OriginatorOrg: intel.com

On Mon, Feb 05, 2024 at 10:27:32AM -0800, Sean Christopherson wrote:
> On Mon, Feb 05, 2024, Yan Zhao wrote:
> > On Fri, Feb 02, 2024 at 04:35:18PM -0800, Sean Christopherson wrote:
> > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > > index 3c193b096b45..8ce9898914f1 100644
> > > --- a/arch/x86/kvm/mmu/mmu.c
> > > +++ b/arch/x86/kvm/mmu/mmu.c
> > > @@ -4415,6 +4415,25 @@ static int kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault,
> > >  	if (unlikely(!fault->slot))
> > >  		return kvm_handle_noslot_fault(vcpu, fault, access);
> > >  
> > > +	/*
> > > +	 * Pre-check for a relevant mmu_notifier invalidation event prior to
> > > +	 * acquiring mmu_lock.  If there is an in-progress invalidation and the
> > > +	 * kernel allows preemption, the invalidation task may drop mmu_lock
> > > +	 * and yield in response to mmu_lock being contended, which is *very*
> > > +	 * counter-productive as this vCPU can't actually make forward progress
> > > +	 * until the invalidation completes.  This "unsafe" check can get false
> > > +	 * negatives, i.e. KVM needs to re-check after acquiring mmu_lock.
> > > +	 *
> > > +	 * Do the pre-check even for non-preemtible kernels, i.e. even if KVM
> > > +	 * will never yield mmu_lock in response to contention, as this vCPU is
> > > +	 * *guaranteed* to need to retry, i.e. waiting until mmu_lock is held
> > > +	 * to detect retry guarantees the worst case latency for the vCPU.
> > > +	 */
> > > +	if (mmu_invalidate_retry_gfn_unsafe(vcpu->kvm, fault->mmu_seq, fault->gfn)) {
> > > +		kvm_release_pfn_clean(fault->pfn);
> > > +		return RET_PF_RETRY;
> > > +	}
> > > +
> > Could we also add this pre-check before fault in the pfn? like
> > @@ -4404,6 +4404,8 @@ static int kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault,
> > 
> >         fault->mmu_seq = vcpu->kvm->mmu_invalidate_seq;
> >         smp_rmb();
> > +       if (mmu_invalidate_retry_gfn_unsafe(vcpu->kvm, fault->mmu_seq, fault->gfn))
> > +               return RET_PF_CONTINUE;
> > 
> >         ret = __kvm_faultin_pfn(vcpu, fault);
> >         if (ret != RET_PF_CONTINUE)
> > 
> > Though the mmu_seq would be always equal in the check, it can avoid repeated faulting
> > and release pfn for vain during a long zap cycle.
> 
> Just to be super claer, by "repeated faulting", you mean repeated faulting in the
> primary MMU, correct?
>
Yes. Faulting in the primary MMU.
(Please ignore my typo in return type above :))

BTW, will you also send the optmization in v1 as below?
iff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 1e340098d034..c7617991e290 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -5725,11 +5725,13 @@ int noinline kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 err
        }
 
        if (r == RET_PF_INVALID) {
-               r = kvm_mmu_do_page_fault(vcpu, cr2_or_gpa,
-                                         lower_32_bits(error_code), false,
-                                         &emulation_type);
-               if (KVM_BUG_ON(r == RET_PF_INVALID, vcpu->kvm))
-                       return -EIO;
+               do {
+                       r = kvm_mmu_do_page_fault(vcpu, cr2_or_gpa,
+                                                 lower_32_bits(error_code),
+                                                 false, &emulation_type);
+                       if (KVM_BUG_ON(r == RET_PF_INVALID, vcpu->kvm))
+                               return -EIO;
+               while (r == RET_PF_RETRY);
        }
 
        if (r < 0)

> Yeah, I agree, that makes sense.  The only question is whether to check before
> and after, or only before.  When abusing KSM, I see ~99.5% of all invalidations
> being detected before (21.5M out of just over 21.6M).
> 
> I think it makes sense to also check after getting the pfn?  The check is super
> cheap, especially when there isn't an invalidation event as the checks should all
> be quite predictable and cache hot.
I think checking at before and after is reasonable if the cost of check is not a
concern.


> > > +/*
> > > + * This lockless version of the range-based retry check *must* be paired with a
> > > + * call to the locked version after acquiring mmu_lock, i.e. this is safe to
> > > + * use only as a pre-check to avoid contending mmu_lock.  This version *will*
> > > + * get false negatives and false positives.
> > > + */
> > > +static inline bool mmu_invalidate_retry_gfn_unsafe(struct kvm *kvm,
> > > +						   unsigned long mmu_seq,
> > > +						   gfn_t gfn)
> > > +{
> > > +	/*
> > > +	 * Use READ_ONCE() to ensure the in-progress flag and sequence counter
> > > +	 * are always read from memory, e.g. so that checking for retry in a
> > > +	 * loop won't result in an infinite retry loop.  Don't force loads for
> > > +	 * start+end, as the key to avoiding infinite retry loops is observing
> > > +	 * the 1=>0 transition of in-progress, i.e. getting false negatives
> > > +	 * due to stale start+end values is acceptable.
> > > +	 */
> > > +	if (unlikely(READ_ONCE(kvm->mmu_invalidate_in_progress)) &&
> > > +	    gfn >= kvm->mmu_invalidate_range_start &&
> > > +	    gfn < kvm->mmu_invalidate_range_end)
> > > +		return true;
> > > +
> > Is a smp_rmb() before below check better, given this retry is defined in a header
> > for all platforms?
> 
> No, because the unsafe check very deliberately doesn't provide any ordering
> guarantees.  The READ_ONCE() is purely to ensure forward progress.  And trying to
> provide ordering for mmu_invalidate_in_progress is rather nonsensical.  The smp_rmb()
> in mmu_invalidate_retry() ensures the caller will see a different mmu_invalidate_seq
> or an elevated mmu_invalidate_in_progress.
> 
> For this code, the order in which mmu_invalidate_seq and mmu_invalidate_in_progress
> are checked truly doesn't matter as the range-based checks are will get false
> negatives when performed outside of mmu_lock.   And from a correctness perspective,
> there are zero constraints on when the checks are performed (beyond the existing
> constraints from the earlier smp_rmb() and acquiring mmu_lock).
> 
> If an arch wants ordering guarantees, it can simply use mmu_invalidate_retry()
> without a gfn, which has the advantage of being safe outside of mmu_lock (the
> obvious disadvantage is that it's very imprecise).
Ok. It makes sense!