On Mon, 10 Sep 2012 16:23:15 -0400, Christoffer Dall
<c.dall@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, Sep 5, 2012 at 1:02 PM, Marc Zyngier <marc.zyngier@xxxxxxx>
wrote:
>> Recomputing the whole GIC state when injecting a single interrupt
>> is a tiny bit silly.
>>
>> Instead, just computing the new state for this particular IRQ saves
>> a bit of processing and makes things noticeably faster.
>
> noticeably how?
By being able to inject about 10% more interrupts.
>
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>> ---
>> arch/arm/include/asm/kvm_vgic.h | 1 +
>> arch/arm/kvm/vgic.c | 69
>> ++++++++++++++++++++++++++++++-----------
>> 2 files changed, 52 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_vgic.h
>> b/arch/arm/include/asm/kvm_vgic.h
>> index 633e697..1f7fd32 100644
>> --- a/arch/arm/include/asm/kvm_vgic.h
>> +++ b/arch/arm/include/asm/kvm_vgic.h
>> @@ -183,6 +183,7 @@ struct vgic_dist {
>> u8 irq_sgi_sources[VGIC_MAX_CPUS][16];
>>
>> /* Target CPU for each IRQ */
>> + u8 irq_spi_cpu[VGIC_NR_SHARED_IRQS];
>> struct vgic_bitmap irq_spi_target[VGIC_MAX_CPUS];
>>
>> /* Bitmap indicating which CPU has something pending */
>> diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c
>> index 5177792..5f32772 100644
>> --- a/arch/arm/kvm/vgic.c
>> +++ b/arch/arm/kvm/vgic.c
>> @@ -47,6 +47,9 @@
>> * registers, stored on each vcpu. We only keep one bit of
>> * information per interrupt, making sure that only one vcpu can
>> * accept the interrupt.
>> + * The same is true when injecting an interrupt, except that we only
>> + * consider a single interrupt at a time. The irq_spi_cpu array
>> + * contains the target CPU for each SPI.
>> *
>> * The handling of level interrupts adds some extra complexity. We
>> * need to track when the interrupt has been EOIed, so we can sample
>> @@ -313,6 +316,7 @@ static void vgic_set_target_reg(struct kvm *kvm,
u32
>> val, int irq)
>> int shift = i * 8;
>> target = ffs((val >> shift) & 0xffU);
>> target = target ? (target - 1) : 0;
>> + dist->irq_spi_cpu[irq] = target;
>> kvm_for_each_vcpu(c, vcpu, kvm) {
>> bmap =
>>
vgic_bitmap_get_shared_map(&dist->irq_spi_target[c]);
>> if (c == target)
>> @@ -866,23 +870,19 @@ static void vgic_kick_vcpus(struct kvm *kvm)
>> }
>> }
>>
>> -int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, const struct
>> kvm_irq_level *irq)
>> +static bool vgic_update_irq_state(struct kvm *kvm, int cpuid,
>> + const struct kvm_irq_level *irq)
>> {
>> struct vgic_dist *dist = &kvm->arch.vgic;
>> - int nrcpus = atomic_read(&kvm->online_vcpus);
>> - int is_edge, state;
>> + struct kvm_vcpu *vcpu;
>> + int is_edge, state, level;
>> + int pend, enabled;
>> unsigned long flags;
>> - bool updated_state = false;
>>
>> - if (cpuid >= nrcpus)
>> - return -EINVAL;
>> -
>> - /* Only PPIs or SPIs */
>> - if (irq->irq >= VGIC_NR_IRQS || irq->irq < 16)
>> - return -EINVAL;
>> + level = !!irq->level;
>>
>> - kvm_debug("Inject IRQ%d\n", irq->irq);
>> spin_lock_irqsave(&dist->lock, flags);
>> +
>> is_edge = vgic_irq_is_edge(dist, irq->irq);
>> state = vgic_bitmap_get_irq_val(&dist->irq_state, cpuid,
>> irq->irq);
>>
>> @@ -891,16 +891,49 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int
>> cpuid, const struct kvm_irq_level *
>> * - level triggered and we change level
>> * - edge triggered and we have a rising edge
>> */
>> - if ((!is_edge && (state ^ !!irq->level)) ||
>> - (is_edge && !state && irq->level)) {
>> - vgic_bitmap_set_irq_val(&dist->irq_state, cpuid,
>> - irq->irq, !!irq->level);
>> - vgic_update_state(kvm);
>> - updated_state = true;
>> + if (!((!is_edge && (state ^ level)) ||
>> + (is_edge && !state && level))) {
>> + spin_unlock_irqrestore(&dist->lock, flags);
>> + return false;
>> + }
>
> wow, this one really hurts now, I'll probably have a go with my good
> old friend De Morgan here and try to get rid of the double negation
> and fit the comment a little better.
I thought of that too but gave up, more pressing issues to solve.
>
>> +
>> + vgic_bitmap_set_irq_val(&dist->irq_state, cpuid, irq->irq,
>> level);
>> +
>> + enabled = vgic_bitmap_get_irq_val(&dist->irq_enabled, cpuid,
>> irq->irq);
>> + pend = level && enabled;
>> +
>> + if (irq->irq >= 32) {
>> + cpuid = dist->irq_spi_cpu[irq->irq - 32];
>> + pend &=
>> vgic_bitmap_get_irq_val(&dist->irq_spi_target[cpuid],
>> + 0, irq->irq);
>> }
>> +
>> + kvm_debug("Inject IRQ%d level %d CPU%d\n", irq->irq, level,
>> cpuid);
>> +
>> + vcpu = kvm_get_vcpu(kvm, cpuid);
>> + if (pend) {
>> + set_bit(irq->irq, vcpu->arch.vgic_cpu.pending);
>> + set_bit(cpuid, &dist->irq_pending_on_cpu);
>> + } else
>> + clear_bit(irq->irq, vcpu->arch.vgic_cpu.pending);
>> +
>> spin_unlock_irqrestore(&dist->lock, flags);
>>
>> - if (updated_state)
>> + return true;
>> +}
>> +
>> +int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, const struct
>> kvm_irq_level *irq)
>> +{
>> + int nrcpus = atomic_read(&kvm->online_vcpus);
>> +
>> + if (cpuid >= nrcpus)
>> + return -EINVAL;
>> +
>> + /* Only PPIs or SPIs */
>> + if (irq->irq >= VGIC_NR_IRQS || irq->irq < 16)
>> + return -EINVAL;
>> +
>> + if (vgic_update_irq_state(kvm, cpuid, irq))
>> vgic_kick_vcpus(kvm);
>>
>> return 0;
>> --
>> 1.7.12
>>
>
> this looks reasonable, I'll apply it on top of the irq rework and
> change it so it fits.
>
> I assume you've tested this as usual?
Yes, but I now have an updated version (this one breaks interrupt
migration in a not-so-subtle way). I also have another couple of VGIC
related patches that relax the locking and make things look really nice in
perf.
I'll post all of this tomorrow, based on your branch (I was in the process
of rebasing my stuff when I started investigating the swap madness).
M.
--
Fast, cheap, reliable. Pick two.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
