[Qemu-devel] [PATCH 0/5] Add netmap backend offloadings support

[Qemu-devel] [PATCH 0/5] Add netmap backend offloadings support

[Vincenzo Maffione]

The purpose of this patch series is to add offloadings support
(TSO/UFO/CSUM) to the netmap network backend, and make it possible
for the paravirtual network frontends (virtio-net and vmxnet3) to
use it.
In order to achieve this, these patches extend the existing
net.h interface to add abstract operations through which a network
frontend can manipulate backend offloading features, instead of
directly calling TAP-specific functions.

Guest-to-guest performance before this patches for virtio-net + netmap:

    TCP_STREAM                  5.0 Gbps
    TCP_RR                      12.7 Gbps
    UDP_STREAM (64-bytes)       790 Kpps

Guest-to-guest performance after this patches for virtio-net + netmap:

    TCP_STREAM                  21.4 Gbps
    TCP_RR                      12.7 Gbps
    UDP_STREAM (64-bytes)       790 Kpps

Experiment details:
    - Processor: Intel i7-3770K CPU @ 3.50GHz (8 cores)
    - Memory @ 1333 MHz
    - Host O.S.: Archlinux with Linux 3.11
    - Guest O.S.: Archlinux with Linux 3.11

    - QEMU command line:
        qemu-system-x86_64 archdisk.qcow -snapshot -enable-kvm -device virtio-net-pci,ioeventfd=on,mac=00:AA:BB:CC:DD:01,netdev=mynet -netdev netmap,ifname=vale0:01,id=mynet -smp 2 -vga std -m 3G


Vincenzo Maffione (5):
  net: extend NetClientInfo for offloading manipulations
  net: TAP uses NetClientInfo offloading callbacks
  net: virtio-net and vmxnet3 use offloading API
  net: add offloadings support to netmap backend
  net: virtio-net and vmxnet3 can use netmap offloadings

 hw/net/virtio-net.c | 16 +++++---------
 hw/net/vmxnet3.c    | 12 +++++-----
 include/net/net.h   | 19 ++++++++++++++++
 net/net.c           | 55 +++++++++++++++++++++++++++++++++++++++++++++
 net/netmap.c        | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 net/tap.c           |  6 +++++
 6 files changed, 154 insertions(+), 18 deletions(-)

-- 
1.8.5.1

[Qemu-devel] [PATCH 1/5] net: extend NetClientInfo for offloading manipulations

[Vincenzo Maffione]

A set of new callbacks has been added to the NetClientInfo struct in
order to abstract the operations done by virtio-net and vmxnet3
frontends to manipulate TAP offloadings.

The net.h API has been extended with functions that access those
abstract operations, providing frontends with a way to manipulate
backend offloadings.

Signed-off-by: Vincenzo Maffione <v.maffione@gmail.com>
---
 include/net/net.h | 19 +++++++++++++++++++
 net/net.c         | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/include/net/net.h b/include/net/net.h
index 11e1468..f5b5bae 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -50,6 +50,12 @@ typedef void (NetCleanup) (NetClientState *);
 typedef void (LinkStatusChanged)(NetClientState *);
 typedef void (NetClientDestructor)(NetClientState *);
 typedef RxFilterInfo *(QueryRxFilter)(NetClientState *);
