Re: [RFC PATCH 0/11] Rework gfn_to_pfn_cache

From: David Woodhouse <dwmw2@infradead.org>
To: Sean Christopherson <seanjc@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Cc: kvm <kvm@vger.kernel.org>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	Joao Martins <joao.m.martins@oracle.com>,
	"jmattson@google.com" <jmattson@google.com>,
	"wanpengli@tencent.com" <wanpengli@tencent.com>,
	"vkuznets@redhat.com" <vkuznets@redhat.com>,
	"mtosatti@redhat.com" <mtosatti@redhat.com>,
	"joro@8bytes.org" <joro@8bytes.org>,
	karahmed@amazon.com
Subject: Re: [RFC PATCH 0/11] Rework gfn_to_pfn_cache
Date: Mon, 15 Nov 2021 23:22:20 +0000	[thread overview]
Message-ID: <57d599584ace8ab410b9b14569f434028e2cf642.camel@infradead.org> (raw)
In-Reply-To: <YZLmapmzs7sLpu/L@google.com>

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

On Mon, 2021-11-15 at 22:59 +0000, Sean Christopherson wrote:
> On Mon, Nov 15, 2021, Paolo Bonzini wrote:
> > On 11/15/21 20:11, David Woodhouse wrote:
> > > > Changing mn_memslots_update_rcuwait to a waitq (and renaming it to
> > > > mn_invalidate_waitq) is of course also a possibility.
> > > I suspect that's the answer.
> > > 
> > > I think the actual*invalidation*  of the cache still lives in the
> > > invalidate_range() callback where I have it at the moment.
> 
> Oooh!  [finally had a lightbulb moment about ->invalidate_range() after years of
> befuddlement].
> 
> Two things:
> 
>   1. Using _only_ ->invalidate_range() is not correct.  ->invalidate_range() is
>      required if and only if the old PFN needs to be _unmapped_.  Specifically,
>      if the protections are being downgraded without changing the PFN, it doesn't
>      need to be called.  E.g. from hugetlb_change_protection():

OK, that's kind of important to realise. Thanks.

So, I had just split the atomic and guest-mode invalidations apart:
https://git.infradead.org/users/dwmw2/linux.git/commitdiff/6cf5fe318fd
but will go back to doing it all in invalidate_range_start from a
single list.

And just deal with the fact that the atomic users now have to
loop/retry/wait for there *not* to be an MMU notification in progress.

>      I believe we could use ->invalidate_range() to handle the unmap case if KVM's
>      ->invalidate_range_start() hook is enhanced to handle the RW=>R case.  The
>      "struct mmu_notifier_range" provides the event type, IIUC we could have the
>      _start() variant handle MMU_NOTIFY_PROTECTION_{VMA,PAGE} (and maybe
>      MMU_NOTIFY_SOFT_DIRTY?), and let the more precise unmap-only variant handle
>      everything else.

Not sure that helps us much. It was the termination condition on the
"when should we keep retrying, and when should we give up?" that was
painful, and a mixed mode doesn't that problem it go away.

I'll go back and have another look in the morning, with something much
closer to what I showed in
https://lore.kernel.org/kvm/040d61dad066eb2517c108232efb975bc1cda780.camel@infradead.org/

>   2. If we do split the logic across the two hooks, we should (a) do it in a separate
>      series and (b) make the logic common to the gfn_to_pfn cache and to the standard
>      kvm_unmap_gfn_range(). 
> > 
> > Yes, I think sooner or later we also want all pfn stuff in one file
> > (together with MMU notifiers) and all hva stuff in another; so for now you
> > can create virt/kvm/hva_to_pfn.h, or virt/kvm/mm.h, or whatever color of the
> > bikeshed you prefer.
> 
> Preemptive bikeshed strike... the MMU notifiers aren't strictly "pfn stuff", as
> they operate on HVAs.  I don't know exactly what Paolo has in mind, but kvm/mm.h
> or kvm/kvm_mm.h seems like it's less likely to become stale in the future.

I'd moved kvm/mmu_lock.h to kvm/kvm_mm.h and added to it.
https://git.infradead.org/users/dwmw2/linux.git/commitdiff/a247bc2d0d9
(which I'll make retrospective as I rework the series).

After frowning a little at all the different architectures' Makefiles
that all add the same(ish) list of $(KVM)/foobar.o I ended up punting
that problem by only adding pfncache.o on x86 anyway.

If we're going to split other parts of kvm_main.c out into smaller
files, providing a Makefile snippet in virt/kvm/Makefile.kvm that gives
the *list* of those files would be a useful thing to do. But
arch/powerpc/kvm/Makefile makes my head hurt too much for me to be
shaving that particular yak tonight (why is $(KVM)/irqchip.o handled
differently to the rest...?)

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