Re: [PATCH 3/3] KVM: arm64: nv: Punt stage-2 recycling to a vCPU request

[Oliver Upton <oliver.upton@linux.dev>]

On Wed, Oct 02, 2024 at 12:49:27AM +0100, Marc Zyngier wrote:
> On Wed, 02 Oct 2024 00:28:18 +0100,
> Sean Christopherson <seanjc@google.com> wrote:
> > 
> > On Tue, Oct 01, 2024, Oliver Upton wrote:
> > > Hey,
> > > 
> > > sidebar: I was a bit confused by the diff for a second, since it looks
> > > like your email client lowercased some stuff :)
> > 
> > Wasn't my mail client, it was PEBKAC.  I copy+pasted a large chunk in Vim because
> > I wanted to pull in the changelog (which I had deleted from my response), but then
> > I changed my mind, and in doing so I managed to fat-finger something that converted
> > everything to lowercase.  And yeah, it confused me too.
> > 
> > > > >  out:
> > > > > +	if (s2_mmu->pending_unmap)
> > > > > +		kvm_make_request(kvm_req_nested_s2_unmap, vcpu);
> > > > 
> > > > If I followed everything correctly, I don't think a request is needed.  the
> > > > request will never be cross-vCPU, and each vCPU holds a reference to the MMU, so
> > > > the MMU can't be recycled, i.e. pending_unmap is guaranteed to be relevant to the
> > > > vCPU's usage of the MMU.  More thoughts below in check_nested_vcpu_requests().
> > > 
> > > I'm (ab)using the request to prevent the vCPU thread from actually
> > > entering the VM without first having done the laundry. We have other
> > > examples of strictly per-vCPU tasks that are tracked with a request so
> > > this doesn't stick out that much.
> > > 
> > > Otherwise we'd need an open-coded check in kvm_vcpu_exit_request() to
> > > catch a 'dirty' MMU or take a pin on it from the point we check the
> > > dirtiness to the point we disable preemption.
> > 
> > Ewww, because kvm_arch_vcpu_put() puts the nested stage-2 when the vCPU is
> > scheduled out.  Mostly out of curiosity, why?  99.9% of the time, the vCPU will
> > be scheduled back in.
> 
> Because s2 MMU structures are a scarce resource. and other vcpus could
> have the opportunity to make use of an unused slot.
> 
> > Now that vcpu->scheduled_out is a thing, retaining the nested s2 MMU should be
> > quite straightforward.  kvm_arch_vcpu_destroy() would need to put the MMU, but
> > that should also be straightforward.
> 
> This code long predates scheduled_out, and I don't think this brings
> much to the table. If the vcpu comes back quickly, it will find its
> toys where it left them. If not, someone else will have borrowed them,
> and it will have to pick new ones. It isn't any different from TLBs,
> which s2 MMUs model.

In line with what Sean is recommending, it might make sense to explore
holding the reference while a vCPU is loaded and runnable, i.e. the vCPU
isn't scheduling out due to an emulated WFI. While not perfect, it would
increase the likelihood that we evict a 'cold' MMU.

But again, we should make sure we're actually happy with this allocation
scheme first before bothering with optimizing it a lot.

-- 
Thanks,
Oliver