| Message ID | 1425479753-18349-4-git-send-email-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 4, 2015 at 8:35 AM, Alex Bennée <alex.bennee@linaro.org> wrote: > While observing KVM traces I can see additional IRQ calls on pretty much > every MMIO access which is just plain inefficient. Only update the QEMU > IRQ level if something has actually changed from last time. Otherwise we > may be papering over other failure modes. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > diff --git a/hw/char/pl011.c b/hw/char/pl011.c > index 0a45115..bb554bc 100644 > --- a/hw/char/pl011.c > +++ b/hw/char/pl011.c > @@ -36,6 +36,9 @@ typedef struct PL011State { > CharDriverState *chr; > qemu_irq irq; > const unsigned char *id; > + > + /* not serialised, prevents pl011_update doing extra set_irqs */ > + uint32_t current_irq; > } PL011State; > > #define PL011_INT_TX 0x20 > @@ -53,10 +56,11 @@ static const unsigned char pl011_id_luminary[8] = > > static void pl011_update(PL011State *s) > { > - uint32_t flags; > - > - flags = s->int_level & s->int_enabled; > - qemu_set_irq(s->irq, flags != 0); > + uint32_t flags = s->int_level & s->int_enabled; > + if (flags != s->current_irq) { > + s->current_irq = flags; > + qemu_set_irq(s->irq, s->current_irq != 0); > + } > } > > static uint64_t pl011_read(void *opaque, hwaddr offset, > -- > 2.3.1 > > Did you find the source of the additional IRQs? This seems like it could be potentially be hiding an issue. From looking at the code, it appears that one source of the additional updates is in pl011_read, where we possibly do an update without a change in int_level. I wonder if finding and fixing the updates is a better approach.
On 4 March 2015 at 14:35, Alex Bennée <alex.bennee@linaro.org> wrote: > While observing KVM traces I can see additional IRQ calls on pretty much > every MMIO access which is just plain inefficient. Only update the QEMU > IRQ level if something has actually changed from last time. Otherwise we > may be papering over other failure modes. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > diff --git a/hw/char/pl011.c b/hw/char/pl011.c > index 0a45115..bb554bc 100644 > --- a/hw/char/pl011.c > +++ b/hw/char/pl011.c > @@ -36,6 +36,9 @@ typedef struct PL011State { > CharDriverState *chr; > qemu_irq irq; > const unsigned char *id; > + > + /* not serialised, prevents pl011_update doing extra set_irqs */ > + uint32_t current_irq; > } PL011State; > > #define PL011_INT_TX 0x20 > @@ -53,10 +56,11 @@ static const unsigned char pl011_id_luminary[8] = > > static void pl011_update(PL011State *s) > { > - uint32_t flags; > - > - flags = s->int_level & s->int_enabled; > - qemu_set_irq(s->irq, flags != 0); > + uint32_t flags = s->int_level & s->int_enabled; > + if (flags != s->current_irq) { > + s->current_irq = flags; > + qemu_set_irq(s->irq, s->current_irq != 0); > + } > } Consider this sequence of events: * the guest does something causing the interrupt to be asserted; int_level and int_enabled are 1, and current_irq is also now 1. We call qemu_set_irq() to raise the interrupt with the GIC * we migrate the guest to another host * on the receiving end, QEMU is in a cleanly reset state, and so current_irq, int_level and int_enabled are all zero before incoming data arrives * int_level and int_enabled are both set to 1 from the incoming data stream * the GIC itself is set to the "interrupt is asserted" state by its own incoming data * current_irq remains zero, because it's not migrated * the guest is resumed, and does something to deassert the interrupt. the new 'flags' value is zero * because flags == s->current_irq, we don't call qemu_set_irq, and so we've just dropped the deassert of this interrupt on the floor. -- PMM
On 12 March 2015 at 15:51, Peter Maydell <peter.maydell@linaro.org> wrote: > On 4 March 2015 at 14:35, Alex Bennée <alex.bennee@linaro.org> wrote: >> While observing KVM traces I can see additional IRQ calls on pretty much >> every MMIO access which is just plain inefficient. Only update the QEMU >> IRQ level if something has actually changed from last time. Otherwise we >> may be papering over other failure modes. > Consider this sequence of events: Incidentally, the code review rule of thumb that led me to construct that scenario is: * state inside the device struct which doesn't correspond to real hardware state is suspicious * state which doesn't have any handling on migration save/restore is doubly suspicious ...and then it was just a matter of "find the situation where this is broken" :-) If we want to avoid making syscalls back into KVM it might be better to attack the problem in the GIC object rather than in all the devices that might be connected to it. In general QEMU devices tend to assume they can just always call qemu_set_irq() and it's the other end's job to avoid doing anything too expensive in that situation. -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 12 March 2015 at 15:51, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 4 March 2015 at 14:35, Alex Bennée <alex.bennee@linaro.org> wrote: >>> While observing KVM traces I can see additional IRQ calls on pretty much >>> every MMIO access which is just plain inefficient. Only update the QEMU >>> IRQ level if something has actually changed from last time. Otherwise we >>> may be papering over other failure modes. > >> Consider this sequence of events: > > Incidentally, the code review rule of thumb that led me to > construct that scenario is: > * state inside the device struct which doesn't correspond > to real hardware state is suspicious > * state which doesn't have any handling on migration > save/restore is doubly suspicious > ...and then it was just a matter of "find the situation > where this is broken" :-) > > If we want to avoid making syscalls back into KVM it might > be better to attack the problem in the GIC object rather > than in all the devices that might be connected to it. > In general QEMU devices tend to assume they can just > always call qemu_set_irq() and it's the other end's > job to avoid doing anything too expensive in that situation. Fair enough - I'll drop this patch on the re-spin > > -- PMM
diff --git a/hw/char/pl011.c b/hw/char/pl011.c index 0a45115..bb554bc 100644 --- a/hw/char/pl011.c +++ b/hw/char/pl011.c @@ -36,6 +36,9 @@ typedef struct PL011State { CharDriverState *chr; qemu_irq irq; const unsigned char *id; + + /* not serialised, prevents pl011_update doing extra set_irqs */ + uint32_t current_irq; } PL011State; #define PL011_INT_TX 0x20 @@ -53,10 +56,11 @@ static const unsigned char pl011_id_luminary[8] = static void pl011_update(PL011State *s) { - uint32_t flags; - - flags = s->int_level & s->int_enabled; - qemu_set_irq(s->irq, flags != 0); + uint32_t flags = s->int_level & s->int_enabled; + if (flags != s->current_irq) { + s->current_irq = flags; + qemu_set_irq(s->irq, s->current_irq != 0); + } } static uint64_t pl011_read(void *opaque, hwaddr offset,
While observing KVM traces I can see additional IRQ calls on pretty much every MMIO access which is just plain inefficient. Only update the QEMU IRQ level if something has actually changed from last time. Otherwise we may be papering over other failure modes. Signed-off-by: Alex Bennée <alex.bennee@linaro.org>