Re: [PATCH] KVM: Move gfn_to_pfn_cache invalidation to invalidate_range_end hook

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3517 bytes --]

On Mon, 2024-08-05 at 17:45 -0700, Sean Christopherson wrote:
> On Mon, Aug 05, 2024, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > The existing retry loop in hva_to_pfn_retry() is extremely pessimistic.
> > If there is an invalidation running concurrently, it is effectively just
> > a complex busy wait loop because its local mmu_notifier_retry_cache()
> > function will always return true.
> > 
> > It ends up functioning as a very unfair read/write lock. If userspace is
> > acting as a 'writer', performing many unrelated MM changes, then the
> > hva_to_pfn_retry() function acting as the 'reader' just backs off and
> > keep retrying for ever, not making any progress.
> > 
> > Solve this by introducing a separate 'validating' flag to the GPC, so
> > that it can be marked invalid before it's even mapped. This allows the
> > invalidation to be moved to the range_end hook, and the retry loop in
> > hva_to_pfn_retry() can be changed to loop only if its particular uHVA
> > has been affected.
> 
> I think I'm missing something.  How does allowing hva_to_pfn_retry() allow KVM
> as a whole to make forward progress?  Doesn't getting past hva_to_pfn_retry()
> just move the problem to kvm_gpc_check()?
> 
> kvm_gpc_refresh() can't be called with gpc->lock held, and nor does it return
> with gpc->lock held, so a racing mmu_notifier invalidation can/will acquire
> gpc->lock and clear gpc->active, no?
> 
> Oh, by "unrelated", you mean _completely_ unrelated?  As in, KVM happens to do a
> refresh when userspace is blasting MADV_DONTNEED, and gets stuck retrying for
> no good reason?

Right. And again, I have no idea why userspace is doing that, and we
need to make it stop. But that's not really the point; the kernel
should be able to make progress anyway.

> Servicing guest pages faults has the same problem, which is why
> mmu_invalidate_retry_gfn() was added.  Supporting hva-only GPCs made our lives a
> little harder, but not horrifically so (there are ordering differences regardless).
> 
> Woefully incomplete, but I think this is the gist of what you want:

Hm, maybe. It does mean that migration occurring all through memory
(indeed, just one at top and bottom of guest memory space) would
perturb GPCs which remain present.

> > @@ -849,6 +837,8 @@ static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn,
> >         wake = !kvm->mn_active_invalidate_count;
> >         spin_unlock(&kvm->mn_invalidate_lock);
> >  
> > +       gfn_to_pfn_cache_invalidate(kvm, range->start, range->end);
> 
> We can't do this.  The contract with mmu_notifiers is that secondary MMUs must
> unmap the hva before returning from invalidate_range_start(), and must not create
> new mappings until invalidate_range_end().

But in the context of the GPC, it is only "mapped" when the ->valid bit is set. 

Even the invalidation callback just clears the valid bit, and that
means nobody is allowed to dereference the ->khva any more. It doesn't
matter that the underlying (stale) PFN is still kmapped.

Can we not apply the same logic to the hva_to_pfn_retry() loop? Yes, it
might kmap a page that gets removed, but it's not actually created a
new mapping if it hasn't set the ->valid bit.

I don't think this version quite meets the constraints, and I might
need to hook *both* the start and end notifiers, and might not like it
once I get there. But I'll have a go...

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]