diff mbox

[2/9] KVM: arm/arm64: vgic: Avoid flushing vgic state when there's no pending IRQ

Message ID 20170320105818.20481-3-cdall@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Christoffer Dall March 20, 2017, 10:58 a.m. UTC
From: Shih-Wei Li <shihwei@cs.columbia.edu>

We do not need to flush vgic states in each world switch unless
there is pending IRQ queued to the vgic's ap list. We can thus reduce
the overhead by not grabbing the spinlock and not making the extra
function call to vgic_flush_lr_state.

Signed-off-by: Shih-Wei Li <shihwei@cs.columbia.edu>
Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 virt/kvm/arm/vgic/vgic.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Marc Zyngier March 21, 2017, 10:57 a.m. UTC | #1
On 20/03/17 10:58, Christoffer Dall wrote:
> From: Shih-Wei Li <shihwei@cs.columbia.edu>
> 
> We do not need to flush vgic states in each world switch unless
> there is pending IRQ queued to the vgic's ap list. We can thus reduce
> the overhead by not grabbing the spinlock and not making the extra
> function call to vgic_flush_lr_state.
> 
> Signed-off-by: Shih-Wei Li <shihwei@cs.columbia.edu>
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> ---
>  virt/kvm/arm/vgic/vgic.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index 2ac0def..1436c2e 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -637,12 +637,17 @@ static void vgic_flush_lr_state(struct kvm_vcpu *vcpu)
>  /* Sync back the hardware VGIC state into our emulation after a guest's run. */
>  void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
>  {
> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> +
>  	if (unlikely(!vgic_initialized(vcpu->kvm)))
>  		return;
>  
>  	vgic_process_maintenance_interrupt(vcpu);
>  	vgic_fold_lr_state(vcpu);
>  	vgic_prune_ap_list(vcpu);
> +
> +	/* Make sure we can fast-path in flush_hwstate */
> +	vgic_cpu->used_lrs = 0;
>  }
>  
>  /* Flush our emulation state into the GIC hardware before entering the guest. */
> @@ -651,6 +656,9 @@ void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
>  	if (unlikely(!vgic_initialized(vcpu->kvm)))
>  		return;
>  
> +	if (list_empty(&vcpu->arch.vgic_cpu.ap_list_head))
> +		return;
> +

While I can see the READ_ONCE in list_empty(), it is not completely
obvious that the race with another CPU injecting an interrupt is safe
(if I get it correctly, it will have the same effect as if it was added
right after the critical section below).

Can we have a nice comment explaining this?

>  	spin_lock(&vcpu->arch.vgic_cpu.ap_list_lock);
>  	vgic_flush_lr_state(vcpu);
>  	spin_unlock(&vcpu->arch.vgic_cpu.ap_list_lock);
> 

Thanks,

	M.
Christoffer Dall March 21, 2017, 11:17 a.m. UTC | #2
On Tue, Mar 21, 2017 at 10:57:49AM +0000, Marc Zyngier wrote:
> On 20/03/17 10:58, Christoffer Dall wrote:
> > From: Shih-Wei Li <shihwei@cs.columbia.edu>
> > 
> > We do not need to flush vgic states in each world switch unless
> > there is pending IRQ queued to the vgic's ap list. We can thus reduce
> > the overhead by not grabbing the spinlock and not making the extra
> > function call to vgic_flush_lr_state.
> > 
> > Signed-off-by: Shih-Wei Li <shihwei@cs.columbia.edu>
> > Signed-off-by: Christoffer Dall <cdall@linaro.org>
> > ---
> >  virt/kvm/arm/vgic/vgic.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> > index 2ac0def..1436c2e 100644
> > --- a/virt/kvm/arm/vgic/vgic.c
> > +++ b/virt/kvm/arm/vgic/vgic.c
> > @@ -637,12 +637,17 @@ static void vgic_flush_lr_state(struct kvm_vcpu *vcpu)
> >  /* Sync back the hardware VGIC state into our emulation after a guest's run. */
> >  void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
> >  {
> > +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> > +
> >  	if (unlikely(!vgic_initialized(vcpu->kvm)))
> >  		return;
> >  
> >  	vgic_process_maintenance_interrupt(vcpu);
> >  	vgic_fold_lr_state(vcpu);
> >  	vgic_prune_ap_list(vcpu);
> > +
> > +	/* Make sure we can fast-path in flush_hwstate */
> > +	vgic_cpu->used_lrs = 0;
> >  }
> >  
> >  /* Flush our emulation state into the GIC hardware before entering the guest. */
> > @@ -651,6 +656,9 @@ void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
> >  	if (unlikely(!vgic_initialized(vcpu->kvm)))
> >  		return;
> >  
> > +	if (list_empty(&vcpu->arch.vgic_cpu.ap_list_head))
> > +		return;
> > +
> 
> While I can see the READ_ONCE in list_empty(), it is not completely
> obvious that the race with another CPU injecting an interrupt is safe
> (if I get it correctly, it will have the same effect as if it was added
> right after the critical section below).

Yes, you either observe virtual interrupts or not, that's a benign race.

> 
> Can we have a nice comment explaining this?
> 

You sure can, I will add something.

> >  	spin_lock(&vcpu->arch.vgic_cpu.ap_list_lock);
> >  	vgic_flush_lr_state(vcpu);
> >  	spin_unlock(&vcpu->arch.vgic_cpu.ap_list_lock);
> > 
> 

Thanks,
-Christoffer
diff mbox

Patch

diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index 2ac0def..1436c2e 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -637,12 +637,17 @@  static void vgic_flush_lr_state(struct kvm_vcpu *vcpu)
 /* Sync back the hardware VGIC state into our emulation after a guest's run. */
 void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
 {
+	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+
 	if (unlikely(!vgic_initialized(vcpu->kvm)))
 		return;
 
 	vgic_process_maintenance_interrupt(vcpu);
 	vgic_fold_lr_state(vcpu);
 	vgic_prune_ap_list(vcpu);
+
+	/* Make sure we can fast-path in flush_hwstate */
+	vgic_cpu->used_lrs = 0;
 }
 
 /* Flush our emulation state into the GIC hardware before entering the guest. */
@@ -651,6 +656,9 @@  void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
 	if (unlikely(!vgic_initialized(vcpu->kvm)))
 		return;
 
+	if (list_empty(&vcpu->arch.vgic_cpu.ap_list_head))
+		return;
+
 	spin_lock(&vcpu->arch.vgic_cpu.ap_list_lock);
 	vgic_flush_lr_state(vcpu);
 	spin_unlock(&vcpu->arch.vgic_cpu.ap_list_lock);