Re: [PATCH v3 09/41] KVM: arm64: Defer restoring host VFP state to vcpu_put

[Marc Zyngier <marc.zyngier@arm.com>]

On Wed, 14 Feb 2018 17:38:11 +0000,
Christoffer Dall wrote:
> 
> On Wed, Feb 14, 2018 at 02:43:42PM +0000, Dave Martin wrote:
> > [CC Ard, in case he has a view on how much we care about softirq NEON
> > performance regressions ... and whether my suggestions make sense]
> > 
> > On Wed, Feb 14, 2018 at 11:15:54AM +0100, Christoffer Dall wrote:
> > > On Tue, Feb 13, 2018 at 02:08:47PM +0000, Dave Martin wrote:
> > > > On Tue, Feb 13, 2018 at 09:51:30AM +0100, Christoffer Dall wrote:
> > > > > On Fri, Feb 09, 2018 at 03:59:30PM +0000, Dave Martin wrote:
> 
> [...]
> 
> > > > 
> > > > kvm_fpsimd_flush_cpu_state() is just an invalidation.  No state is
> > > > actually saved today because we explicitly don't care about preserving
> > > > the SVE state, because the syscall ABI throws the SVE regs away as
> > > > a side effect any syscall including ioctl(KVM_RUN); also (currently) KVM
> > > > ensures that the non-SVE FPSIMD bits _are_ restored by itself.
> > > > 
> > > > I think my proposal is that this hook might take on the role of
> > > > actually saving the state too, if we move that out of the KVM host
> > > > context save/restore code.
> > > > 
> > > > Perhaps we could even replace
> > > > 
> > > > 	preempt_disable();
> > > > 	kvm_fpsimd_flush_cpu_state();
> > > > 	/* ... */
> > > > 	preempt_enable();
> > > > 
> > > > with
> > > > 
> > > > 	kernel_neon_begin();
> > > > 	/* ... */
> > > > 	kernel_neon_end();
> > > 
> > > I'm not entirely sure where the begin and end points would be in the
> > > context of KVM?
> > 
> > Hmmm, actually there's a bug in your VHE changes now I look more
> > closely in this area:
> > 
> > You assume that the only way for the FPSIMD regs to get unexpectedly
> > dirtied is through a context switch, but actually this is not the case:
> > a softirq can use kernel-mode NEON any time that softirqs are enabled.
> > 
> > This means that in between kvm_arch_vcpu_load() and _put() (whether via
> > preempt notification or not), the guest's FPSIMD state in the regs may
> > be trashed by a softirq.
> 
> ouch.
> 
> > 
> > The simplest fix is to disable softirqs and preemption for that whole
> > region, but since we can stay in it indefinitely that's obviously not
> > the right approach.  Putting kernel_neon_begin() in _load() and
> > kernel_neon_end() in _put() achieves the same without disabling
> > softirq, but preemption is still disabled throughout, which is bad.
> > This effectively makes the run ioctl nonpreemptible...
> > 
> > A better fix would be to set the cpu's kernel_neon_busy flag, which
> > makes softirq code use non-NEON fallback code.
> > 
> > We could expose an interface from fpsimd.c to support that.
> > 
> > It still comes at a cost though: due to the switching from NEON to
> > fallback code in softirq handlers, we may get a big performance
> > regression in setups that rely heavily on NEON in softirq for
> > performance.
> > 
> 
> I wasn't aware that softirqs would use fpsimd.
> 
> > 
> > Alternatively we could do something like the following, but it's a
> > rather gross abstraction violation:
> > 
> > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > index 2e43f9d..6a1ff3a 100644
> > --- a/virt/kvm/arm/arm.c
> > +++ b/virt/kvm/arm/arm.c
> > @@ -746,9 +746,24 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >  		 * the effect of taking the interrupt again, in SVC
> >  		 * mode this time.
> >  		 */
> > +		local_bh_disable();
> >  		local_irq_enable();
> >  
> >  		/*
> > +		 * If we exited due to one or mode pending interrupts, they
> > +		 * have now been handled.  If such an interrupt pended a
> > +		 * softirq, we shouldn't prevent that softirq from using
> > +		 * kernel-mode NEON indefinitely: instead, give FPSIMD back to
> > +		 * the host to manage as it likes.  We'll grab it again on the
> > +		 * next FPSIMD trap from the guest (if any).
> > +		 */
> > +		if (local_softirq_pending() && FPSIMD untrapped for guest) {
> > +			/* save vcpu FPSIMD context */
> > +			/* enable FPSIMD trap for guest */
> > +		}
> > +		local_bh_enable();
> > +
> > +		/*
> >  		 * We do local_irq_enable() before calling guest_exit() so
> >  		 * that if a timer interrupt hits while running the guest we
> >  		 * account that tick as being spent in the guest.  We enable
> > 
> > [...]
> > 
> 
> I can't see this working, what if an IRQ comes in and a softirq gets
> pending immediately after local_bh_enable() above?
> 
> And as you say, it's really not pretty.
> 
> This is really making me think that I'll drop this part of the
> optimization and when we do optimize fpsimd handling, we do it properly
> by integrating it with the kernel tracking.
> 
> What do you think?

[catching up with the discussion]

I think it makes sense. I'd be worried if we'd merge something that we
know to be sub-par, and that could introduce unexpected performance
regressions. It looks like we have a slightly more long-term plan to
address this in a way that integrates with the rest of the kernel
infrastructure, so let's take this opportunity.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.