On Mon, 10 Sep 2012 16:43:59 -0400, Christoffer Dall
<c.dall@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, Sep 10, 2012 at 4:33 PM, Marc Zyngier <marc.zyngier@xxxxxxx>
wrote:
>> 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.
>>
>
> it doesn't take very long, I would declare is_edge as a bool and do an
> is_level, like so:
>
> bool is_edge, is_level;
> ...
> is_edge = ...;
> is_level = !is_edge;
>
> /*
> * Only inject an interrupt if either:
> * - level triggered and we change level, or
> * - edge triggered and we have a rising edge
> */
> if (is_level && !(state ^ level) ||
> is_edge && (state || !level)) {
> vgic_bitmap_set_irq_val(&dist->irq_state, cpuid,
> irq_num, level);
> spin_unlock_irqrestore(&dist->lock, flags);
> return false;
> }
>
>
>>>
>>>> +
>>>> + 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.
>>
>
> ugh, doesn't sound good. Details?
Just a thinko when assigning the target to dist->irq_spi_cpu...
M.
--
Fast, cheap, reliable. Pick two.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