+typedef bool (HasUfo)(NetClientState *);
+typedef int (HasVnetHdr)(NetClientState *);
+typedef int (HasVnetHdrLen)(NetClientState *, int);
+typedef void (UsingVnetHdr)(NetClientState *, bool);
+typedef void (SetOffload)(NetClientState *, int, int, int, int, int);
+typedef void (SetVnetHdrLen)(NetClientState *, int);
 
 typedef struct NetClientInfo {
     NetClientOptionsKind type;
@@ -62,6 +68,12 @@ typedef struct NetClientInfo {
     LinkStatusChanged *link_status_changed;
     QueryRxFilter *query_rx_filter;
     NetPoll *poll;
+    HasUfo *has_ufo;
+    HasVnetHdr *has_vnet_hdr;
+    HasVnetHdrLen *has_vnet_hdr_len;
+    UsingVnetHdr *using_vnet_hdr;
+    SetOffload *set_offload;
+    SetVnetHdrLen *set_vnet_hdr_len;
 } NetClientInfo;
 
 struct NetClientState {
@@ -120,6 +132,13 @@ ssize_t qemu_send_packet_async(NetClientState *nc, const uint8_t *buf,
 void qemu_purge_queued_packets(NetClientState *nc);
 void qemu_flush_queued_packets(NetClientState *nc);
 void qemu_format_nic_info_str(NetClientState *nc, uint8_t macaddr[6]);
+bool qemu_peer_has_ufo(NetClientState *nc);
+int qemu_peer_has_vnet_hdr(NetClientState *nc);
+int qemu_peer_has_vnet_hdr_len(NetClientState *nc, int len);
+void qemu_peer_using_vnet_hdr(NetClientState *nc, bool enable);
+void qemu_peer_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
+                           int ecn, int ufo);
+void qemu_peer_set_vnet_hdr_len(NetClientState *nc, int len);
 void qemu_macaddr_default_if_unset(MACAddr *macaddr);
 int qemu_show_nic_models(const char *arg, const char *const *models);
 void qemu_check_nic_model(NICInfo *nd, const char *model);
diff --git a/net/net.c b/net/net.c
index 9db88cc..96f05d9 100644
--- a/net/net.c
+++ b/net/net.c
@@ -381,6 +381,61 @@ void qemu_foreach_nic(qemu_nic_foreach func, void *opaque)
     }
 }
 
