[Qemu-devel] [PATCH 0/8] TUNSETSNDBUF - pushback to help UDP tx

[Qemu-devel] [PATCH 0/8] TUNSETSNDBUF - pushback to help UDP tx

[Mark McLoughlin]

Hi,
        The first two patches are a fix to fix a potential crash with
host_net_remove while doing heavy network I/O.

        The rest of the patches add support for using the new
TUNSETSNDBUF ioctl in 2.6.30 in order to provide some pushback against
UDP applications in the guest who are sending packets faster than the
host can handle them. More detail in the 06/08 patch.

Cheers,
Mark.

[Qemu-devel] [PATCH 1/8] net: add qemu_purge_queued_packets()

[Mark McLoughlin]

If net client sends packets asynchronously, it needs to purge its queued
packets in cleanup() so as to prevent sent callbacks being invoked with
a freed client.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net.c |   16 ++++++++++++++++
 net.h |    1 +
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/net.c b/net.c
index 9f9e363..0753a7c 100644
--- a/net.c
+++ b/net.c
@@ -439,6 +439,22 @@ qemu_deliver_packet(VLANClientState *sender, const uint8_t *buf, int size)
     return ret;
 }
 
+void qemu_purge_queued_packets(VLANClientState *vc)
+{
+    VLANPacket **pp = &vc->vlan->send_queue;
+
+    while (*pp != NULL) {
+        VLANPacket *packet = *pp;
+
+        if (packet->sender == vc) {
+            *pp = packet->next;
+            qemu_free(packet);
+        } else {
+            pp = &packet->next;
+        }
+    }
+}
+
 void qemu_flush_queued_packets(VLANClientState *vc)
 {
     VLANPacket *packet;
diff --git a/net.h b/net.h
index a9abf63..ef85087 100644
--- a/net.h
+++ b/net.h
@@ -70,6 +70,7 @@ ssize_t qemu_sendv_packet_async(VLANClientState *vc, const struct iovec *iov,
 void qemu_send_packet(VLANClientState *vc, const uint8_t *buf, int size);
 ssize_t qemu_send_packet_async(VLANClientState *vc, const uint8_t *buf,
                                int size, NetPacketSent *sent_cb);
+void qemu_purge_queued_packets(VLANClientState *vc);
 void qemu_flush_queued_packets(VLANClientState *vc);
 void qemu_format_nic_info_str(VLANClientState *vc, uint8_t macaddr[6]);
 void qemu_check_nic_model(NICInfo *nd, const char *model);
-- 
1.6.0.6

[Qemu-devel] [PATCH 2/8] net: purge queued packets in tap_cleanup()

[Mark McLoughlin]

If tap has any packets queued at host_net_remove time, it needs to purge
them in order to prevent a sent callback being invoked for it.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net.c b/net.c
index 0753a7c..91c08bd 100644
--- a/net.c
+++ b/net.c
@@ -1128,6 +1128,8 @@ static void tap_cleanup(VLANClientState *vc)
 {
     TAPState *s = vc->opaque;
 
+    qemu_purge_queued_packets(vc);
+
     if (s->down_script[0])
         launch_script(s->down_script, s->down_script_arg, s->fd);
 
-- 
1.6.0.6

[Qemu-devel] [PATCH 3/8] net: add tap_read_poll() helper

[Mark McLoughlin]

Add a helper to enable/disable the read polling on tapfd.

We need this, because we want to start write polling on the tapfd too
and enable/disable both types of polling independently.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net.c |   30 +++++++++++++++++++++++-------
 1 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/net.c b/net.c
index 91c08bd..9005822 100644
--- a/net.c
+++ b/net.c
@@ -1043,10 +1043,29 @@ typedef struct TAPState {
     char down_script[1024];
     char down_script_arg[128];
     uint8_t buf[4096];
+    unsigned int read_poll : 1;
 } TAPState;
 
 static int launch_script(const char *setup_script, const char *ifname, int fd);
 
+static int tap_can_send(void *opaque);
+static void tap_send(void *opaque);
+
+static void tap_update_fd_handler(TAPState *s)
+{
+    qemu_set_fd_handler2(s->fd,
+                         s->read_poll  ? tap_can_send : NULL,
+                         s->read_poll  ? tap_send     : NULL,
+                         NULL,
+                         s);
+}
+
+static void tap_read_poll(TAPState *s, int enable)
+{
+    s->read_poll = !!enable;
+    tap_update_fd_handler(s);
+}
+
 static ssize_t tap_receive_iov(VLANClientState *vc, const struct iovec *iov,
                                int iovcnt)
 {
@@ -1097,13 +1116,10 @@ static ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen)
 }
 #endif
 
-static void tap_send(void *opaque);
-
 static void tap_send_completed(VLANClientState *vc)
 {
     TAPState *s = vc->opaque;
-
-    qemu_set_fd_handler2(s->fd, tap_can_send, tap_send, NULL, s);
+    tap_read_poll(s, 1);
 }
 
 static void tap_send(void *opaque)
@@ -1119,7 +1135,7 @@ static void tap_send(void *opaque)
 
         size = qemu_send_packet_async(s->vc, s->buf, size, tap_send_completed);
         if (size == 0) {
-            qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+            tap_read_poll(s, 0);
         }
     } while (size > 0);
 }
