Reorder the TDP MMU check to be first for both kvm_test_age_gfn and
kvm_age_gfn. For kvm_test_age_gfn, this allows us to completely avoid
needing to grab the MMU lock when the TDP MMU reports that the page is
young. Do the same for kvm_age_gfn merely for consistency.

Signed-off-by: James Houghton <jthoughton@google.com>
---
 arch/x86/kvm/mmu/mmu.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 26797ccd34d8..793565a3a573 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -1586,15 +1586,15 @@ bool kvm_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
 {
 	bool young = false;
 
+	if (tdp_mmu_enabled)
+		young = kvm_tdp_mmu_age_gfn_range(kvm, range);
+
 	if (kvm_memslots_have_rmaps(kvm)) {
 		write_lock(&kvm->mmu_lock);
-		young = kvm_rmap_age_gfn_range(kvm, range, false);
+		young |= kvm_rmap_age_gfn_range(kvm, range, false);
 		write_unlock(&kvm->mmu_lock);
 	}
 
-	if (tdp_mmu_enabled)
-		young |= kvm_tdp_mmu_age_gfn_range(kvm, range);
-
 	return young;
 }
 
@@ -1602,15 +1602,15 @@ bool kvm_test_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
 {
 	bool young = false;
 
-	if (kvm_memslots_have_rmaps(kvm)) {
+	if (tdp_mmu_enabled)
+		young = kvm_tdp_mmu_test_age_gfn(kvm, range);
+
+	if (!young && kvm_memslots_have_rmaps(kvm)) {
 		write_lock(&kvm->mmu_lock);
-		young = kvm_rmap_age_gfn_range(kvm, range, true);
+		young |= kvm_rmap_age_gfn_range(kvm, range, true);
 		write_unlock(&kvm->mmu_lock);
 	}
 
-	if (tdp_mmu_enabled)
-		young |= kvm_tdp_mmu_test_age_gfn(kvm, range);
-
 	return young;
 }
 
-- 
2.47.0.199.ga7371fff76-goog

On Tue, Nov 05, 2024, James Houghton wrote:
> Reorder the TDP MMU check to be first for both kvm_test_age_gfn and

() on functions, i.e. kvm_test_age_gfn().  That said, even better would be to
avoid using the function names.  Let the patch itself communicate which functions
are affected, and instead write the changelog as you would verbally communicate
the change.

> kvm_age_gfn. For kvm_test_age_gfn, this allows us to completely avoid

No "us" or "we".


> needing to grab the MMU lock when the TDP MMU reports that the page is
> young.

The changelog should make it clear that the patch actually does this, i.e. that
there is a functional change beyond just changing the ordering.  Ooh, and that
definitely needs to be captured in the shortlog.  I would even go so far as to
say it should be the focal point of the shortlog.

E.g. something like:

KVM: x86/mmu: Skip shadow MMU test_young if TDP MMU reports page as young

Reorder the processing of the TDP MMU versus the shadow MMU when aging
SPTEs, and skip the shadow MMU entirely in the test-only case if the TDP
MMU reports that the page is young, i.e. completely avoid taking mmu_lock
if the TDP MMU SPTE is young.  Swap the order for the test-and-age helper
as well for consistency.

> Do the same for kvm_age_gfn merely for consistency.

On Fri, Jan 10, 2025 at 2:59 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Nov 05, 2024, James Houghton wrote:
> > Reorder the TDP MMU check to be first for both kvm_test_age_gfn and
>
> () on functions, i.e. kvm_test_age_gfn().  That said, even better would be to
> avoid using the function names.  Let the patch itself communicate which functions
> are affected, and instead write the changelog as you would verbally communicate
> the change.
>
> > kvm_age_gfn. For kvm_test_age_gfn, this allows us to completely avoid
>
> No "us" or "we".
>
>
> > needing to grab the MMU lock when the TDP MMU reports that the page is
> > young.
>
> The changelog should make it clear that the patch actually does this, i.e. that
> there is a functional change beyond just changing the ordering.  Ooh, and that
> definitely needs to be captured in the shortlog.  I would even go so far as to
> say it should be the focal point of the shortlog.
>
> E.g. something like:
>
> KVM: x86/mmu: Skip shadow MMU test_young if TDP MMU reports page as young
>
> Reorder the processing of the TDP MMU versus the shadow MMU when aging
> SPTEs, and skip the shadow MMU entirely in the test-only case if the TDP
> MMU reports that the page is young, i.e. completely avoid taking mmu_lock
> if the TDP MMU SPTE is young.  Swap the order for the test-and-age helper
> as well for consistency.

Thanks, I think this is worded very clearly. Applied verbatim.

Noted your tips for future changelogs.

On Tue, Nov 5, 2024 at 11:43 AM James Houghton <jthoughton@google.com> wrote:
>
> Reorder the TDP MMU check to be first for both kvm_test_age_gfn and
> kvm_age_gfn. For kvm_test_age_gfn, this allows us to completely avoid
> needing to grab the MMU lock when the TDP MMU reports that the page is
> young. Do the same for kvm_age_gfn merely for consistency.
>
> Signed-off-by: James Houghton <jthoughton@google.com>

Acked-by: Yu Zhao <yuzhao@google.com>