KVM: PIC: enhance IPI avoidance

KVM: PIC: enhance IPI avoidance

[Marcelo Tosatti]

KVM: PIC: enhance IPI avoidance

The PIC code makes little effort to avoid kvm_vcpu_kick(), resulting in
unnecessary guest exits in some conditions.

For example, if the timer interrupt is routed through the IOAPIC, IRR
for IRQ 0 will get set but not cleared, since the APIC is handling the
acks.

This means that everytime an interrupt < 16 is triggered, the priority
logic will find IRQ0 pending and send an IPI to vcpu0 (in case IRQ0 is
not masked, which is Linux's case).

Introduce a new variable isr_ack to represent the IRQ's for which the
guest has been signalled / cleared the ISR. Use it to avoid more than
one IPI per trigger-ack cycle, in addition to the avoidance when ISR is
set in get_priority().

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>


Index: kvm/arch/x86/kvm/i8259.c
===================================================================
--- kvm.orig/arch/x86/kvm/i8259.c
+++ kvm/arch/x86/kvm/i8259.c
@@ -33,6 +33,7 @@
 static void pic_clear_isr(struct kvm_kpic_state *s, int irq)
 {
 	s->isr &= ~(1 << irq);
+	s->isr_ack |= (1 << irq);
 }
 
 /*
@@ -213,6 +214,7 @@ void kvm_pic_reset(struct kvm_kpic_state
 	s->irr = 0;
 	s->imr = 0;
 	s->isr = 0;
+	s->isr_ack = 0xff;
 	s->priority_add = 0;
 	s->irq_base = 0;
 	s->read_reg_select = 0;
@@ -444,10 +446,14 @@ static void pic_irq_request(void *opaque
 {
 	struct kvm *kvm = opaque;
 	struct kvm_vcpu *vcpu = kvm->vcpus[0];
+	struct kvm_pic *s = pic_irqchip(kvm);
+	int irq = pic_get_irq(&s->pics[0]);
 
-	pic_irqchip(kvm)->output = level;
-	if (vcpu)
+	s->output = level;
+	if (vcpu && level && (s->pics[0].isr_ack & (1 << irq))) {
+		s->pics[0].isr_ack &= ~(1 << irq);
 		kvm_vcpu_kick(vcpu);
+	}
 }
 
 struct kvm_pic *kvm_create_pic(struct kvm *kvm)
Index: kvm/arch/x86/kvm/irq.h
===================================================================
--- kvm.orig/arch/x86/kvm/irq.h
+++ kvm/arch/x86/kvm/irq.h
@@ -42,6 +42,7 @@ struct kvm_kpic_state {
 	u8 irr;		/* interrupt request register */
 	u8 imr;		/* interrupt mask register */
 	u8 isr;		/* interrupt service register */
+	u8 isr_ack;	/* interrupt ack detection */
 	u8 priority_add;	/* highest irq priority */
 	u8 irq_base;
 	u8 read_reg_select;

Re: KVM: PIC: enhance IPI avoidance

[David S. Ahern]

This patched worked very nicely for me -- about an 8% performance
improvement for my workload.

david


Marcelo Tosatti wrote:
> KVM: PIC: enhance IPI avoidance
> 
> The PIC code makes little effort to avoid kvm_vcpu_kick(), resulting in
> unnecessary guest exits in some conditions.
> 
> For example, if the timer interrupt is routed through the IOAPIC, IRR
> for IRQ 0 will get set but not cleared, since the APIC is handling the
> acks.
> 
> This means that everytime an interrupt < 16 is triggered, the priority
> logic will find IRQ0 pending and send an IPI to vcpu0 (in case IRQ0 is
> not masked, which is Linux's case).
> 
> Introduce a new variable isr_ack to represent the IRQ's for which the
> guest has been signalled / cleared the ISR. Use it to avoid more than
> one IPI per trigger-ack cycle, in addition to the avoidance when ISR is
> set in get_priority().
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> 
> Index: kvm/arch/x86/kvm/i8259.c
> ===================================================================
> --- kvm.orig/arch/x86/kvm/i8259.c
> +++ kvm/arch/x86/kvm/i8259.c
> @@ -33,6 +33,7 @@
>  static void pic_clear_isr(struct kvm_kpic_state *s, int irq)
>  {
>  	s->isr &= ~(1 << irq);
> +	s->isr_ack |= (1 << irq);
>  }
>  
>  /*
> @@ -213,6 +214,7 @@ void kvm_pic_reset(struct kvm_kpic_state
>  	s->irr = 0;
>  	s->imr = 0;
>  	s->isr = 0;
> +	s->isr_ack = 0xff;
>  	s->priority_add = 0;
>  	s->irq_base = 0;
>  	s->read_reg_select = 0;
> @@ -444,10 +446,14 @@ static void pic_irq_request(void *opaque
>  {
>  	struct kvm *kvm = opaque;
>  	struct kvm_vcpu *vcpu = kvm->vcpus[0];
> +	struct kvm_pic *s = pic_irqchip(kvm);
> +	int irq = pic_get_irq(&s->pics[0]);
>  
> -	pic_irqchip(kvm)->output = level;
> -	if (vcpu)
> +	s->output = level;
> +	if (vcpu && level && (s->pics[0].isr_ack & (1 << irq))) {
> +		s->pics[0].isr_ack &= ~(1 << irq);
>  		kvm_vcpu_kick(vcpu);
> +	}
>  }
>  
>  struct kvm_pic *kvm_create_pic(struct kvm *kvm)
> Index: kvm/arch/x86/kvm/irq.h
> ===================================================================
> --- kvm.orig/arch/x86/kvm/irq.h
> +++ kvm/arch/x86/kvm/irq.h
> @@ -42,6 +42,7 @@ struct kvm_kpic_state {
>  	u8 irr;		/* interrupt request register */
>  	u8 imr;		/* interrupt mask register */
>  	u8 isr;		/* interrupt service register */
> +	u8 isr_ack;	/* interrupt ack detection */
>  	u8 priority_add;	/* highest irq priority */
>  	u8 irq_base;
>  	u8 read_reg_select;
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: KVM: PIC: enhance IPI avoidance

[Avi Kivity]

Marcelo Tosatti wrote:
> KVM: PIC: enhance IPI avoidance
>
> The PIC code makes little effort to avoid kvm_vcpu_kick(), resulting in
> unnecessary guest exits in some conditions.
>
> For example, if the timer interrupt is routed through the IOAPIC, IRR
> for IRQ 0 will get set but not cleared, since the APIC is handling the
> acks.
>
> This means that everytime an interrupt < 16 is triggered, the priority
> logic will find IRQ0 pending and send an IPI to vcpu0 (in case IRQ0 is
> not masked, which is Linux's case).
>
> Introduce a new variable isr_ack to represent the IRQ's for which the
> guest has been signalled / cleared the ISR. Use it to avoid more than
> one IPI per trigger-ack cycle, in addition to the avoidance when ISR is
> set in get_priority().
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
>  struct kvm_pic *kvm_create_pic(struct kvm *kvm)
> Index: kvm/arch/x86/kvm/irq.h
> ===================================================================
> --- kvm.orig/arch/x86/kvm/irq.h
> +++ kvm/arch/x86/kvm/irq.h
> @@ -42,6 +42,7 @@ struct kvm_kpic_state {
>  	u8 irr;		/* interrupt request register */
>  	u8 imr;		/* interrupt mask register */
>  	u8 isr;		/* interrupt service register */
> +	u8 isr_ack;	/* interrupt ack detection */
>  	u8 priority_add;	/* highest irq priority */
>  	u8 irq_base;
>  	u8 read_reg_select;
>   

Needs to be cleared by reset and by register load from userspace, no?

-- 
error compiling committee.c: too many arguments to function

Re: KVM: PIC: enhance IPI avoidance

[Marcelo Tosatti]

On Wed, Sep 24, 2008 at 03:19:47PM +0300, Avi Kivity wrote:
>> Index: kvm/arch/x86/kvm/irq.h
>> ===================================================================
>> --- kvm.orig/arch/x86/kvm/irq.h
>> +++ kvm/arch/x86/kvm/irq.h
>> @@ -42,6 +42,7 @@ struct kvm_kpic_state {
>>  	u8 irr;		/* interrupt request register */
>>  	u8 imr;		/* interrupt mask register */
>>  	u8 isr;		/* interrupt service register */
>> +	u8 isr_ack;	/* interrupt ack detection */
>>  	u8 priority_add;	/* highest irq priority */
>>  	u8 irq_base;
>>  	u8 read_reg_select;
>>   
>
> Needs to be cleared by reset 

@@ -213,6 +214,7 @@ void kvm_pic_reset(struct kvm_kpic_state
        s->irr = 0;
        s->imr = 0;
        s->isr = 0;
+       s->isr_ack = 0xff;
        s->priority_add = 0;
        s->irq_base = 0;
        s->read_reg_select = 0;

> and by register load from userspace, no?

Isnt that responsability of the guest? Unacked IOAPIC interrupts are not
cleared on register load, are they?

Re: KVM: PIC: enhance IPI avoidance

[Avi Kivity]

Marcelo Tosatti wrote:
>   
>> and by register load from userspace, no?
>>     
>
> Isnt that responsability of the guest? 

I'm talking about a restore to previous state scenario.  In this case we 
want to disable any IPI avoidance in case it avoids a needed IPI.

> Unacked IOAPIC interrupts are not
> cleared on register load, are they?
>
>   

Good question.  I don't know if they should or shouldn't.  But that's a 
different question.  isr_ack is not guest visible, so nothing is lost 
from clearing it, but we can fail if we don't clear it.

-- 
error compiling committee.c: too many arguments to function

Re: KVM: PIC: enhance IPI avoidance

[Marcelo Tosatti]

On Wed, Sep 24, 2008 at 05:49:37PM +0300, Avi Kivity wrote:
> Marcelo Tosatti wrote:
>>   
>>> and by register load from userspace, no?
>>>     
>>
>> Isnt that responsability of the guest? 
>
> I'm talking about a restore to previous state scenario.  In this case we  
> want to disable any IPI avoidance in case it avoids a needed IPI.
>
>> Unacked IOAPIC interrupts are not
>> cleared on register load, are they?
>>
>>   
>
> Good question.  I don't know if they should or shouldn't.  But that's a  
> different question.  isr_ack is not guest visible, so nothing is lost  
> from clearing it, but we can fail if we don't clear it.

True. Anything other potential problem you could think of?

KVM: PIC: enhance IPI avoidance

The PIC code makes little effort to avoid kvm_vcpu_kick(), resulting in
unnecessary guest exits in some conditions.

For example, if the timer interrupt is routed through the IOAPIC, IRR
for IRQ 0 will get set but not cleared, since the APIC is handling the
acks.

This means that everytime an interrupt < 16 is triggered, the priority
logic will find IRQ0 pending and send an IPI to vcpu0 (in case IRQ0 is
not masked, which is Linux's case).

Introduce a new variable isr_ack to represent the IRQ's for which the
guest has been signalled / cleared the ISR. Use it to avoid more than
one IPI per trigger-ack cycle, in addition to the avoidance when ISR is
set in get_priority().


Index: kvm/arch/x86/kvm/i8259.c
===================================================================
--- kvm.orig/arch/x86/kvm/i8259.c
+++ kvm/arch/x86/kvm/i8259.c
@@ -33,6 +33,14 @@
 static void pic_clear_isr(struct kvm_kpic_state *s, int irq)
 {
 	s->isr &= ~(1 << irq);
+	s->isr_ack |= (1 << irq);
+}
+
+void kvm_pic_clear_isr_ack(struct kvm *kvm)
+{
+	struct kvm_pic *s = pic_irqchip(kvm);
+	s->pics[0].isr_ack = 0xff;
+	s->pics[1].isr_ack = 0xff;
 }
 
 /*
@@ -213,6 +221,7 @@ void kvm_pic_reset(struct kvm_kpic_state
 	s->irr = 0;
 	s->imr = 0;
 	s->isr = 0;
+	s->isr_ack = 0xff;
 	s->priority_add = 0;
 	s->irq_base = 0;
 	s->read_reg_select = 0;
@@ -444,10 +453,14 @@ static void pic_irq_request(void *opaque
 {
 	struct kvm *kvm = opaque;
 	struct kvm_vcpu *vcpu = kvm->vcpus[0];
+	struct kvm_pic *s = pic_irqchip(kvm);
+	int irq = pic_get_irq(&s->pics[0]);
 
-	pic_irqchip(kvm)->output = level;
-	if (vcpu)
+	s->output = level;
+	if (vcpu && level && (s->pics[0].isr_ack & (1 << irq))) {
+		s->pics[0].isr_ack &= ~(1 << irq);
 		kvm_vcpu_kick(vcpu);
+	}
 }
 
 struct kvm_pic *kvm_create_pic(struct kvm *kvm)
Index: kvm/arch/x86/kvm/irq.h
===================================================================
--- kvm.orig/arch/x86/kvm/irq.h
+++ kvm/arch/x86/kvm/irq.h
@@ -42,6 +42,7 @@ struct kvm_kpic_state {
 	u8 irr;		/* interrupt request register */
 	u8 imr;		/* interrupt mask register */
 	u8 isr;		/* interrupt service register */
+	u8 isr_ack;	/* interrupt ack detection */
 	u8 priority_add;	/* highest irq priority */
 	u8 irq_base;
 	u8 read_reg_select;
@@ -70,6 +71,7 @@ struct kvm_pic *kvm_create_pic(struct kv
 void kvm_pic_set_irq(void *opaque, int irq, int level);
 int kvm_pic_read_irq(struct kvm *kvm);
 void kvm_pic_update_irq(struct kvm_pic *s);
+void kvm_pic_clear_isr_ack(struct kvm *kvm);
 
 static inline struct kvm_pic *pic_irqchip(struct kvm *kvm)
 {
Index: kvm/arch/x86/kvm/x86.c
===================================================================
--- kvm.orig/arch/x86/kvm/x86.c
+++ kvm/arch/x86/kvm/x86.c
@@ -3963,6 +3963,7 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct
 			pr_debug("Set back pending irq %d\n",
 				 pending_vec);
 		}
+		kvm_pic_clear_isr_ack(vcpu->kvm);
 	}
 
 	kvm_set_segment(vcpu, &sregs->cs, VCPU_SREG_CS);

Re: KVM: PIC: enhance IPI avoidance

[Avi Kivity]

Marcelo Tosatti wrote:
> True. Anything other potential problem you could think of?
>
>   

No, so applied the patch.  Thanks.

-- 
error compiling committee.c: too many arguments to function