[Qemu-devel] [PATCH 1/2] Usb: create the receive queue for the virtual USB NIC

[Qemu-devel] [PATCH 1/2] Usb: create the receive queue for the virtual USB NIC

[rongqing.li]

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(-)

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,
-- 
1.7.4.1

[Qemu-devel] [PATCH 2/2] hub: change hub can_receive() strategy

[rongqing.li]

From: Roy.Li <rongqing.li@windriver.com>

Only one hub port's peer can_receive() returns 1, the source
hub port .can_receive should return 1, to fix the below bug:

    The up state NIC can not receive any packets if guest has
    more than two NICs and only one NIC is in down state.
    http://lists.nongnu.org/archive/html/qemu-discuss/2012-08/msg00036.html

This bug is introduced by 52a3cb8(add the support for hub
own flow control) and 60c07d93 (fix qemu_can_send_packet logic),
they are tried to fix that usb NIC lost packets by blocking hub
receive until all port attached this hub can receive since usb
NIC only can accept one packet at one time, their logic is wrong,
we should fix it by creating a queue for usb NIC.

Signed-off-by: Roy.Li <rongqing.li@windriver.com>
---
 net/hub.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/hub.c b/net/hub.c
index ac157e3..650a8b4 100644
--- a/net/hub.c
+++ b/net/hub.c
@@ -97,12 +97,12 @@ static int net_hub_port_can_receive(NetClientState *nc)
             continue;
         }
 
-        if (!qemu_can_send_packet(&port->nc)) {
-            return 0;
+        if (qemu_can_send_packet(&port->nc)) {
+            return 1;
         }
     }
 
-    return 1;
+    return 0;
 }
 
 static ssize_t net_hub_port_receive(NetClientState *nc,
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 2/2] hub: change hub can_receive() strategy

[Paolo Bonzini]

Il 24/08/2012 09:56, rongqing.li@windriver.com ha scritto:
> Only one hub port's peer can_receive() returns 1, the source
> hub port .can_receive should return 1, to fix the below bug:
> 
>     The up state NIC can not receive any packets if guest has
>     more than two NICs and only one NIC is in down state.
>     http://lists.nongnu.org/archive/html/qemu-discuss/2012-08/msg00036.html
> 
> This bug is introduced by 52a3cb8(add the support for hub
> own flow control) and 60c07d93 (fix qemu_can_send_packet logic),
> they are tried to fix that usb NIC lost packets by blocking hub
> receive until all port attached this hub can receive since usb
> NIC only can accept one packet at one time, their logic is wrong,
> we should fix it by creating a queue for usb NIC.

A link-down NIC should always return 1 from can_receive (and will drop
the packet).  Is that the real bug here?

Paolo

Re: [Qemu-devel] [PATCH 2/2] hub: change hub can_receive() strategy

[Rongqing Li]

On 2012年08月24日 16:20, Paolo Bonzini wrote:
> A link-down NIC should always return 1 from can_receive (and will drop
> the packet).  Is that the real bug here?
>

A link-down NIC always return 0 from can_receive.

Yes, it is a bug.

-Roy


> Paolo
>
>

-- 
Best Reagrds,
Roy | RongQing Li

Re: [Qemu-devel] [PATCH 2/2] hub: change hub can_receive() strategy

[Stefan Hajnoczi]

On Fri, Aug 24, 2012 at 9:33 AM, Rongqing Li <rongqing.li@windriver.com> wrote:
>
>
> On 2012年08月24日 16:20, Paolo Bonzini wrote:
>>
>> A link-down NIC should always return 1 from can_receive (and will drop
>> the packet).  Is that the real bug here?
>>
>
> A link-down NIC always return 0 from can_receive.
>
> Yes, it is a bug.

The code says something different.  link_down isn't taken into account
by netc.:qemu_can_send_packet():

int qemu_can_send_packet(NetClientState *sender)
{
    if (!sender->peer) {
        return 1;
    }

    if (sender->peer->receive_disabled) {
        return 0;
    } else if (sender->peer->info->can_receive &&
               !sender->peer->info->can_receive(sender->peer)) {
        return 0;
    }
    return 1;
}

If the net client has no ->can_receive then we return 1.  Otherwise we
return ->can_receive().

Were you thinking of a specific NIC where .can_receive() returns 0
when link_down=1?

Stefan

Re: [Qemu-devel] [PATCH 1/2] Usb: create the receive queue for the virtual USB NIC

[Stefan Hajnoczi]

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