[Qemu-devel] [PATCH 1/5] usb-redir: Clear iso / irq error when stopping the stream

[Qemu-devel] [PATCH 1/5] usb-redir: Clear iso / irq error when stopping the stream

[Hans de Goede]

And ignore status messages from the client which arrive after stream
stop (the stream stop send to the client and an error status reported by
the client my cross each other due to network latency).

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 usb-redir.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/usb-redir.c b/usb-redir.c
index fb91c92..7678f1a 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -410,6 +410,7 @@ static void usbredir_stop_iso_stream(USBRedirDevice *dev, uint8_t ep)
         DPRINTF("iso stream stopped ep %02X\n", ep);
         dev->endpoint[EP2I(ep)].iso_started = 0;
     }
+    dev->endpoint[EP2I(ep)].iso_error = 0;
     usbredir_free_bufpq(dev, ep);
 }
 
@@ -522,6 +523,7 @@ static void usbredir_stop_interrupt_receiving(USBRedirDevice *dev,
         DPRINTF("interrupt recv stopped ep %02X\n", ep);
         dev->endpoint[EP2I(ep)].interrupt_started = 0;
     }
+    dev->endpoint[EP2I(ep)].interrupt_error = 0;
     usbredir_free_bufpq(dev, ep);
 }
 
@@ -1029,7 +1031,7 @@ static void usbredir_iso_stream_status(void *priv, uint32_t id,
     DPRINTF("iso status %d ep %02X id %u\n", iso_stream_status->status,
             ep, id);
 
-    if (!dev->dev.attached) {
+    if (!dev->dev.attached || !dev->endpoint[EP2I(ep)].iso_started) {
         return;
     }
 
@@ -1050,7 +1052,7 @@ static void usbredir_interrupt_receiving_status(void *priv, uint32_t id,
     DPRINTF("interrupt recv status %d ep %02X id %u\n",
             interrupt_receiving_status->status, ep, id);
 
-    if (!dev->dev.attached) {
+    if (!dev->dev.attached || !dev->endpoint[EP2I(ep)].interrupt_started) {
         return;
     }
 
-- 
1.7.7.4

[Qemu-devel] [PATCH 2/5] usb-redir: Dynamically adjust iso buffering size based on ep interval

[Hans de Goede]

Note the bufpq_target_size id stored in the endpoint info struct,
even though it only used once. This is done because it will be
referenced from other code in a follow up patch.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 usb-redir.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/usb-redir.c b/usb-redir.c
index 7678f1a..76cae16 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -61,6 +61,7 @@ struct endp_data {
     uint8_t interrupt_started;
     uint8_t interrupt_error;
     QTAILQ_HEAD(, buf_packet) bufpq;
+    int bufpq_target_size;
 };
 
 struct USBRedirDevice {
@@ -332,15 +333,42 @@ static int usbredir_handle_iso_data(USBRedirDevice *dev, USBPacket *p,
                                      uint8_t ep)
 {
     int status, len;
-
     if (!dev->endpoint[EP2I(ep)].iso_started &&
             !dev->endpoint[EP2I(ep)].iso_error) {
         struct usb_redir_start_iso_stream_header start_iso = {
             .endpoint = ep,
-            /* TODO maybe do something with these depending on ep interval? */
-            .pkts_per_urb = 32,
-            .no_urbs = 3,
         };
+        int pkts_per_sec;
+
+        if (dev->dev.speed == USB_SPEED_HIGH) {
+            pkts_per_sec = 8000 / dev->endpoint[EP2I(ep)].interval;
+        } else {
+            pkts_per_sec = 1000 / dev->endpoint[EP2I(ep)].interval;
+        }
+        /* Testing has shown that we need circa 60 ms buffer */
+        dev->endpoint[EP2I(ep)].bufpq_target_size = (pkts_per_sec * 60) / 1000;
+
+        /* Aim for approx 100 interrupts / second on the client to
+           balance latency and interrupt load */
+        start_iso.pkts_per_urb = pkts_per_sec / 100;
+        if (start_iso.pkts_per_urb < 1) {
+            start_iso.pkts_per_urb = 1;
+        } else if (start_iso.pkts_per_urb > 32) {
+            start_iso.pkts_per_urb = 32;
+        }
+
+        start_iso.no_urbs = (dev->endpoint[EP2I(ep)].bufpq_target_size +
+                             start_iso.pkts_per_urb - 1) /
+                            start_iso.pkts_per_urb;
+        /* Output endpoints pre-fill only 1/2 of the packets, keeping the rest
+           as overflow buffer. Also see the usbredir protocol documentation */
+        if (!(ep & USB_DIR_IN)) {
+            start_iso.no_urbs *= 2;
+        }
+        if (start_iso.no_urbs > 16) {
+            start_iso.no_urbs = 16;
+        }
+
         /* No id, we look at the ep when receiving a status back */
         usbredirparser_send_start_iso_stream(dev->parser, 0, &start_iso);
         usbredirparser_do_write(dev->parser);
@@ -961,9 +989,24 @@ static void usbredir_ep_info(void *priv,
         dev->endpoint[i].type = ep_info->type[i];
         dev->endpoint[i].interval = ep_info->interval[i];
         dev->endpoint[i].interface = ep_info->interface[i];
-        if (dev->endpoint[i].type != usb_redir_type_invalid) {
+        switch (dev->endpoint[i].type) {
+        case usb_redir_type_invalid:
+            break;
+        case usb_redir_type_iso:
+        case usb_redir_type_interrupt:
+            if (dev->endpoint[i].interval == 0) {
+                ERROR("Received 0 interval for isoc or irq endpoint\n");
+                usbredir_device_disconnect(dev);
+            }
+            /* Fall through */
+        case usb_redir_type_control:
+        case usb_redir_type_bulk:
             DPRINTF("ep: %02X type: %d interface: %d\n", I2EP(i),
                     dev->endpoint[i].type, dev->endpoint[i].interface);
+            break;
+        default:
+            ERROR("Received invalid endpoint type\n");
+            usbredir_device_disconnect(dev);
         }
     }
 }
-- 
1.7.7.4

[Qemu-devel] [PATCH 3/5] usb-redir: Pre-fill our isoc input buffer before sending pkts to the host

[Hans de Goede]

This is something which should have been done from the first version of
usb-redir, but wasn't.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 usb-redir.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/usb-redir.c b/usb-redir.c
index 76cae16..9970b85 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -60,7 +60,9 @@ struct endp_data {
     uint8_t iso_error; /* For reporting iso errors to the HC */
     uint8_t interrupt_started;
     uint8_t interrupt_error;
+    uint8_t bufpq_prefilled;
     QTAILQ_HEAD(, buf_packet) bufpq;
+    int bufpq_size;
     int bufpq_target_size;
 };
 
@@ -296,6 +298,7 @@ static struct buf_packet *bufp_alloc(USBRedirDevice *dev,
     bufp->len    = len;
     bufp->status = status;
     QTAILQ_INSERT_TAIL(&dev->endpoint[EP2I(ep)].bufpq, bufp, next);
+    dev->endpoint[EP2I(ep)].bufpq_size++;
     return bufp;
 }
 
@@ -303,6 +306,7 @@ static void bufp_free(USBRedirDevice *dev, struct buf_packet *bufp,
     uint8_t ep)
 {
     QTAILQ_REMOVE(&dev->endpoint[EP2I(ep)].bufpq, bufp, next);
+    dev->endpoint[EP2I(ep)].bufpq_size--;
     free(bufp->data);
     g_free(bufp);
 }
@@ -374,14 +378,26 @@ static int usbredir_handle_iso_data(USBRedirDevice *dev, USBPacket *p,
         usbredirparser_do_write(dev->parser);
         DPRINTF("iso stream started ep %02X\n", ep);
         dev->endpoint[EP2I(ep)].iso_started = 1;
+        dev->endpoint[EP2I(ep)].bufpq_prefilled = 0;
     }
 
     if (ep & USB_DIR_IN) {
         struct buf_packet *isop;
 
+        if (dev->endpoint[EP2I(ep)].iso_started &&
+                !dev->endpoint[EP2I(ep)].bufpq_prefilled) {
+            if (dev->endpoint[EP2I(ep)].bufpq_size <
+                    dev->endpoint[EP2I(ep)].bufpq_target_size) {
+                return usbredir_handle_status(dev, 0, 0);
+            }
+            dev->endpoint[EP2I(ep)].bufpq_prefilled = 1;
+        }
+
         isop = QTAILQ_FIRST(&dev->endpoint[EP2I(ep)].bufpq);
         if (isop == NULL) {
             DPRINTF2("iso-token-in ep %02X, no isop\n", ep);
+            /* Re-fill the buffer */
+            dev->endpoint[EP2I(ep)].bufpq_prefilled = 0;
             /* Check iso_error for stream errors, otherwise its an underrun */
             status = dev->endpoint[EP2I(ep)].iso_error;
             dev->endpoint[EP2I(ep)].iso_error = 0;
-- 
1.7.7.4

[Qemu-devel] [PATCH 4/5] usb-redir: Try to keep our buffer size near the target size

[Hans de Goede]

Before this patch we would allow the (iso) buffer to grow unlimited
(and it would under certain circumstances) leading to way too high
latencies for iso data streams.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 usb-redir.c |   30 +++++++++++++++++++++++++++---
 1 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/usb-redir.c b/usb-redir.c
index 9970b85..7b967a6 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -61,6 +61,7 @@ struct endp_data {
     uint8_t interrupt_started;
     uint8_t interrupt_error;
     uint8_t bufpq_prefilled;
+    uint8_t bufpq_dropping_packets;
     QTAILQ_HEAD(, buf_packet) bufpq;
     int bufpq_size;
     int bufpq_target_size;
@@ -290,16 +291,34 @@ static void usbredir_cancel_packet(USBDevice *udev, USBPacket *p)
     }
 }
 
-static struct buf_packet *bufp_alloc(USBRedirDevice *dev,
+static void bufp_alloc(USBRedirDevice *dev,
     uint8_t *data, int len, int status, uint8_t ep)
 {
-    struct buf_packet *bufp = g_malloc(sizeof(struct buf_packet));
+    struct buf_packet *bufp;
+
+    if (!dev->endpoint[EP2I(ep)].bufpq_dropping_packets &&
+        dev->endpoint[EP2I(ep)].bufpq_size >
+            2 * dev->endpoint[EP2I(ep)].bufpq_target_size) {
+        DPRINTF("bufpq overflow, dropping packets ep %02X\n", ep);
+        dev->endpoint[EP2I(ep)].bufpq_dropping_packets = 1;
+    }
+    /* Since we're interupting the stream anyways, drop enough packets to get
+       back to our target buffer size */
+    if (dev->endpoint[EP2I(ep)].bufpq_dropping_packets) {
+        if (dev->endpoint[EP2I(ep)].bufpq_size >
+                dev->endpoint[EP2I(ep)].bufpq_target_size) {
+            free(data);
+            return;
+        }
+        dev->endpoint[EP2I(ep)].bufpq_dropping_packets = 0;
+    }
+
+    bufp = g_malloc(sizeof(struct buf_packet));
     bufp->data   = data;
     bufp->len    = len;
     bufp->status = status;
     QTAILQ_INSERT_TAIL(&dev->endpoint[EP2I(ep)].bufpq, bufp, next);
     dev->endpoint[EP2I(ep)].bufpq_size++;
-    return bufp;
 }
 
 static void bufp_free(USBRedirDevice *dev, struct buf_packet *bufp,
@@ -379,6 +398,7 @@ static int usbredir_handle_iso_data(USBRedirDevice *dev, USBPacket *p,
         DPRINTF("iso stream started ep %02X\n", ep);
         dev->endpoint[EP2I(ep)].iso_started = 1;
         dev->endpoint[EP2I(ep)].bufpq_prefilled = 0;
+        dev->endpoint[EP2I(ep)].bufpq_dropping_packets = 0;
     }
 
     if (ep & USB_DIR_IN) {
@@ -505,6 +525,10 @@ static int usbredir_handle_interrupt_data(USBRedirDevice *dev,
             usbredirparser_do_write(dev->parser);
             DPRINTF("interrupt recv started ep %02X\n", ep);
             dev->endpoint[EP2I(ep)].interrupt_started = 1;
+            /* We don't really want to drop interrupt packets ever, but
+               having some upper limit to how much we buffer is good. */
+            dev->endpoint[EP2I(ep)].bufpq_target_size = 1000;
+            dev->endpoint[EP2I(ep)].bufpq_dropping_packets = 0;
         }
 
         intp = QTAILQ_FIRST(&dev->endpoint[EP2I(ep)].bufpq);
-- 
1.7.7.4

[Qemu-devel] [PATCH 5/5] usb-redir: Improve some debugging messages

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 usb-redir.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/usb-redir.c b/usb-redir.c
index 7b967a6..40dfe6f 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -395,7 +395,8 @@ static int usbredir_handle_iso_data(USBRedirDevice *dev, USBPacket *p,
         /* No id, we look at the ep when receiving a status back */
         usbredirparser_send_start_iso_stream(dev->parser, 0, &start_iso);
         usbredirparser_do_write(dev->parser);
-        DPRINTF("iso stream started ep %02X\n", ep);
+        DPRINTF("iso stream started pkts/sec %d pkts/urb %d urbs %d ep %02X\n",
+                pkts_per_sec, start_iso.pkts_per_urb, start_iso.no_urbs, ep);
         dev->endpoint[EP2I(ep)].iso_started = 1;
         dev->endpoint[EP2I(ep)].bufpq_prefilled = 0;
         dev->endpoint[EP2I(ep)].bufpq_dropping_packets = 0;
@@ -415,7 +416,8 @@ static int usbredir_handle_iso_data(USBRedirDevice *dev, USBPacket *p,
 
         isop = QTAILQ_FIRST(&dev->endpoint[EP2I(ep)].bufpq);
         if (isop == NULL) {
-            DPRINTF2("iso-token-in ep %02X, no isop\n", ep);
+            DPRINTF("iso-token-in ep %02X, no isop, iso_error: %d\n",
+                    ep, dev->endpoint[EP2I(ep)].iso_error);
             /* Re-fill the buffer */
             dev->endpoint[EP2I(ep)].bufpq_prefilled = 0;
             /* Check iso_error for stream errors, otherwise its an underrun */
@@ -423,8 +425,8 @@ static int usbredir_handle_iso_data(USBRedirDevice *dev, USBPacket *p,
             dev->endpoint[EP2I(ep)].iso_error = 0;
             return usbredir_handle_status(dev, status, 0);
         }
-        DPRINTF2("iso-token-in ep %02X status %d len %d\n", ep, isop->status,
-                 isop->len);
+        DPRINTF2("iso-token-in ep %02X status %d len %d queue-size: %d\n", ep,
+                 isop->status, isop->len, dev->endpoint[EP2I(ep)].bufpq_size);
 
         status = isop->status;
         if (status != usb_redir_success) {
@@ -434,7 +436,8 @@ static int usbredir_handle_iso_data(USBRedirDevice *dev, USBPacket *p,
 
         len = isop->len;
         if (len > p->iov.size) {
-            ERROR("received iso data is larger then packet ep %02X\n", ep);
+            ERROR("received iso data is larger then packet ep %02X (%d > %d)\n",
+                  ep, len, (int)p->iov.size);
             bufp_free(dev, isop, ep);
             return USB_RET_NAK;
         }
-- 
1.7.7.4

Re: [Qemu-devel] [PATCH 1/5] usb-redir: Clear iso / irq error when stopping the stream

[Gerd Hoffmann]

  Hi,

On 01/10/12 14:13, Hans de Goede wrote:
> And ignore status messages from the client which arrive after stream
> stop (the stream stop send to the client and an error status reported by
> the client my cross each other due to network latency).

Series looks fine.

Anthony, can you pull it in?

thanks,
  Gerd

The following changes since commit 520a02f8b844152929817b686113aed27229d3a9:

  cris-dis: Clean memory allocation (2012-01-10 09:36:43 +0100)

are available in the git repository at:
  git://git.kraxel.org/qemu usb.35

Hans de Goede (5):
      usb-redir: Clear iso / irq error when stopping the stream
      usb-redir: Dynamically adjust iso buffering size based on ep interval
      usb-redir: Pre-fill our isoc input buffer before sending pkts to
the host
      usb-redir: Try to keep our buffer size near the target size
      usb-redir: Improve some debugging messages

 usb-redir.c |  118
+++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 103 insertions(+), 15 deletions(-)

[Qemu-devel] [PATCH 1/5] usb-redir: Clear iso / irq error when stopping the stream

[Hans de Goede]

And ignore status messages from the client which arrive after stream
stop (the stream stop send to the client and an error status reported by
the client my cross each other due to network latency).

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 usb-redir.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/usb-redir.c b/usb-redir.c
index c232c7c..477c609 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -420,6 +420,7 @@ static void usbredir_stop_iso_stream(USBRedirDevice *dev, uint8_t ep)
         DPRINTF("iso stream stopped ep %02X\n", ep);
         dev->endpoint[EP2I(ep)].iso_started = 0;
     }
+    dev->endpoint[EP2I(ep)].iso_error = 0;
     usbredir_free_bufpq(dev, ep);
 }
 
@@ -532,6 +533,7 @@ static void usbredir_stop_interrupt_receiving(USBRedirDevice *dev,
         DPRINTF("interrupt recv stopped ep %02X\n", ep);
         dev->endpoint[EP2I(ep)].interrupt_started = 0;
     }
+    dev->endpoint[EP2I(ep)].interrupt_error = 0;
     usbredir_free_bufpq(dev, ep);
 }
 
@@ -1056,7 +1058,7 @@ static void usbredir_iso_stream_status(void *priv, uint32_t id,
     DPRINTF("iso status %d ep %02X id %u\n", iso_stream_status->status,
             ep, id);
 
-    if (!dev->dev.attached) {
+    if (!dev->dev.attached || !dev->endpoint[EP2I(ep)].iso_started) {
         return;
     }
 
@@ -1077,7 +1079,7 @@ static void usbredir_interrupt_receiving_status(void *priv, uint32_t id,
     DPRINTF("interrupt recv status %d ep %02X id %u\n",
             interrupt_receiving_status->status, ep, id);
 
-    if (!dev->dev.attached) {
+    if (!dev->dev.attached || !dev->endpoint[EP2I(ep)].interrupt_started) {
         return;
     }
 
-- 
1.7.7.4

[Qemu-devel] [PATCH 2/5] usb-redir: Dynamically adjust iso buffering size based on ep interval

[Hans de Goede]

Note the bufpq_target_size id stored in the endpoint info struct,
even though it only used once. This is done because it will be
referenced from other code in a follow up patch.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 usb-redir.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/usb-redir.c b/usb-redir.c
index 477c609..0a1157b 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -61,6 +61,7 @@ struct endp_data {
     uint8_t interrupt_started;
     uint8_t interrupt_error;
     QTAILQ_HEAD(, buf_packet) bufpq;
+    int bufpq_target_size;
 };
 
 struct USBRedirDevice {
@@ -342,15 +343,41 @@ static int usbredir_handle_iso_data(USBRedirDevice *dev, USBPacket *p,
                                      uint8_t ep)
 {
     int status, len;
-
     if (!dev->endpoint[EP2I(ep)].iso_started &&
             !dev->endpoint[EP2I(ep)].iso_error) {
         struct usb_redir_start_iso_stream_header start_iso = {
             .endpoint = ep,
-            /* TODO maybe do something with these depending on ep interval? */
-            .pkts_per_urb = 32,
-            .no_urbs = 3,
         };
+        int pkts_per_sec;
+
+        if (dev->dev.speed == USB_SPEED_HIGH)
+            pkts_per_sec = 8000 / dev->endpoint[EP2I(ep)].interval;
+        else
+            pkts_per_sec = 1000 / dev->endpoint[EP2I(ep)].interval;
+        /* Testing has shown that we need circa 60 ms buffer */
+        dev->endpoint[EP2I(ep)].bufpq_target_size = (pkts_per_sec * 60) / 1000;
+
+        /* Aim for approx 100 interrupts / second on the client to
+           balance latency and interrupt load */
+        start_iso.pkts_per_urb = pkts_per_sec / 100;
+        if (start_iso.pkts_per_urb < 1) {
+            start_iso.pkts_per_urb = 1;
+        } else if (start_iso.pkts_per_urb > 32) {
+            start_iso.pkts_per_urb = 32;
+        }
+
+        start_iso.no_urbs = (dev->endpoint[EP2I(ep)].bufpq_target_size +
+                             start_iso.pkts_per_urb - 1) /
+                            start_iso.pkts_per_urb;
+        /* Output endpoints pre-fill only 1/2 of the packets, keeping the rest
+           as overflow buffer. Also see the usbredir protocol documentation */
+        if (!(ep & USB_DIR_IN)) {
+            start_iso.no_urbs *= 2;
+        }
+        if (start_iso.no_urbs > 16) {
+            start_iso.no_urbs = 16;
+        }
+
         /* No id, we look at the ep when receiving a status back */
         usbredirparser_send_start_iso_stream(dev->parser, 0, &start_iso);
         usbredirparser_do_write(dev->parser);
@@ -988,9 +1015,24 @@ static void usbredir_ep_info(void *priv,
         dev->endpoint[i].type = ep_info->type[i];
         dev->endpoint[i].interval = ep_info->interval[i];
         dev->endpoint[i].interface = ep_info->interface[i];
-        if (dev->endpoint[i].type != usb_redir_type_invalid) {
+        switch (dev->endpoint[i].type) {
+        case usb_redir_type_invalid:
+            break;
+        case usb_redir_type_iso:
+        case usb_redir_type_interrupt:
+            if (dev->endpoint[i].interval == 0) {
+                ERROR("Received 0 interval for isoc or irq endpoint\n");
+                usbredir_device_disconnect(dev);
+            }
+            /* Fall through */
+        case usb_redir_type_control:
+        case usb_redir_type_bulk:
             DPRINTF("ep: %02X type: %d interface: %d\n", I2EP(i),
                     dev->endpoint[i].type, dev->endpoint[i].interface);
+            break;
+        default:
+            ERROR("Received invalid endpoint type\n");
+            usbredir_device_disconnect(dev);
         }
     }
 }
-- 
1.7.7.4

Re: [Qemu-devel] [PATCH 2/5] usb-redir: Dynamically adjust iso buffering size based on ep interval

[Gerd Hoffmann]

On 12/22/11 12:12, Hans de Goede wrote:
> Note the bufpq_target_size id stored in the endpoint info struct,
> even though it only used once. This is done because it will be
> referenced from other code in a follow up patch.

=== checkpatch complains ===
WARNING: braces {} are necessary for all arms of this statement
#29: FILE: usb-redir.c:343:
+        if (dev->dev.speed == USB_SPEED_HIGH)
[...]
+        else
[...]

total: 0 errors, 1 warnings, 77 lines checked

cheers,
  Gerd

Hint:
http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html