+bool qemu_peer_has_ufo(NetClientState *nc)
+{
+    if (!nc->peer || !nc->peer->info->has_ufo) {
+        return false;
+    }
+
+    return nc->peer->info->has_ufo(nc->peer);
+}
+
+int qemu_peer_has_vnet_hdr(NetClientState *nc)
+{
+    if (!nc->peer || !nc->peer->info->has_vnet_hdr) {
+        return false;
+    }
+
+    return nc->peer->info->has_vnet_hdr(nc->peer);
+}
+
+int qemu_peer_has_vnet_hdr_len(NetClientState *nc, int len)
+{
+    if (!nc->peer || !nc->peer->info->has_vnet_hdr_len) {
+        return false;
+    }
+
+    return nc->peer->info->has_vnet_hdr_len(nc->peer, len);
+}
+
+void qemu_peer_using_vnet_hdr(NetClientState *nc, bool enable)
+{
+    if (!nc->peer || !nc->peer->info->using_vnet_hdr) {
+        return;
+    }
+
+    nc->peer->info->using_vnet_hdr(nc->peer, enable);
+}
+
+void qemu_peer_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
+                          int ecn, int ufo)
+{
+    if (!nc->peer || !nc->peer->info->set_offload) {
+        return;
+    }
+
+    nc->peer->info->set_offload(nc->peer, csum, tso4, tso6, ecn, ufo);
+}
+
+void qemu_peer_set_vnet_hdr_len(NetClientState *nc, int len)
+{
+    if (!nc->peer || !nc->peer->info->set_vnet_hdr_len) {
+        return;
+    }
+
+    nc->peer->info->set_vnet_hdr_len(nc->peer, len);
+}
+
 int qemu_can_send_packet(NetClientState *sender)
 {
     if (!sender->peer) {
-- 
1.8.5.1

Re: [Qemu-devel] [PATCH 1/5] net: extend NetClientInfo for offloading manipulations

[Stefan Hajnoczi]

On Fri, Dec 13, 2013 at 01:04:59PM +0100, Vincenzo Maffione wrote:
> diff --git a/include/net/net.h b/include/net/net.h
> index 11e1468..f5b5bae 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -50,6 +50,12 @@ typedef void (NetCleanup) (NetClientState *);
>  typedef void (LinkStatusChanged)(NetClientState *);
>  typedef void (NetClientDestructor)(NetClientState *);
>  typedef RxFilterInfo *(QueryRxFilter)(NetClientState *);
> +typedef bool (HasUfo)(NetClientState *);
> +typedef int (HasVnetHdr)(NetClientState *);

Please change the return type to bool.  bool is easier to understand
since the reader can be sure the variable only takes true/false values.

> +typedef int (HasVnetHdrLen)(NetClientState *, int);

Please change the return type to bool.

Re: [Qemu-devel] [PATCH 1/5] net: extend NetClientInfo for offloading manipulations

[Vincenzo Maffione]

[-- Attachment #1: Type: text/plain, Size: 990 bytes --]

Ok.
I did not include those modifications into the patch in order to avoid
modifications to the TAP netdev.


2014/1/13 Stefan Hajnoczi <stefanha@gmail.com>

> On Fri, Dec 13, 2013 at 01:04:59PM +0100, Vincenzo Maffione wrote:
> > diff --git a/include/net/net.h b/include/net/net.h
> > index 11e1468..f5b5bae 100644
> > --- a/include/net/net.h
> > +++ b/include/net/net.h
> > @@ -50,6 +50,12 @@ typedef void (NetCleanup) (NetClientState *);
> >  typedef void (LinkStatusChanged)(NetClientState *);
> >  typedef void (NetClientDestructor)(NetClientState *);
> >  typedef RxFilterInfo *(QueryRxFilter)(NetClientState *);
> > +typedef bool (HasUfo)(NetClientState *);
> > +typedef int (HasVnetHdr)(NetClientState *);
>
> Please change the return type to bool.  bool is easier to understand
> since the reader can be sure the variable only takes true/false values.
>
> > +typedef int (HasVnetHdrLen)(NetClientState *, int);
>
> Please change the return type to bool.
>



-- 
Vincenzo Maffione

[-- Attachment #2: Type: text/html, Size: 1463 bytes --]

[Qemu-devel] [PATCH 2/5] net: TAP uses NetClientInfo offloading callbacks

[Vincenzo Maffione]

The TAP NetClientInfo structure is inizialized with the TAP-specific
callbacks that manipulates backend offloading features.

Signed-off-by: Vincenzo Maffione <v.maffione@gmail.com>
---
 net/tap.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/tap.c b/net/tap.c
index 39c1cda..175fcb3 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -314,6 +314,12 @@ static NetClientInfo net_tap_info = {
     .receive_iov = tap_receive_iov,
     .poll = tap_poll,
     .cleanup = tap_cleanup,
+    .has_ufo = tap_has_ufo,
+    .has_vnet_hdr = tap_has_vnet_hdr,
+    .has_vnet_hdr_len = tap_has_vnet_hdr_len,
+    .using_vnet_hdr = tap_using_vnet_hdr,
+    .set_offload = tap_set_offload,
+    .set_vnet_hdr_len = tap_set_vnet_hdr_len,
 };
 
 static TAPState *net_tap_fd_init(NetClientState *peer,
-- 
1.8.5.1

[Qemu-devel] [PATCH 3/5] net: virtio-net and vmxnet3 use offloading API

[Vincenzo Maffione]

With this patch, virtio-net and vmxnet3 frontends make
use of the qemu_peer_* API for backend offloadings manipulations,
instead of calling TAP-specific functions directly.

Signed-off-by: Vincenzo Maffione <v.maffione@gmail.com>
---
 hw/net/virtio-net.c | 12 ++++++------
 hw/net/vmxnet3.c    | 14 +++++++-------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index d312b9c..c8ee2fa 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -329,7 +329,7 @@ static void peer_test_vnet_hdr(VirtIONet *n)
         return;
     }
 
-    n->has_vnet_hdr = tap_has_vnet_hdr(nc->peer);
+    n->has_vnet_hdr = qemu_peer_has_vnet_hdr(nc);
 }
 
 static int peer_has_vnet_hdr(VirtIONet *n)
@@ -342,7 +342,7 @@ static int peer_has_ufo(VirtIONet *n)
     if (!peer_has_vnet_hdr(n))
         return 0;
 
-    n->has_ufo = tap_has_ufo(qemu_get_queue(n->nic)->peer);
+    n->has_ufo = qemu_peer_has_ufo(qemu_get_queue(n->nic));
 
     return n->has_ufo;
 }
@@ -361,8 +361,8 @@ static void virtio_net_set_mrg_rx_bufs(VirtIONet *n, int mergeable_rx_bufs)
         nc = qemu_get_subqueue(n->nic, i);
 
         if (peer_has_vnet_hdr(n) &&
-            tap_has_vnet_hdr_len(nc->peer, n->guest_hdr_len)) {
-            tap_set_vnet_hdr_len(nc->peer, n->guest_hdr_len);
+            qemu_peer_has_vnet_hdr_len(nc, n->guest_hdr_len)) {
+            qemu_peer_set_vnet_hdr_len(nc, n->guest_hdr_len);
             n->host_hdr_len = n->guest_hdr_len;
         }
     }
@@ -463,7 +463,7 @@ static uint32_t virtio_net_bad_features(VirtIODevice *vdev)
 
 static void virtio_net_apply_guest_offloads(VirtIONet *n)
 {
-    tap_set_offload(qemu_get_subqueue(n->nic, 0)->peer,
+    qemu_peer_set_offload(qemu_get_subqueue(n->nic, 0),
             !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_CSUM)),
             !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_TSO4)),
             !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_TSO6)),
