[PATCH 0/2] KVM: x86/mmu: Micro-optimize TDP MMU cond_resched()

[PATCH 0/2] KVM: x86/mmu: Micro-optimize TDP MMU cond_resched()

[Sean Christopherson]

Invert the order of checks in tdp_mmu_iter_cond_resched() so that the common
case (no resched needed) is checked first, and opportunsitically clean up a
wart where the helper would return "yielded" in an error path, even if that
invocation didn't actually yield.  The latter cleanup allows extracting the
base "need resched" logic to a separate helper.

Note, I "speculatively" pushed these patches to kvm-x86 mmu, as they are
effectively needed for David's series to optimize hugepage recovery when
disabling dirty logging.  I'll rework the commits sometime next weeks, e.g.
fix a few changelog typos (I found two, so far), to capture reviews, and
add Links.  And if there are problems, I'll yank you the entire series
(David's and mine).

  012a5c17cba4d165961a4643c5e10a151f003733 KVM: x86/mmu: Demote the WARN on yielded in xxx_cond_resched() to KVM_MMU_WARN_ON
  d400ce271d9c555a93e38fb36ecb2d3bd60b234c KVM: x86/mmu: Check yielded_gfn for forward progress iff resched is needed

Sean Christopherson (2):
  KVM: x86/mmu: Check yielded_gfn for forward progress iff resched is
    needed
  KVM: x86/mmu: Demote the WARN on yielded in xxx_cond_resched() to
    KVM_MMU_WARN_ON

 arch/x86/kvm/mmu/tdp_mmu.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)


base-commit: 5cb1659f412041e4780f2e8ee49b2e03728a2ba6
-- 
2.47.0.163.g1226f6d8fa-goog

[PATCH 1/2] KVM: x86/mmu: Check yielded_gfn for forward progress iff resched is needed

[Sean Christopherson]

Swap the order of the checks in tdp_mmu_iter_cond_resched() so that KVM
checks to see if a resched is needed _before_ checking to see if yielding
must be disallowed to guarantee forward progress.  Iterating over TDP MMU
SPTEs is a hot path, e.g. tearing down a root can touch millions of SPTEs,
and not needing to reschedule is by far the common case.  On the other
hand, disallowing yielding because forward progress has not been made is a
very rare case.

Returning early for the common case (no resched), effectively reduces the
number of checks from 2 to 1 for the common case, and should make the code
slightly more predictable for the CPU.

To resolve a weird conundrum where the forward progress check currently
returns false, but the need resched check subtly returns iter->yielded,
which _should_ be false (enforced by a WARN), return false unconditionally
(which might also help make the sequence more predictable).  If KVM has a
bug where iter->yielded is left danging, continuing to yield is neither
right nor wrong, it was simply an artifact of how the original code was
written.

Unconditionally returning false when yielding is unnecessary or unwanted
will also allow extracting the "should resched" logic to a separate helper
in a future patch.

