[05/22] ehci: Improve latency of interrupt delivery and async schedule scanning

Submitted by Hans de Goede on Oct. 15, 2012, 10:38 a.m.


Message ID 1350297511-25437-6-git-send-email-hdegoede@redhat.com
State New
Headers show

Commit Message

Hans de Goede Oct. 15, 2012, 10:38 a.m.
While doing various performance tests of reading from USB mass storage devices
I noticed the following::
1) When an async handled packet completes, we don't immediately report an
   interrupt to the guest, instead we wait for the frame-timer to run and
   report it from there
2) If 1) has been fixed and an async handled packet takes a while to complete,
   then async_stepdown will become a high value, which means that there
   will be a large latency before any new packets queued by the guest in
   response to the interrupt get seen

1) was done deliberately as part of commit f0ad01f92:
Since setting the interrupt immediately on async packet completion was causing
issues with Linux guests, I believe this recently fixed Linux bug explains
why this is happening:

Note that we can *not* count on this fix being present in all Linux guests!

I was hoping that the recently added support for Interrupt Threshold Control
would fix the issues with Linux guests, but adding a simple ehci_commit_irq()
call to ehci_async_bh() still caused problems with Linux guests.

The problem is, that when doing ehci_commit_irq() from ehci_async_bh(),
the "old" frindex value is used to calculate usbsts_frindex, and when
the frame-timer then runs possibly very shortly after ehci_async_bh(),
it increases the frame-timer, and thus any interrupts raised from that
frame-timer run, will also get reported to the guest immediately, rather
then being delayed to the next frame-timer run.

Luckily the solution for this is simple, this means that we need to
increase frindex before calling ehci_commit_irq() from ehci_async_bh(),
which in the end boils down to simple calling ehci_frame_timer() instead
of ehci_async_bh() from the bh.

This may seem like it causes a lot of extra work to be done, but this
is not true. Any work done from the frame-timer processing the periodic
schedule is work which then does not need to be done the next time the
frame timer runs, also the frame-timer will re-arm itself at (possibly)
a later time then it was armed for saving a vmexit at that time.

As an additional advantage moving to simply calling the frame-timer also
fixes 2) as the packet completion will set async_stepdown to 0, and the
re-arming of the timer with an async_stepdown of 0 ensures that any
newly queued up packets get seen in a reasonable amount of time.

This improves the speed (MB/s) of a Linux guest reading from a USB mass
storage device by a factor of 1.5 - 1.7 with input pipelining disabled,
and by a factor of 1.8 with input pipelining enabled.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
 hw/usb/hcd-ehci.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

Patch hide | download patch | download mbox

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index d9d4918..bbfa441 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1244,7 +1244,7 @@  static void ehci_opreg_write(void *ptr, target_phys_addr_t addr,
             s->usbcmd = val; /* Set usbcmd for ehci_update_halt() */
             s->async_stepdown = 0;
-            qemu_mod_timer(s->frame_timer, qemu_get_clock_ns(vm_clock));
+            qemu_bh_schedule(s->async_bh);
@@ -2509,12 +2509,6 @@  static void ehci_frame_timer(void *opaque)
-static void ehci_async_bh(void *opaque)
-    EHCIState *ehci = opaque;
-    ehci_advance_async_state(ehci);
 static const MemoryRegionOps ehci_mmio_caps_ops = {
     .read = ehci_caps_read,
     .valid.min_access_size = 1,
@@ -2743,7 +2737,7 @@  static int usb_ehci_initfn(PCIDevice *dev)
     s->frame_timer = qemu_new_timer_ns(vm_clock, ehci_frame_timer, s);
-    s->async_bh = qemu_bh_new(ehci_async_bh, s);
+    s->async_bh = qemu_bh_new(ehci_frame_timer, s);