On Thu, Jun 14, 2018 at 01:50:00PM +0100, Marc Zyngier wrote:
> Hi Dave,
>
> On 14/06/18 12:42, Dave Martin wrote:
> > Commit e6b673b ("KVM: arm64: Optimise FPSIMD handling to reduce
> > guest/host thrashing") introduces a specific helper
> > kvm_arch_vcpu_put_fp() for saving the vcpu FPSIMD state during
> > vcpu_put().
> >
> > This function uses local_bh_disable()/_enable() to protect the
> > FPSIMD context manipulation from interruption by softirqs.
> >
> > This approach is not correct, because vcpu_put() can be invoked
> > either from the KVM host vcpu thread (when exiting the vcpu run
> > loop), or via a preempt notifier. In the former case, only
> > preemption is disabled. In the latter case, the function is called
> > from inside __schedule(), which means that IRQs are disabled.
> >
> > Use of local_bh_disable()/_enable() with IRQs disabled is considerd
> > an error, resulting in lockdep splats while running VMs if lockdep
> > is enabled.
> >
> > It is probably possible in theory to relax this restriction on
> > local_bh_disable()/_enable() usage, but for now this patch takes
> > the simple approach of managing softirq masking only if IRQs happen
> > to be enabled when kvm_arch_vcpu_put_fp() is called.
> >
> > Fixes: e6b673b741ea ("KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing")
> > Reported-by: Andre Przywara <andre.przywara@xxxxxxx>
> > Signed-off-by: Dave Martin <Dave.Martin@xxxxxxx>
> > ---
> > arch/arm64/kvm/fpsimd.c | 16 ++++++++++++----
> > 1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> > index dc6ecfa..b51ff80 100644
> > --- a/arch/arm64/kvm/fpsimd.c
> > +++ b/arch/arm64/kvm/fpsimd.c
> > @@ -90,10 +90,8 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
> > * disappears and another task or vcpu appears that recycles the same
> > * struct fpsimd_state.
> > */
> > -void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> > +static void __kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> > {
> > - local_bh_disable();
> > -
> > update_thread_flag(TIF_SVE,
> > vcpu->arch.flags & KVM_ARM64_HOST_SVE_IN_USE);
> >
> > @@ -105,6 +103,16 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> > /* Ensure user trap controls are correctly restored */
> > fpsimd_bind_task_to_cpu();
> > }
> > +}
> > +
> >
> > - local_bh_enable();
> > +void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> > +{
> > + if (irqs_disabled())
> > + __kvm_arch_vcpu_put_fp(vcpu);
> > + else {
> > + local_bh_disable();
> > + __kvm_arch_vcpu_put_fp(vcpu);
> > + local_bh_enable();
> > + }
> > }
> >
>
> I'm of the opinion to always run kvm_arch_vcpu_put_fp() with interrupt
> disabled. local_bh_enable() does quite a lot of stuff (running the
> softirqs), which adds overhead we could do without.
>
> I'd replace local_bh_{disable,enable} with local_irq_{save,restore).
I don't have a huge problem with that. This creates interrupt blackout
on run loop exit, but it's a) not worse than the blackout in
__schedule() and b) presumably the rare case compared with run loop
preemption.
So, while disabling interrupts seemed a bit brutal, in context it
doesn't look like such a big deal.
Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