Cc: David Matlack <dmatlack@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/mmu/tdp_mmu.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 91caa73a905b..a06f3d5cb651 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -700,29 +700,29 @@ static inline bool __must_check tdp_mmu_iter_cond_resched(struct kvm *kvm,
 {
 	WARN_ON_ONCE(iter->yielded);
 
+	if (!need_resched() && !rwlock_needbreak(&kvm->mmu_lock))
+		return false;
+
 	/* Ensure forward progress has been made before yielding. */
 	if (iter->next_last_level_gfn == iter->yielded_gfn)
 		return false;
 
-	if (need_resched() || rwlock_needbreak(&kvm->mmu_lock)) {
-		if (flush)
-			kvm_flush_remote_tlbs(kvm);
+	if (flush)
+		kvm_flush_remote_tlbs(kvm);
 
-		rcu_read_unlock();
+	rcu_read_unlock();
 
-		if (shared)
-			cond_resched_rwlock_read(&kvm->mmu_lock);
-		else
-			cond_resched_rwlock_write(&kvm->mmu_lock);
+	if (shared)
+		cond_resched_rwlock_read(&kvm->mmu_lock);
+	else
+		cond_resched_rwlock_write(&kvm->mmu_lock);
 
-		rcu_read_lock();
+	rcu_read_lock();
 
-		WARN_ON_ONCE(iter->gfn > iter->next_last_level_gfn);
+	WARN_ON_ONCE(iter->gfn > iter->next_last_level_gfn);
 
-		iter->yielded = true;
-	}
-
-	return iter->yielded;
+	iter->yielded = true;
+	return true;
 }
 
 static inline gfn_t tdp_mmu_max_gfn_exclusive(void)
-- 
2.47.0.163.g1226f6d8fa-goog

Re: [PATCH 1/2] KVM: x86/mmu: Check yielded_gfn for forward progress iff resched is needed

[James Houghton]

On Thu, Oct 31, 2024 at 10:07 AM Sean Christopherson <seanjc@google.com> wrote:
>
> Swap the order of the checks in tdp_mmu_iter_cond_resched() so that KVM
> checks to see if a resched is needed _before_ checking to see if yielding
> must be disallowed to guarantee forward progress.  Iterating over TDP MMU
> SPTEs is a hot path, e.g. tearing down a root can touch millions of SPTEs,
> and not needing to reschedule is by far the common case.  On the other
> hand, disallowing yielding because forward progress has not been made is a
> very rare case.
>
> Returning early for the common case (no resched), effectively reduces the
> number of checks from 2 to 1 for the common case, and should make the code
> slightly more predictable for the CPU.
>
> To resolve a weird conundrum where the forward progress check currently
> returns false, but the need resched check subtly returns iter->yielded,
> which _should_ be false (enforced by a WARN), return false unconditionally
> (which might also help make the sequence more predictable).  If KVM has a
> bug where iter->yielded is left danging, continuing to yield is neither
> right nor wrong, it was simply an artifact of how the original code was
> written.
>
> Unconditionally returning false when yielding is unnecessary or unwanted
> will also allow extracting the "should resched" logic to a separate helper
> in a future patch.
>
> Cc: David Matlack <dmatlack@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Feel free to add:

Reviewed-by: James Houghton <jthoughton@google.com>

[PATCH 2/2] KVM: x86/mmu: Demote the WARN on yielded in xxx_cond_resched() to KVM_MMU_WARN_ON

[Sean Christopherson]

Convert the WARN in tdp_mmu_iter_cond_resched() that the iterator hasn't
already yielded to a KVM_MMU_WARN_ON() so the code is compiled out for
production kernels (assuming production kernels disable KVM_PROVE_MMU).

Checking for a needed reschedule is a hot path, and KVM sanity checks
iter->yielded in several other less-hot paths, i.e. the odds of KVM not
flagging that something went sideways are quite low.  Furthermore, the
odds of KVM not noticing *and* the WARN detecting something worth
investigating are even lower.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/mmu/tdp_mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index a06f3d5cb651..c158ef8c1a36 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -698,7 +698,7 @@ static inline bool __must_check tdp_mmu_iter_cond_resched(struct kvm *kvm,
 							  struct tdp_iter *iter,
 							  bool flush, bool shared)
 {
-	WARN_ON_ONCE(iter->yielded);
+	KVM_MMU_WARN_ON(iter->yielded);
 
 	if (!need_resched() && !rwlock_needbreak(&kvm->mmu_lock))
 		return false;
-- 
2.47.0.163.g1226f6d8fa-goog

Re: [PATCH 0/2] KVM: x86/mmu: Micro-optimize TDP MMU cond_resched()

[Sean Christopherson]

On Thu, 31 Oct 2024 10:06:31 -0700, Sean Christopherson wrote:
> Invert the order of checks in tdp_mmu_iter_cond_resched() so that the common
> case (no resched needed) is checked first, and opportunsitically clean up a
> wart where the helper would return "yielded" in an error path, even if that
> invocation didn't actually yield.  The latter cleanup allows extracting the
> base "need resched" logic to a separate helper.
> 
> Note, I "speculatively" pushed these patches to kvm-x86 mmu, as they are
> effectively needed for David's series to optimize hugepage recovery when
> disabling dirty logging.  I'll rework the commits sometime next weeks, e.g.
> fix a few changelog typos (I found two, so far), to capture reviews, and
> add Links.  And if there are problems, I'll yank you the entire series
> (David's and mine).
> 
> [...]

"Officially" applied to kvm-x86 mmu.

[1/2] KVM: x86/mmu: Check yielded_gfn for forward progress iff resched is needed
      https://github.com/kvm-x86/linux/commit/e287e4316713
[2/2] KVM: x86/mmu: Demote the WARN on yielded in xxx_cond_resched() to KVM_MMU_WARN_ON
      https://github.com/kvm-x86/linux/commit/38b0ac47169b

--
https://github.com/kvm-x86/linux/tree/next