@@ -1133,7 +1149,7 @@ static void tap_cleanup(VLANClientState *vc)
     if (s->down_script[0])
         launch_script(s->down_script, s->down_script_arg, s->fd);
 
-    qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
+    tap_read_poll(s, 0);
     close(s->fd);
     qemu_free(s);
 }
@@ -1151,7 +1167,7 @@ static TAPState *net_tap_fd_init(VLANState *vlan,
     s->fd = fd;
     s->vc = qemu_new_vlan_client(vlan, model, name, NULL, tap_receive,
                                  tap_receive_iov, tap_cleanup, s);
-    qemu_set_fd_handler2(s->fd, tap_can_send, tap_send, NULL, s);
+    tap_read_poll(s, 1);
     snprintf(s->vc->info_str, sizeof(s->vc->info_str), "fd=%d", fd);
     return s;
 }
-- 
1.6.0.6

[Qemu-devel] [PATCH 4/8] net: handle EAGAIN from tapfd write()

[Mark McLoughlin]

If a write() on tapfd returns EAGAIN, return zero so that the packet
gets queued (in the case of async send) and enable polling tapfd for
writing.

When tapfd becomes writable, disable write polling and flush any queued
packets.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net.c |   27 +++++++++++++++++++++++++--
 1 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/net.c b/net.c
index 9005822..e2e73ef 100644
--- a/net.c
+++ b/net.c
@@ -1044,19 +1044,21 @@ typedef struct TAPState {
     char down_script_arg[128];
     uint8_t buf[4096];
     unsigned int read_poll : 1;
+    unsigned int write_poll : 1;
 } TAPState;
 
 static int launch_script(const char *setup_script, const char *ifname, int fd);
 
 static int tap_can_send(void *opaque);
 static void tap_send(void *opaque);
+static void tap_writable(void *opaque);
 
 static void tap_update_fd_handler(TAPState *s)
 {
     qemu_set_fd_handler2(s->fd,
                          s->read_poll  ? tap_can_send : NULL,
                          s->read_poll  ? tap_send     : NULL,
-                         NULL,
+                         s->write_poll ? tap_writable : NULL,
                          s);
 }
 
@@ -1066,6 +1068,21 @@ static void tap_read_poll(TAPState *s, int enable)
     tap_update_fd_handler(s);
 }
 
+static void tap_write_poll(TAPState *s, int enable)
+{
+    s->write_poll = !!enable;
+    tap_update_fd_handler(s);
+}
+
+static void tap_writable(void *opaque)
+{
+    TAPState *s = opaque;
+
+    tap_write_poll(s, 0);
+
+    qemu_flush_queued_packets(s->vc);
+}
+
 static ssize_t tap_receive_iov(VLANClientState *vc, const struct iovec *iov,
                                int iovcnt)
 {
@@ -1074,7 +1091,12 @@ static ssize_t tap_receive_iov(VLANClientState *vc, const struct iovec *iov,
 
     do {
         len = writev(s->fd, iov, iovcnt);
-    } while (len == -1 && (errno == EINTR || errno == EAGAIN));
+    } while (len == -1 && errno == EINTR);
+
+    if (len == -1 && errno == EAGAIN) {
+        tap_write_poll(s, 1);
+        return 0;
+    }
 
     return len;
 }
@@ -1150,6 +1172,7 @@ static void tap_cleanup(VLANClientState *vc)
         launch_script(s->down_script, s->down_script_arg, s->fd);
 
     tap_read_poll(s, 0);