@@ -1546,7 +1546,7 @@ static int virtio_net_device_init(VirtIODevice *vdev)
     peer_test_vnet_hdr(n);
     if (peer_has_vnet_hdr(n)) {
         for (i = 0; i < n->max_queues; i++) {
-            tap_using_vnet_hdr(qemu_get_subqueue(n->nic, i)->peer, true);
+            qemu_peer_using_vnet_hdr(qemu_get_subqueue(n->nic, i), true);
         }
         n->host_hdr_len = sizeof(struct virtio_net_hdr);
     } else {
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 19687aa..f00c649 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1290,7 +1290,7 @@ static void vmxnet3_update_features(VMXNET3State *s)
               s->lro_supported, rxcso_supported,
               s->rx_vlan_stripping);
     if (s->peer_has_vhdr) {
-        tap_set_offload(qemu_get_queue(s->nic)->peer,
+        qemu_peer_set_offload(qemu_get_queue(s->nic),
                         rxcso_supported,
                         s->lro_supported,
                         s->lro_supported,
@@ -1883,11 +1883,11 @@ static NetClientInfo net_vmxnet3_info = {
 
 static bool vmxnet3_peer_has_vnet_hdr(VMXNET3State *s)
 {
-    NetClientState *peer = qemu_get_queue(s->nic)->peer;
+    NetClientState *nc = qemu_get_queue(s->nic);
 
-    if ((NULL != peer)                              &&
-        (peer->info->type == NET_CLIENT_OPTIONS_KIND_TAP)   &&
-        tap_has_vnet_hdr(peer)) {
+    if ((NULL != nc->peer)                              &&
+        (nc->peer->info->type == NET_CLIENT_OPTIONS_KIND_TAP)   &&
+        qemu_peer_has_vnet_hdr(nc)) {
         return true;
     }
 
@@ -1935,10 +1935,10 @@ static void vmxnet3_net_init(VMXNET3State *s)
     s->lro_supported = false;
 
     if (s->peer_has_vhdr) {
-        tap_set_vnet_hdr_len(qemu_get_queue(s->nic)->peer,
+        qemu_peer_set_vnet_hdr_len(qemu_get_queue(s->nic),
             sizeof(struct virtio_net_hdr));
 
-        tap_using_vnet_hdr(qemu_get_queue(s->nic)->peer, 1);
+        qemu_peer_using_vnet_hdr(qemu_get_queue(s->nic), 1);
     }
 
     qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
-- 
1.8.5.1

[Qemu-devel] [PATCH 4/5] net: add offloadings support to netmap backend

[Vincenzo Maffione]

Whit this patch, the netmap backend supports TSO/UFO/CSUM
offloadings, and accepts the virtio-net header, similarly to what
happens with TAP. The offloading callbacks in the NetClientInfo
interface have been implemented.

Signed-off-by: Vincenzo Maffione <v.maffione@gmail.com>
---
 net/netmap.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

diff --git a/net/netmap.c b/net/netmap.c
index 0ccc497..f02a574 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -54,6 +54,7 @@ typedef struct NetmapState {
     bool                read_poll;
     bool                write_poll;
     struct iovec        iov[IOV_MAX];
+    int                 vnet_hdr_len;  /* Current virtio-net header length. */
 } NetmapState;
 
 #define D(format, ...)                                          \
@@ -274,7 +275,7 @@ static ssize_t netmap_receive_iov(NetClientState *nc,
         return iov_size(iov, iovcnt);
     }
 
-    i = ring->cur;
+    last = i = ring->cur;
     avail = ring->avail;
 
     if (avail < iovcnt) {
@@ -394,6 +395,60 @@ static void netmap_cleanup(NetClientState *nc)
     s->me.fd = -1;
 }
 
+/* Offloading manipulation support callbacks.
+ *
+ * Currently we are simply able to pass the virtio-net header
+ * throughout the VALE switch, without modifying it or interpreting it.
+ * For this reason we are always able to support TSO/UFO/CSUM and
+ * different header lengths as long as two virtio-net-header-capable
+ * VMs are attached to the bridge.
+ */
+static bool netmap_has_ufo(NetClientState *nc)
+{
+    return true;
+}
+
+static int netmap_has_vnet_hdr(NetClientState *nc)
+{
+    return true;
+}
+
+static int netmap_has_vnet_hdr_len(NetClientState *nc, int len)
+{
+    return len >= 0 && len <= NETMAP_BDG_MAX_OFFSET;
+}
+
+static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
+{
+}
+
+static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
+                               int ecn, int ufo)
+{
+}
+
+static void netmap_set_vnet_hdr_len(NetClientState *nc, int len)
+{
+    NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
+    int err;
+    struct nmreq req;
+
+    /* Issue a NETMAP_BDG_OFFSET command to change the netmap adapter
+       offset of 'me->ifname'. */
+    memset(&req, 0, sizeof(req));
+    pstrcpy(req.nr_name, sizeof(req.nr_name), s->me.ifname);
+    req.nr_version = NETMAP_API;
+    req.nr_cmd = NETMAP_BDG_OFFSET;
+    req.nr_arg1 = len;
+    err = ioctl(s->me.fd, NIOCREGIF, &req);
+    if (err) {
+        error_report("Unable to execute NETMAP_BDG_OFFSET on %s: %s",
+                     s->me.ifname, strerror(errno));
+    } else {
+        /* Keep track of the current length, may be usefule in the future. */
+        s->vnet_hdr_len = len;
+    }
+}
 
 /* NetClientInfo methods */
 static NetClientInfo net_netmap_info = {
@@ -403,6 +458,12 @@ static NetClientInfo net_netmap_info = {
     .receive_iov = netmap_receive_iov,
     .poll = netmap_poll,
     .cleanup = netmap_cleanup,
+    .has_ufo = netmap_has_ufo,
+    .has_vnet_hdr = netmap_has_vnet_hdr,
+    .has_vnet_hdr_len = netmap_has_vnet_hdr_len,
+    .using_vnet_hdr = netmap_using_vnet_hdr,
+    .set_offload = netmap_set_offload,
+    .set_vnet_hdr_len = netmap_set_vnet_hdr_len,
 };
 
 /* The exported init function
@@ -428,6 +489,7 @@ int net_init_netmap(const NetClientOptions *opts,
     nc = qemu_new_net_client(&net_netmap_info, peer, "netmap", name);
     s = DO_UPCAST(NetmapState, nc, nc);
     s->me = me;
+    s->vnet_hdr_len = 0;
     netmap_read_poll(s, true); /* Initially only poll for reads. */
 
     return 0;
-- 
1.8.5.1

Re: [Qemu-devel] [PATCH 4/5] net: add offloadings support to netmap backend

[Stefan Hajnoczi]

On Fri, Dec 13, 2013 at 01:05:02PM +0100, Vincenzo Maffione wrote:
> +static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
> +{
> +}

I was trying to figure out whether it's okay for this function to be a
nop.  I've come to the conclusion that it's okay:

If the netdev supports vnet_hdr then we enable vnet_hdr.  If not, it
will not enable it.

Other NICs never enable vnet_hdr even if the netdev supports it.

This interface is basically useless because set_vnet_hdr_len() is
also needed and provides the same information, i.e. that we are
transitioning from vnet_hdr off -> on.

The bool argument is misleading since we never use this function to
disable vnet_hdr.  It's impossible to transition on -> off.

I suggest removing using_vnet_hdr() and instead relying solely on
set_vnet_hdr_len().  Do this as the first patch in the series before
introducing the function pointers, that way all your following patches
become simpler.

> +static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
> +                               int ecn, int ufo)
> +{
> +}

This interface is necessary for toggling offload features at runtime,
e.g. because ethtool was used inside the guest.  Offloads can impact
performance and sometimes expose bugs.  Therefore users may wish to
disable certain offloads.

Please consider supporting set_offload()!

> +static void netmap_set_vnet_hdr_len(NetClientState *nc, int len)
> +{
> +    NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
> +    int err;
> +    struct nmreq req;
> +
> +    /* Issue a NETMAP_BDG_OFFSET command to change the netmap adapter
> +       offset of 'me->ifname'. */
> +    memset(&req, 0, sizeof(req));
> +    pstrcpy(req.nr_name, sizeof(req.nr_name), s->me.ifname);
> +    req.nr_version = NETMAP_API;
> +    req.nr_cmd = NETMAP_BDG_OFFSET;
> +    req.nr_arg1 = len;
> +    err = ioctl(s->me.fd, NIOCREGIF, &req);
> +    if (err) {
> +        error_report("Unable to execute NETMAP_BDG_OFFSET on %s: %s",
> +                     s->me.ifname, strerror(errno));
> +    } else {
> +        /* Keep track of the current length, may be usefule in the future. */

s/usefule/useful/

Re: [Qemu-devel] [PATCH 4/5] net: add offloadings support to netmap backend

[Vincenzo Maffione]

[-- Attachment #1: Type: text/plain, Size: 2653 bytes --]

2014/1/13 Stefan Hajnoczi <stefanha@gmail.com>

> On Fri, Dec 13, 2013 at 01:05:02PM +0100, Vincenzo Maffione wrote:
> > +static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
> > +{
> > +}
>
> I was trying to figure out whether it's okay for this function to be a
> nop.  I've come to the conclusion that it's okay:
>
> If the netdev supports vnet_hdr then we enable vnet_hdr.  If not, it
> will not enable it.
>
> Other NICs never enable vnet_hdr even if the netdev supports it.
>
> This interface is basically useless because set_vnet_hdr_len() is
> also needed and provides the same information, i.e. that we are
> transitioning from vnet_hdr off -> on.
>
> The bool argument is misleading since we never use this function to
> disable vnet_hdr.  It's impossible to transition on -> off.
>
> I suggest removing using_vnet_hdr() and instead relying solely on
> set_vnet_hdr_len().  Do this as the first patch in the series before
> introducing the function pointers, that way all your following patches
> become simpler.
>
> Completely agree. As usual, I didn't want to change too much the existing
code.
This means that I have to remove the tap_using_vnet_hdr() calls from
virtio-net and vmxnet3 NICs before this patches, haven't I?



> > +static void netmap_set_offload(NetClientState *nc, int csum, int tso4,
> int tso6,
> > +                               int ecn, int ufo)
> > +{
> > +}
>
> This interface is necessary for toggling offload features at runtime,
> e.g. because ethtool was used inside the guest.  Offloads can impact
> performance and sometimes expose bugs.  Therefore users may wish to
> disable certain offloads.
>
> Please consider supporting set_offload()!
>

Yes, we are considering to do that, but at the moment there is no such a
support.


>
> > +static void netmap_set_vnet_hdr_len(NetClientState *nc, int len)
> > +{
> > +    NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
> > +    int err;
> > +    struct nmreq req;
> > +
> > +    /* Issue a NETMAP_BDG_OFFSET command to change the netmap adapter
> > +       offset of 'me->ifname'. */
> > +    memset(&req, 0, sizeof(req));
> > +    pstrcpy(req.nr_name, sizeof(req.nr_name), s->me.ifname);
> > +    req.nr_version = NETMAP_API;
> > +    req.nr_cmd = NETMAP_BDG_OFFSET;
> > +    req.nr_arg1 = len;
> > +    err = ioctl(s->me.fd, NIOCREGIF, &req);
> > +    if (err) {
> > +        error_report("Unable to execute NETMAP_BDG_OFFSET on %s: %s",
> > +                     s->me.ifname, strerror(errno));
> > +    } else {
> > +        /* Keep track of the current length, may be usefule in the
> future. */
>
> s/usefule/useful/
>



-- 
Vincenzo Maffione

[-- Attachment #2: Type: text/html, Size: 3654 bytes --]

Re: [Qemu-devel] [PATCH 4/5] net: add offloadings support to netmap backend

[Stefan Hajnoczi]

On Mon, Jan 13, 2014 at 04:11:25PM +0100, Vincenzo Maffione wrote:
> 2014/1/13 Stefan Hajnoczi <stefanha@gmail.com>
> 
> > On Fri, Dec 13, 2013 at 01:05:02PM +0100, Vincenzo Maffione wrote:
> > > +static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
> > > +{
> > > +}
> >
> > I was trying to figure out whether it's okay for this function to be a
> > nop.  I've come to the conclusion that it's okay:
> >
> > If the netdev supports vnet_hdr then we enable vnet_hdr.  If not, it
> > will not enable it.
> >
> > Other NICs never enable vnet_hdr even if the netdev supports it.
> >
> > This interface is basically useless because set_vnet_hdr_len() is
> > also needed and provides the same information, i.e. that we are
> > transitioning from vnet_hdr off -> on.
> >
> > The bool argument is misleading since we never use this function to
> > disable vnet_hdr.  It's impossible to transition on -> off.
> >
> > I suggest removing using_vnet_hdr() and instead relying solely on
> > set_vnet_hdr_len().  Do this as the first patch in the series before
> > introducing the function pointers, that way all your following patches
> > become simpler.
> >
> > Completely agree. As usual, I didn't want to change too much the existing
> code.
> This means that I have to remove the tap_using_vnet_hdr() calls from
> virtio-net and vmxnet3 NICs before this patches, haven't I?

Yep.  I suggest starting this patch series with a few cleanup patches to
get the tap_*() interface into shape.

Then you can move that cleaned-up interface to NetClient in the second
half of the patch series.  By doing the cleanup first, you make the
second half simpler.

Stefan

[Qemu-devel] [PATCH 5/5] net: virtio-net and vmxnet3 can use netmap offloadings

[Vincenzo Maffione]

With this patch we remove the existing checks in the virtio-net
and vmxnet3 frontends that prevents them from using
offloadings with backends different from TAP (e.g. netmap).

Signed-off-by: Vincenzo Maffione <v.maffione@gmail.com>
---
 hw/net/virtio-net.c | 4 ----
 hw/net/vmxnet3.c    | 4 +---
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index c8ee2fa..8a94539 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -325,10 +325,6 @@ static void peer_test_vnet_hdr(VirtIONet *n)
         return;
     }
 
-    if (nc->peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP) {
-        return;
-    }
-
     n->has_vnet_hdr = qemu_peer_has_vnet_hdr(nc);
 }
 
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index f00c649..0524684 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1885,9 +1885,7 @@ static bool vmxnet3_peer_has_vnet_hdr(VMXNET3State *s)
 {
     NetClientState *nc = qemu_get_queue(s->nic);
 
-    if ((NULL != nc->peer)                              &&
-        (nc->peer->info->type == NET_CLIENT_OPTIONS_KIND_TAP)   &&
-        qemu_peer_has_vnet_hdr(nc)) {
+    if (qemu_peer_has_vnet_hdr(nc)) {
         return true;
     }
 
-- 
1.8.5.1

Re: [Qemu-devel] [PATCH 0/5] Add netmap backend offloadings support

[Stefan Hajnoczi]

On Fri, Dec 13, 2013 at 01:04:58PM +0100, Vincenzo Maffione wrote:
> The purpose of this patch series is to add offloadings support
> (TSO/UFO/CSUM) to the netmap network backend, and make it possible
> for the paravirtual network frontends (virtio-net and vmxnet3) to
> use it.
> In order to achieve this, these patches extend the existing
> net.h interface to add abstract operations through which a network
> frontend can manipulate backend offloading features, instead of
> directly calling TAP-specific functions.
> 
> Guest-to-guest performance before this patches for virtio-net + netmap:
> 
>     TCP_STREAM                  5.0 Gbps
>     TCP_RR                      12.7 Gbps
>     UDP_STREAM (64-bytes)       790 Kpps
> 
> Guest-to-guest performance after this patches for virtio-net + netmap:
> 
>     TCP_STREAM                  21.4 Gbps
>     TCP_RR                      12.7 Gbps
>     UDP_STREAM (64-bytes)       790 Kpps
> 
> Experiment details:
>     - Processor: Intel i7-3770K CPU @ 3.50GHz (8 cores)
>     - Memory @ 1333 MHz
>     - Host O.S.: Archlinux with Linux 3.11
>     - Guest O.S.: Archlinux with Linux 3.11
> 
>     - QEMU command line:
>         qemu-system-x86_64 archdisk.qcow -snapshot -enable-kvm -device virtio-net-pci,ioeventfd=on,mac=00:AA:BB:CC:DD:01,netdev=mynet -netdev netmap,ifname=vale0:01,id=mynet -smp 2 -vga std -m 3G
> 
> 
> Vincenzo Maffione (5):
>   net: extend NetClientInfo for offloading manipulations
>   net: TAP uses NetClientInfo offloading callbacks
>   net: virtio-net and vmxnet3 use offloading API
>   net: add offloadings support to netmap backend
>   net: virtio-net and vmxnet3 can use netmap offloadings
> 
>  hw/net/virtio-net.c | 16 +++++---------
>  hw/net/vmxnet3.c    | 12 +++++-----
>  include/net/net.h   | 19 ++++++++++++++++
>  net/net.c           | 55 +++++++++++++++++++++++++++++++++++++++++++++
>  net/netmap.c        | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  net/tap.c           |  6 +++++
>  6 files changed, 154 insertions(+), 18 deletions(-)

Overall I'm happy with the approach.  I left comments about cleaning up
the tap interface that you're moving to NetClient, and about supporting
runtime offload feature toggling.