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: http://www.kraxel.org/cgit/qemu/commit/?h=usb.57&id=f0ad01f92ca02eee7cadbfd225c5de753ebd5fce Since setting reporting 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: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=361aabf395e4a23cf554cf4ec0c0c6963b8beb01 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 have 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 <hdego...@redhat.com> --- hw/usb/hcd-ehci.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index 444db15..fb3547d 100644 --- a/hw/usb/hcd-ehci.c +++ b/hw/usb/hcd-ehci.c @@ -1261,7 +1261,7 @@ static void ehci_opreg_write(void *ptr, target_phys_addr_t addr, s->usbcmd = val; /* Set usbcmd for ehci_update_halt() */ ehci_update_halt(s); s->async_stepdown = 0; - qemu_mod_timer(s->frame_timer, qemu_get_clock_ns(vm_clock)); + qemu_bh_schedule(s->async_bh); } break; @@ -2534,12 +2534,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, @@ -2770,7 +2764,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); QTAILQ_INIT(&s->aqueues); QTAILQ_INIT(&s->pqueues); usb_packet_init(&s->ipacket); -- 1.7.12.1