+    tap_write_poll(s, 0);
     close(s->fd);
     qemu_free(s);
 }
-- 
1.6.0.6

[Qemu-devel] [PATCH 5/8] net: return TAPState from net_tap_init()

[Mark McLoughlin]

net_tap_fd_init() already returns TAPState, so this is a sensible
cleanup in its own right.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net.c |   27 ++++++++++++++++-----------
 1 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/net.c b/net.c
index e2e73ef..58f3d51 100644
--- a/net.c
+++ b/net.c
@@ -1429,9 +1429,9 @@ static int launch_script(const char *setup_script, const char *ifname, int fd)
     return -1;
 }
 
-static int net_tap_init(VLANState *vlan, const char *model,
-                        const char *name, const char *ifname1,
-                        const char *setup_script, const char *down_script)
+static TAPState *net_tap_init(VLANState *vlan, const char *model,
+                              const char *name, const char *ifname1,
+                              const char *setup_script, const char *down_script)
 {
     TAPState *s;
     int fd;
@@ -1443,13 +1443,13 @@ static int net_tap_init(VLANState *vlan, const char *model,
         ifname[0] = '\0';
     TFR(fd = tap_open(ifname, sizeof(ifname)));
     if (fd < 0)
-        return -1;
+        return NULL;
 
     if (!setup_script || !strcmp(setup_script, "no"))
         setup_script = "";
-    if (setup_script[0] != '\0') {
-	if (launch_script(setup_script, ifname, fd))
-	    return -1;
+    if (setup_script[0] != '\0' &&
+        launch_script(setup_script, ifname, fd)) {
+        return NULL;
     }
     s = net_tap_fd_init(vlan, model, name, fd);
     snprintf(s->vc->info_str, sizeof(s->vc->info_str),
@@ -1459,7 +1459,7 @@ static int net_tap_init(VLANState *vlan, const char *model,
         snprintf(s->down_script, sizeof(s->down_script), "%s", down_script);
         snprintf(s->down_script_arg, sizeof(s->down_script_arg), "%s", ifname);
     }
-    return 0;
+    return s;
 }
 
 #endif /* !_WIN32 */
@@ -2291,6 +2291,7 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
     if (!strcmp(device, "tap")) {
         char ifname[64], chkbuf[64];
         char setup_script[1024], down_script[1024];
+        TAPState *s;
         int fd;
         vlan->nb_host_devs++;
         if (get_param_value(buf, sizeof(buf), "fd", p) > 0) {
@@ -2301,8 +2302,7 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
             }
             fd = strtol(buf, NULL, 0);
             fcntl(fd, F_SETFL, O_NONBLOCK);
-            net_tap_fd_init(vlan, device, name, fd);
-            ret = 0;
+            s = net_tap_fd_init(vlan, device, name, fd);
         } else {
             static const char * const tap_params[] = {
                 "vlan", "name", "ifname", "script", "downscript", NULL
@@ -2321,7 +2321,12 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
             if (get_param_value(down_script, sizeof(down_script), "downscript", p) == 0) {
                 pstrcpy(down_script, sizeof(down_script), DEFAULT_NETWORK_DOWN_SCRIPT);
             }
-            ret = net_tap_init(vlan, device, name, ifname, setup_script, down_script);
+            s = net_tap_init(vlan, device, name, ifname, setup_script, down_script);
+        }
+        if (s != NULL) {
+            ret = 0;
+        } else {
+            ret = -1;
         }
     } else
 #endif
-- 
1.6.0.6

[Qemu-devel] [PATCH 6/8] net: add '-net tap,sndbuf=nbytes'

[Mark McLoughlin]

2.6.30 adds a new TUNSETSNDBUF ioctl() which allows a send buffer limit
for the tap device to be specified. When this limit is reached, a tap
write() will return EAGAIN and poll() will indicate the fd isn't
writable.

This allows people to tune their setups so as to avoid e.g. UDP packet
loss when the sending application in the guest out-runs the NIC in the
host.

There is no obviously sensible default setting - a suitable value
depends mostly on the capabilities of the physical NIC through which the
packets are being sent.

Also, note that when using a bridge with netfilter enabled, we currently
never get EAGAIN because netfilter causes the packet to be immediately
orphaned. Set /proc/sys/net/bridge/bridge nf-call-iptables to zero to
disable this behaviour.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net.c           |   26 ++++++++++++++++++++++----
 qemu-options.hx |    9 ++++++++-
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/net.c b/net.c
index 58f3d51..1bf7c6b 100644
--- a/net.c
+++ b/net.c
@@ -1162,6 +1162,18 @@ static void tap_send(void *opaque)
     } while (size > 0);
 }
 
+static void tap_set_sndbuf(TAPState *s, int sndbuf, Monitor *mon)
+{
+#ifdef TUNSETSNDBUF
+    if (ioctl(s->fd, TUNSETSNDBUF, &sndbuf) == -1) {
+        config_error(mon, "TUNSETSNDBUF ioctl failed: %s\n",
+                     strerror(errno));
+    }
+#else
+    config_error(mon, "No '-net tap,sndbuf=<nbytes>' support available\n");
+#endif
+}
+
 static void tap_cleanup(VLANClientState *vc)
 {
     TAPState *s = vc->opaque;
@@ -2141,9 +2153,6 @@ void qemu_check_nic_model_list(NICInfo *nd, const char * const *models,
 
 int net_client_init(Monitor *mon, const char *device, const char *p)
 {
-    static const char * const fd_params[] = {
-        "vlan", "name", "fd", NULL
-    };
     char buf[1024];
     int vlan_id, ret;
     VLANState *vlan;
@@ -2295,6 +2304,9 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
         int fd;
         vlan->nb_host_devs++;
         if (get_param_value(buf, sizeof(buf), "fd", p) > 0) {
+            static const char * const fd_params[] = {
+                "vlan", "name", "fd", "sndbuf", NULL
+            };
             if (check_params(chkbuf, sizeof(chkbuf), fd_params, p) < 0) {
                 config_error(mon, "invalid parameter '%s' in '%s'\n", chkbuf, p);
                 ret = -1;
@@ -2305,7 +2317,7 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
             s = net_tap_fd_init(vlan, device, name, fd);
         } else {
             static const char * const tap_params[] = {
-                "vlan", "name", "ifname", "script", "downscript", NULL
+                "vlan", "name", "ifname", "script", "downscript", "sndbuf", NULL
             };
             if (check_params(chkbuf, sizeof(chkbuf), tap_params, p) < 0) {
                 config_error(mon, "invalid parameter '%s' in '%s'\n", chkbuf, p);
@@ -2324,6 +2336,9 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
             s = net_tap_init(vlan, device, name, ifname, setup_script, down_script);
         }
         if (s != NULL) {
+            if (get_param_value(buf, sizeof(buf), "sndbuf", p)) {
+                tap_set_sndbuf(s, atoi(buf), mon);
+            }
             ret = 0;
         } else {
             ret = -1;
@@ -2333,6 +2348,9 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
     if (!strcmp(device, "socket")) {
         char chkbuf[64];
         if (get_param_value(buf, sizeof(buf), "fd", p) > 0) {
+            static const char * const fd_params[] = {
+                "vlan", "name", "fd", NULL
+            };
             int fd;
             if (check_params(chkbuf, sizeof(chkbuf), fd_params, p) < 0) {
                 config_error(mon, "invalid parameter '%s' in '%s'\n", chkbuf, p);
diff --git a/qemu-options.hx b/qemu-options.hx
index 9d5e05a..06e2505 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -744,12 +744,19 @@ DEF("net", HAS_ARG, QEMU_OPTION_net,
     "-net tap[,vlan=n][,name=str],ifname=name\n"
     "                connect the host TAP network interface to VLAN 'n'\n"
 #else
-    "-net tap[,vlan=n][,name=str][,fd=h][,ifname=name][,script=file][,downscript=dfile]\n"
+    "-net tap[,vlan=n][,name=str][,fd=h][,ifname=name][,script=file][,downscript=dfile]"
+#ifdef TUNSETSNDBUF
+    "[,sndbuf=nbytes]"
+#endif
+    "\n"
     "                connect the host TAP network interface to VLAN 'n' and use the\n"
     "                network scripts 'file' (default=%s)\n"
     "                and 'dfile' (default=%s);\n"
     "                use '[down]script=no' to disable script execution;\n"
     "                use 'fd=h' to connect to an already opened TAP interface\n"
+#ifdef TUNSETSNDBUF
+    "                use 'sndbuf=nbytes' to limit the size of the send buffer\n"
+#endif
 #endif
     "-net socket[,vlan=n][,name=str][,fd=h][,listen=[host]:port][,connect=host:port]\n"
     "                connect the vlan 'n' to another VLAN using a socket connection\n"
-- 
1.6.0.6

[Qemu-devel] [PATCH 7/8] net: add packet length to NetPacketSent callback

[Mark McLoughlin]

virtio-net needs this - for the same purpose that it currently uses the
return value from qemu_sendv_packet().

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net.c |    4 ++--
 net.h |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net.c b/net.c
index 1bf7c6b..1f816ca 100644
--- a/net.c
+++ b/net.c
@@ -472,7 +472,7 @@ void qemu_flush_queued_packets(VLANClientState *vc)
         }
 
         if (packet->sent_cb)
-            packet->sent_cb(packet->sender);
+            packet->sent_cb(packet->sender, ret);
 
         qemu_free(packet);
     }
@@ -1138,7 +1138,7 @@ static ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen)
 }
 #endif
 
-static void tap_send_completed(VLANClientState *vc)
+static void tap_send_completed(VLANClientState *vc, ssize_t len)
 {
     TAPState *s = vc->opaque;
     tap_read_poll(s, 1);
diff --git a/net.h b/net.h
index ef85087..567f39c 100644
--- a/net.h
+++ b/net.h
@@ -32,7 +32,7 @@ struct VLANClientState {
 
 typedef struct VLANPacket VLANPacket;
 
-typedef void (NetPacketSent) (VLANClientState *);
+typedef void (NetPacketSent) (VLANClientState *, ssize_t);
 
 struct VLANPacket {
     struct VLANPacket *next;
-- 
1.6.0.6

[Qemu-devel] [PATCH 8/8] virtio-net: implement async packet sending

[Mark McLoughlin]

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 hw/virtio-net.c |   39 +++++++++++++++++++++++++++++++++++++--
 1 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index e1efbbe..4fbeaeb 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -32,6 +32,10 @@ typedef struct VirtIONet
     VLANClientState *vc;
     QEMUTimer *tx_timer;
     int tx_timer_active;
+    struct {
+        VirtQueueElement elem;
+        ssize_t len;
+    } async_tx;
     int mergeable_rx_bufs;
     uint8_t promisc;
     uint8_t allmulti;
@@ -483,6 +487,21 @@ static ssize_t virtio_net_receive(VLANClientState *vc, const uint8_t *buf, size_
     return size;
 }
 
+static void virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq);
+
+static void virtio_net_tx_complete(VLANClientState *vc, ssize_t len)
+{
+    VirtIONet *n = vc->opaque;
+
+    virtqueue_push(n->tx_vq, &n->async_tx.elem, n->async_tx.len);
+    virtio_notify(&n->vdev, n->tx_vq);
+
+    n->async_tx.elem.out_num = n->async_tx.len = 0;
+
+    virtio_queue_set_notification(n->tx_vq, 1);
+    virtio_net_flush_tx(n, n->tx_vq);
+}
+
 /* TX */
 static void virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
 {
@@ -492,8 +511,13 @@ static void virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
     if (!(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK))
         return;
 
+    if (n->async_tx.elem.out_num) {
+        virtio_queue_set_notification(n->tx_vq, 0);
+        return;
+    }
+
     while (virtqueue_pop(vq, &elem)) {
-        ssize_t len = 0;
+        ssize_t ret, len = 0;
         unsigned int out_num = elem.out_num;
         struct iovec *out_sg = &elem.out_sg[0];
         unsigned hdr_len;
@@ -520,7 +544,16 @@ static void virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
             len += hdr_len;
         }
 
-        len += qemu_sendv_packet(n->vc, out_sg, out_num);
+        ret = qemu_sendv_packet_async(n->vc, out_sg, out_num,
+                                      virtio_net_tx_complete);
+        if (ret == 0) {
+            virtio_queue_set_notification(n->tx_vq, 0);
+            n->async_tx.elem = elem;
+            n->async_tx.len  = len;
+            return;
+        }
+
+        len += ret;
 
         virtqueue_push(vq, &elem, len);
         virtio_notify(&n->vdev, vq);
@@ -663,6 +696,8 @@ static void virtio_net_cleanup(VLANClientState *vc)
 {
     VirtIONet *n = vc->opaque;
 
+    qemu_purge_queued_packets(vc);
+
     unregister_savevm("virtio-net", n);
 
     qemu_free(n->mac_table.macs);
-- 
1.6.0.6