diff mbox

[1/2] Usb: create the receive queue for the virtual USB NIC

Message ID 1345794984-10337-1-git-send-email-rongqing.li@windriver.com
State New
Headers show

Commit Message

li qongqing Aug. 24, 2012, 7:56 a.m. UTC 
From: Roy.Li <rongqing.li@windriver.com>

The virtual USB NIC originally used a fixed buffer to receive packets which
only store 1 packet at a time, which is easy to overrun with packets if the
guest does not consume it quickly, and always lost packets at the below case:

	The emulated machine is using an USB-ethernet adapter,
	it is connected to the network using SLIRP and I'm
	dumping the traffic in a .pcap file.  As per the following
	command line :
	-net nic,model=usb,vlan=1 -net user,vlan=1 -net dump,vlan=1,file=/tmp/pkt.pcap
	Every time that two packets are coming in a row from
	the host, the usb-net code will receive the first one,
	then returns 0 to can_receive call since it has a
	1 packet long queue. But as the dump code is always
	ready to receive, qemu_can_send_packet will return
	true and the next packet will discard the previous
	one in the usb-net code.

Commit 60c07d933c(net: fix qemu_can_send_packet logic) is trying to fix,
but the logic is wrong and introduce other bug. Now replace the fixed buffer
with a receive queue which can accept a variable number of packets to fix
this bug.

Signed-off-by: Roy.Li <rongqing.li@windriver.com>
---
 hw/usb/dev-network.c |   87 ++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 63 insertions(+), 24 deletions(-)

Comments

Stefan Hajnoczi Aug. 24, 2012, 12:25 p.m. UTC | #1 
On Fri, Aug 24, 2012 at 8:56 AM,  <rongqing.li@windriver.com> wrote:
> From: Roy.Li <rongqing.li@windriver.com>
>
> The virtual USB NIC originally used a fixed buffer to receive packets which
> only store 1 packet at a time, which is easy to overrun with packets if the
> guest does not consume it quickly, and always lost packets at the below case:
>
>         The emulated machine is using an USB-ethernet adapter,
>         it is connected to the network using SLIRP and I'm
>         dumping the traffic in a .pcap file.  As per the following
>         command line :
>         -net nic,model=usb,vlan=1 -net user,vlan=1 -net dump,vlan=1,file=/tmp/pkt.pcap
>         Every time that two packets are coming in a row from
>         the host, the usb-net code will receive the first one,
>         then returns 0 to can_receive call since it has a
>         1 packet long queue. But as the dump code is always
>         ready to receive, qemu_can_send_packet will return
>         true and the next packet will discard the previous
>         one in the usb-net code.
>
> Commit 60c07d933c(net: fix qemu_can_send_packet logic) is trying to fix,
> but the logic is wrong and introduce other bug. Now replace the fixed buffer
> with a receive queue which can accept a variable number of packets to fix
> this bug.
>
> Signed-off-by: Roy.Li <rongqing.li@windriver.com>
> ---
>  hw/usb/dev-network.c |   87 ++++++++++++++++++++++++++++++++++++--------------
>  1 files changed, 63 insertions(+), 24 deletions(-)

This doesn't really solve the problem, it just makes it less likely
we'll hit the bug.  What happens if 46 packets are ready?
MAX_RCV_QUEUE is 45 so we'll drop packets again.

The net subsystem already has queues to hold packets when a peer
cannot handle them.  We need to use the existing net/queue.c mechanism
instead of introducing more queues.

I will send out patches later today to fix this.

Stefan
diff mbox

Patch

diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index c84892c..0c867b7 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -87,6 +87,7 @@  enum usbstring_idx {
 #define STATUS_BYTECOUNT		16   /* 8 byte header + data */
 
 #define ETH_FRAME_LEN			1514 /* Max. octets in frame sans FCS */
+#define MAX_RCV_QUEUE			45 /* Max length of receiving queue */
 
 static const USBDescStrings usb_net_stringtable = {
     [STRING_MANUFACTURER]       = "QEMU",
@@ -622,6 +623,12 @@  struct rndis_response {
     uint8_t buf[0];
 };
 
+struct USBNet_buffer {
+    QTAILQ_ENTRY(USBNet_buffer) entries;
+    unsigned int in_ptr, in_len;
+    uint8_t in_buf[0];
+};
+
 typedef struct USBNetState {
     USBDevice dev;
 
@@ -636,8 +643,8 @@  typedef struct USBNetState {
     uint8_t out_buf[2048];
 
     USBPacket *inpkt;
-    unsigned int in_ptr, in_len;
-    uint8_t in_buf[2048];
+    QTAILQ_HEAD(USBNet_buffer_head, USBNet_buffer) rndis_netbuf;
+    uint32_t buf_len;
 
     char usbstring_mac[13];
     NICState *nic;
@@ -867,6 +874,17 @@  static void rndis_clear_responsequeue(USBNetState *s)
     }
 }
 
+static void rndis_clear_netbuffer(USBNetState *s)
+{
+    struct USBNet_buffer *r;
+
+    while ((r = s->rndis_netbuf.tqh_first)) {
+        QTAILQ_REMOVE(&s->rndis_netbuf, r, entries);
+        g_free(r);
+    }
+    s->buf_len = 0;
+}
+
 static int rndis_init_response(USBNetState *s, rndis_init_msg_type *buf)
 {
     rndis_init_cmplt_type *resp =
@@ -1025,7 +1043,8 @@  static int rndis_parse(USBNetState *s, uint8_t *data, int length)
 
     case RNDIS_RESET_MSG:
         rndis_clear_responsequeue(s);
-        s->out_ptr = s->in_ptr = s->in_len = 0;
+        rndis_clear_netbuffer(s);
+        s->out_ptr = 0;
         return rndis_reset_response(s, (rndis_reset_msg_type *) data);
 
     case RNDIS_KEEPALIVE_MSG:
@@ -1134,25 +1153,39 @@  static int usb_net_handle_datain(USBNetState *s, USBPacket *p)
 {
     int ret = USB_RET_NAK;
 
-    if (s->in_ptr > s->in_len) {
-        s->in_ptr = s->in_len = 0;
-        ret = USB_RET_NAK;
+    struct USBNet_buffer *r = s->rndis_netbuf.tqh_first;
+
+    if (!r) {
         return ret;
     }
-    if (!s->in_len) {
-        ret = USB_RET_NAK;
+
+    if (r->in_ptr > r->in_len) {
+        s->buf_len--;
+        QTAILQ_REMOVE(&s->rndis_netbuf, r, entries);
+        g_free(r);
+        return ret;
+    }
+
+    if (!r->in_len) {
+        s->buf_len--;
+        QTAILQ_REMOVE(&s->rndis_netbuf, r, entries);
+        g_free(r);
         return ret;
     }
-    ret = s->in_len - s->in_ptr;
+
+    ret = r->in_len - r->in_ptr;
     if (ret > p->iov.size) {
         ret = p->iov.size;
     }
-    usb_packet_copy(p, &s->in_buf[s->in_ptr], ret);
-    s->in_ptr += ret;
-    if (s->in_ptr >= s->in_len &&
-                    (is_rndis(s) || (s->in_len & (64 - 1)) || !ret)) {
+
+    usb_packet_copy(p, &r->in_buf[r->in_ptr], ret);
+    r->in_ptr += ret;
+    if (r->in_ptr >= r->in_len &&
+                    (is_rndis(s) || (r->in_len & (64 - 1)) || !ret)) {
         /* no short packet necessary */
-        s->in_ptr = s->in_len = 0;
+        s->buf_len--;
+        QTAILQ_REMOVE(&s->rndis_netbuf, r, entries);
+        g_free(r);
     }
 
 #ifdef TRAFFIC_DEBUG
@@ -1251,15 +1284,16 @@  static ssize_t usbnet_receive(NetClientState *nc, const uint8_t *buf, size_t siz
 {
     USBNetState *s = DO_UPCAST(NICState, nc, nc)->opaque;
     struct rndis_packet_msg_type *msg;
+    struct USBNet_buffer *r;
 
     if (is_rndis(s)) {
-        msg = (struct rndis_packet_msg_type *) s->in_buf;
         if (s->rndis_state != RNDIS_DATA_INITIALIZED) {
             return -1;
         }
-        if (size + sizeof(struct rndis_packet_msg_type) > sizeof(s->in_buf))
-            return -1;
 
+        r = g_malloc0(sizeof(struct USBNet_buffer) + \
+                      sizeof(struct rndis_packet_msg_type) + size);
+        msg = (struct rndis_packet_msg_type *) r->in_buf;
         memset(msg, 0, sizeof(struct rndis_packet_msg_type));
         msg->MessageType = cpu_to_le32(RNDIS_PACKET_MSG);
         msg->MessageLength = cpu_to_le32(size + sizeof(struct rndis_packet_msg_type));
@@ -1274,14 +1308,16 @@  static ssize_t usbnet_receive(NetClientState *nc, const uint8_t *buf, size_t siz
          * msg->Reserved;
          */
         memcpy(msg + 1, buf, size);
-        s->in_len = size + sizeof(struct rndis_packet_msg_type);
+        r->in_len = size + sizeof(struct rndis_packet_msg_type);
     } else {
-        if (size > sizeof(s->in_buf))
-            return -1;
-        memcpy(s->in_buf, buf, size);
-        s->in_len = size;
+        r = g_malloc0(sizeof(struct USBNet_buffer) + size);
+        memcpy(r->in_buf, buf, size);
+        r->in_len = size;
     }
-    s->in_ptr = 0;
+    r->in_ptr = 0;
+    s->buf_len++;
+    QTAILQ_INSERT_TAIL(&s->rndis_netbuf, r, entries);
+
     return size;
 }
 
@@ -1293,7 +1329,7 @@  static int usbnet_can_receive(NetClientState *nc)
         return 1;
     }
 
-    return !s->in_len;
+    return s->buf_len < MAX_RCV_QUEUE;
 }
 
 static void usbnet_cleanup(NetClientState *nc)
@@ -1309,6 +1345,7 @@  static void usb_net_handle_destroy(USBDevice *dev)
 
     /* TODO: remove the nd_table[] entry */
     rndis_clear_responsequeue(s);
+    rndis_clear_netbuffer(s);
     qemu_del_net_client(&s->nic->nc);
 }
 
@@ -1329,12 +1366,14 @@  static int usb_net_initfn(USBDevice *dev)
 
     s->rndis_state = RNDIS_UNINITIALIZED;
     QTAILQ_INIT(&s->rndis_resp);
+    QTAILQ_INIT(&s->rndis_netbuf);
 
     s->medium = 0;	/* NDIS_MEDIUM_802_3 */
     s->speed = 1000000; /* 100MBps, in 100Bps units */
     s->media_state = 0;	/* NDIS_MEDIA_STATE_CONNECTED */;
     s->filter = 0;
     s->vendorid = 0x1234;
+    s->buf_len = 0;
 
     qemu_macaddr_default_if_unset(&s->conf.macaddr);
     s->nic = qemu_new_nic(&net_usbnet_info, &s->conf,