On Thu, Aug 29, 2024, James Houghton wrote:

On Fri, Aug 16, 2024 at 6:05 PM Sean Christopherson [off-list ref] wrote:

quoted

quoted

+static __always_inline bool kvm_tdp_mmu_handle_gfn_lockless(
+             struct kvm *kvm,
+             struct kvm_gfn_range *range,
+             tdp_handler_t handler)

Please burn all the Google3 from your brain, and code ;-)

I indented this way to avoid going past the 80 character limit. I've
adjusted it to be more like the other functions in this file.

Perhaps I should put `static __always_inline bool` on its own line?

Noooo. Do not wrap before the function name.  Linus has a nice explanation/rant
on this[1].

In this case, I'm pretty sure you can avoid the helper and simply handle all aging
paths in a single API, e.g. similar to what I proposed for the shadow MMU[2].

[1] https://lore.kernel.org/all/CAHk-=wjoLAYG446ZNHfg=GhjSY6nFmuB_wA8fYd5iLBNXjo9Bw@mail.gmail.com (local)
[2] https://lore.kernel.org/all/20240809194335.1726916-16-seanjc@google.com (local)

quoted

quoted

 /*
  * Mark the SPTEs range of GFNs [start, end) unaccessed and return non-zero
  * if any of the GFNs in the range have been accessed.

@@ -1237,28 +1272,30 @@ static bool age_gfn_range(struct kvm *kvm, struct tdp_iter *iter,
 {
      u64 new_spte;

+retry:
      /* If we have a non-accessed entry we don't need to change the pte. */
      if (!is_accessed_spte(iter->old_spte))
              return false;

      if (spte_ad_enabled(iter->old_spte)) {
-             iter->old_spte = tdp_mmu_clear_spte_bits(iter->sptep,
-                                                      iter->old_spte,
-                                                      shadow_accessed_mask,
-                                                      iter->level);
+             iter->old_spte = tdp_mmu_clear_spte_bits_atomic(iter->sptep,
+                                             shadow_accessed_mask);
              new_spte = iter->old_spte & ~shadow_accessed_mask;
      } else {
-             /*
-              * Capture the dirty status of the page, so that it doesn't get
-              * lost when the SPTE is marked for access tracking.
-              */
+             new_spte = mark_spte_for_access_track(iter->old_spte);
+             if (__tdp_mmu_set_spte_atomic(iter, new_spte)) {
+                     /*
+                      * The cmpxchg failed. If the spte is still a
+                      * last-level spte, we can safely retry.
+                      */
+                     if (is_shadow_present_pte(iter->old_spte) &&
+                         is_last_spte(iter->old_spte, iter->level))
+                             goto retry;

Do we have a feel for how often conflicts actually happen?  I.e. is it worth
retrying and having to worry about infinite loops, however improbable they may
be?

I'm not sure how common this is. I think it's probably better not to
retry actually. If the cmpxchg fails, this spte is probably young
anyway, so I can just `return true` instead of potentially retrying.
This is all best-effort anyway.

+1