[Qemu-devel] [PATCH] net: netmap: implement virtio-net header probe

[Qemu-devel] [PATCH] net: netmap: implement virtio-net header probe

[Vincenzo Maffione]

Current implementation for netmap backend does not really probe the netmap
kernel module to know whether virtio-net header is supported for a specific
netmap port. This is only correct for VALE ports, but it is not correct for
hardware ports, pipes, etc.

This patch properly implements the missing probe mechanism, fixing the
implementation of the involved hooks (has_vnet_hdr, has_vnet_hdr_len, has_ufo,
...).

Vincenzo Maffione (1):
  net: netmap: probe netmap interface for virtio-net header

 net/netmap.c | 70 ++++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 26 deletions(-)

-- 
2.7.0

[Qemu-devel] [PATCH] net: netmap: probe netmap interface for virtio-net header

[Vincenzo Maffione]

Previous implementation of has_ufo, has_vnet_hdr, has_vnet_hdr_len, etc.
did not really probe for virtio-net header support for the netmap
interface attached to the backend. These callbacks were correct for
VALE ports, but incorrect for hardware NICs, pipes, monitors, etc.

This patch fixes the implementation to work properly with all kinds
of netmap ports.

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

diff --git a/net/netmap.c b/net/netmap.c
index 9710321..f2dcaeb 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -323,20 +323,55 @@ static void netmap_cleanup(NetClientState *nc)
 }
 
 /* Offloading manipulation support callbacks. */
-static bool netmap_has_ufo(NetClientState *nc)
+static int netmap_do_set_vnet_hdr_len(NetmapState *s, int len, bool err_report)
 {
-    return true;
+    struct nmreq req;
+    int err;
+
+    /* Issue a NETMAP_BDG_VNET_HDR command to change the virtio-net header
+     * length for the netmap adapter associated to 's->ifname'.
+     */
+    memset(&req, 0, sizeof(req));
+    pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname);
+    req.nr_version = NETMAP_API;
+    req.nr_cmd = NETMAP_BDG_VNET_HDR;
+    req.nr_arg1 = len;
+    err = ioctl(s->nmd->fd, NIOCREGIF, &req);
+    if (err) {
+        if (err_report) {
+            error_report("Unable to execute NETMAP_BDG_VNET_HDR on %s: %s",
+                         s->ifname, strerror(errno));
+        }
+        return -1;
+    }
+
+    /* Keep track of the current length. */
+    s->vnet_hdr_len = len;
+
+    return 0;
 }
 
-static bool netmap_has_vnet_hdr(NetClientState *nc)
+static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
 {
+    NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
+    int prev_len = s->vnet_hdr_len;
+
+    /* Check that we can set the new length. */
+    if (netmap_do_set_vnet_hdr_len(s, len, false)) {
+        return false;
+    }
+
+    /* Restore the previous length. */
+    netmap_do_set_vnet_hdr_len(s, prev_len, true);
+
     return true;
 }
 
-static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
+/* A netmap interface that supports virtio-net headers always
+ * supports UFO, so we use this callback also for the has_ufo hook. */
+static bool netmap_has_vnet_hdr(NetClientState *nc)
 {
-    return len == 0 || len == sizeof(struct virtio_net_hdr) ||
-                len == sizeof(struct virtio_net_hdr_mrg_rxbuf);
+    return netmap_has_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
 }
 
 static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
@@ -346,25 +381,8 @@ static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
 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_VNET_HDR command to change the virtio-net header
-     * length for the netmap adapter associated to 's->ifname'.
-     */
-    memset(&req, 0, sizeof(req));
-    pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname);
-    req.nr_version = NETMAP_API;
-    req.nr_cmd = NETMAP_BDG_VNET_HDR;
-    req.nr_arg1 = len;
-    err = ioctl(s->nmd->fd, NIOCREGIF, &req);
-    if (err) {
-        error_report("Unable to execute NETMAP_BDG_VNET_HDR on %s: %s",
-                     s->ifname, strerror(errno));
-    } else {
-        /* Keep track of the current length. */
-        s->vnet_hdr_len = len;
-    }
+    netmap_do_set_vnet_hdr_len(s, len, true);
 }
 
 static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
@@ -376,7 +394,7 @@ static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
      * enables the offloadings.
      */
     if (!s->vnet_hdr_len) {
-        netmap_set_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
+        netmap_do_set_vnet_hdr_len(s, sizeof(struct virtio_net_hdr), true);
     }
 }
 
@@ -388,7 +406,7 @@ static NetClientInfo net_netmap_info = {
     .receive_iov = netmap_receive_iov,
     .poll = netmap_poll,
     .cleanup = netmap_cleanup,
-    .has_ufo = netmap_has_ufo,
+    .has_ufo = netmap_has_vnet_hdr,
     .has_vnet_hdr = netmap_has_vnet_hdr,
     .has_vnet_hdr_len = netmap_has_vnet_hdr_len,
     .using_vnet_hdr = netmap_using_vnet_hdr,
-- 
2.7.0

Re: [Qemu-devel] [PATCH] net: netmap: probe netmap interface for virtio-net header

[Jason Wang]

On 02/05/2016 06:30 PM, Vincenzo Maffione wrote:
> Previous implementation of has_ufo, has_vnet_hdr, has_vnet_hdr_len, etc.
> did not really probe for virtio-net header support for the netmap
> interface attached to the backend. These callbacks were correct for
> VALE ports, but incorrect for hardware NICs, pipes, monitors, etc.
>
> This patch fixes the implementation to work properly with all kinds
> of netmap ports.
>
> Signed-off-by: Vincenzo Maffione <v.maffione@gmail.com>
> ---
>  net/netmap.c | 70 ++++++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 44 insertions(+), 26 deletions(-)
>
> diff --git a/net/netmap.c b/net/netmap.c
> index 9710321..f2dcaeb 100644
> --- a/net/netmap.c
> +++ b/net/netmap.c
> @@ -323,20 +323,55 @@ static void netmap_cleanup(NetClientState *nc)
>  }
>  
>  /* Offloading manipulation support callbacks. */
> -static bool netmap_has_ufo(NetClientState *nc)
> +static int netmap_do_set_vnet_hdr_len(NetmapState *s, int len, bool err_report)

Passing something like err_report usually means it was the
responsibility of caller to report. So let's remove the err_report and
let caller to decide based on the return value.

>  {
> -    return true;
> +    struct nmreq req;
> +    int err;
> +
> +    /* Issue a NETMAP_BDG_VNET_HDR command to change the virtio-net header
> +     * length for the netmap adapter associated to 's->ifname'.
> +     */
> +    memset(&req, 0, sizeof(req));
> +    pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname);
> +    req.nr_version = NETMAP_API;
> +    req.nr_cmd = NETMAP_BDG_VNET_HDR;
> +    req.nr_arg1 = len;
> +    err = ioctl(s->nmd->fd, NIOCREGIF, &req);
> +    if (err) {
> +        if (err_report) {
> +            error_report("Unable to execute NETMAP_BDG_VNET_HDR on %s: %s",
> +                         s->ifname, strerror(errno));
> +        }
> +        return -1;
> +    }
> +
> +    /* Keep track of the current length. */
> +    s->vnet_hdr_len = len;
> +
> +    return 0;
>  }
>  
> -static bool netmap_has_vnet_hdr(NetClientState *nc)
> +static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
>  {
> +    NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
> +    int prev_len = s->vnet_hdr_len;
> +
> +    /* Check that we can set the new length. */
> +    if (netmap_do_set_vnet_hdr_len(s, len, false)) {
> +        return false;
> +    }
> +
> +    /* Restore the previous length. */
> +    netmap_do_set_vnet_hdr_len(s, prev_len, true);
> +
>      return true;
>  }
>  
> -static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
> +/* A netmap interface that supports virtio-net headers always
> + * supports UFO, so we use this callback also for the has_ufo hook. */
> +static bool netmap_has_vnet_hdr(NetClientState *nc)
>  {
> -    return len == 0 || len == sizeof(struct virtio_net_hdr) ||
> -                len == sizeof(struct virtio_net_hdr_mrg_rxbuf);
> +    return netmap_has_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
>  }
>  
>  static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
> @@ -346,25 +381,8 @@ static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
>  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_VNET_HDR command to change the virtio-net header
> -     * length for the netmap adapter associated to 's->ifname'.
> -     */
> -    memset(&req, 0, sizeof(req));
> -    pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname);
> -    req.nr_version = NETMAP_API;
> -    req.nr_cmd = NETMAP_BDG_VNET_HDR;
> -    req.nr_arg1 = len;
> -    err = ioctl(s->nmd->fd, NIOCREGIF, &req);
> -    if (err) {
> -        error_report("Unable to execute NETMAP_BDG_VNET_HDR on %s: %s",
> -                     s->ifname, strerror(errno));
> -    } else {
> -        /* Keep track of the current length. */
> -        s->vnet_hdr_len = len;
> -    }
> +    netmap_do_set_vnet_hdr_len(s, len, true);
>  }
>  
>  static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
> @@ -376,7 +394,7 @@ static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
>       * enables the offloadings.
>       */
>      if (!s->vnet_hdr_len) {
> -        netmap_set_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
> +        netmap_do_set_vnet_hdr_len(s, sizeof(struct virtio_net_hdr), true);
>      }
>  }
>  
> @@ -388,7 +406,7 @@ static NetClientInfo net_netmap_info = {
>      .receive_iov = netmap_receive_iov,
>      .poll = netmap_poll,
>      .cleanup = netmap_cleanup,
> -    .has_ufo = netmap_has_ufo,
> +    .has_ufo = netmap_has_vnet_hdr,

This look suspicious, I'm not sure this is correct for netmap. May need
a comment to explain. Usually vnet hdr does not imply ufo.

>      .has_vnet_hdr = netmap_has_vnet_hdr,
>      .has_vnet_hdr_len = netmap_has_vnet_hdr_len,
>      .using_vnet_hdr = netmap_using_vnet_hdr,

Re: [Qemu-devel] [PATCH] net: netmap: probe netmap interface for virtio-net header

[Vincenzo Maffione]

2016-02-18 3:52 GMT+01:00 Jason Wang <jasowang@redhat.com>:
>
>
> On 02/05/2016 06:30 PM, Vincenzo Maffione wrote:
>> Previous implementation of has_ufo, has_vnet_hdr, has_vnet_hdr_len, etc.
>> did not really probe for virtio-net header support for the netmap
>> interface attached to the backend. These callbacks were correct for
>> VALE ports, but incorrect for hardware NICs, pipes, monitors, etc.
>>
>> This patch fixes the implementation to work properly with all kinds
>> of netmap ports.
>>
>> Signed-off-by: Vincenzo Maffione <v.maffione@gmail.com>
>> ---
>>  net/netmap.c | 70 ++++++++++++++++++++++++++++++++++++++----------------------
>>  1 file changed, 44 insertions(+), 26 deletions(-)
>>
>> diff --git a/net/netmap.c b/net/netmap.c
>> index 9710321..f2dcaeb 100644
>> --- a/net/netmap.c
>> +++ b/net/netmap.c
>> @@ -323,20 +323,55 @@ static void netmap_cleanup(NetClientState *nc)
>>  }
>>
>>  /* Offloading manipulation support callbacks. */
>> -static bool netmap_has_ufo(NetClientState *nc)
>> +static int netmap_do_set_vnet_hdr_len(NetmapState *s, int len, bool err_report)
>
> Passing something like err_report usually means it was the
> responsibility of caller to report. So let's remove the err_report and
> let caller to decide based on the return value.
>

Ok, I'll try to fix this with the next patch.

>>  {
>> -    return true;
>> +    struct nmreq req;
>> +    int err;
>> +
>> +    /* Issue a NETMAP_BDG_VNET_HDR command to change the virtio-net header
>> +     * length for the netmap adapter associated to 's->ifname'.
>> +     */
>> +    memset(&req, 0, sizeof(req));
>> +    pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname);
>> +    req.nr_version = NETMAP_API;
>> +    req.nr_cmd = NETMAP_BDG_VNET_HDR;
>> +    req.nr_arg1 = len;
>> +    err = ioctl(s->nmd->fd, NIOCREGIF, &req);
>> +    if (err) {
>> +        if (err_report) {
>> +            error_report("Unable to execute NETMAP_BDG_VNET_HDR on %s: %s",
>> +                         s->ifname, strerror(errno));
>> +        }
>> +        return -1;
>> +    }
>> +
>> +    /* Keep track of the current length. */
>> +    s->vnet_hdr_len = len;
>> +
>> +    return 0;
>>  }
>>
>> -static bool netmap_has_vnet_hdr(NetClientState *nc)
>> +static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
>>  {
>> +    NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
>> +    int prev_len = s->vnet_hdr_len;
>> +
>> +    /* Check that we can set the new length. */
>> +    if (netmap_do_set_vnet_hdr_len(s, len, false)) {
>> +        return false;
>> +    }
>> +
>> +    /* Restore the previous length. */
>> +    netmap_do_set_vnet_hdr_len(s, prev_len, true);
>> +
>>      return true;
>>  }
>>
>> -static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
>> +/* A netmap interface that supports virtio-net headers always
>> + * supports UFO, so we use this callback also for the has_ufo hook. */
>> +static bool netmap_has_vnet_hdr(NetClientState *nc)
>>  {
>> -    return len == 0 || len == sizeof(struct virtio_net_hdr) ||
>> -                len == sizeof(struct virtio_net_hdr_mrg_rxbuf);
>> +    return netmap_has_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
>>  }
>>
>>  static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
>> @@ -346,25 +381,8 @@ static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
>>  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_VNET_HDR command to change the virtio-net header
>> -     * length for the netmap adapter associated to 's->ifname'.
>> -     */
>> -    memset(&req, 0, sizeof(req));
>> -    pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname);
>> -    req.nr_version = NETMAP_API;
>> -    req.nr_cmd = NETMAP_BDG_VNET_HDR;
>> -    req.nr_arg1 = len;
>> -    err = ioctl(s->nmd->fd, NIOCREGIF, &req);
>> -    if (err) {
>> -        error_report("Unable to execute NETMAP_BDG_VNET_HDR on %s: %s",
>> -                     s->ifname, strerror(errno));
>> -    } else {
>> -        /* Keep track of the current length. */
>> -        s->vnet_hdr_len = len;
>> -    }
>> +    netmap_do_set_vnet_hdr_len(s, len, true);
>>  }
>>
>>  static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
>> @@ -376,7 +394,7 @@ static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
>>       * enables the offloadings.
>>       */
>>      if (!s->vnet_hdr_len) {
>> -        netmap_set_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
>> +        netmap_do_set_vnet_hdr_len(s, sizeof(struct virtio_net_hdr), true);
>>      }
>>  }
>>
>> @@ -388,7 +406,7 @@ static NetClientInfo net_netmap_info = {
>>      .receive_iov = netmap_receive_iov,
>>      .poll = netmap_poll,
>>      .cleanup = netmap_cleanup,
>> -    .has_ufo = netmap_has_ufo,
>> +    .has_ufo = netmap_has_vnet_hdr,
>
> This look suspicious, I'm not sure this is correct for netmap. May need
> a comment to explain. Usually vnet hdr does not imply ufo.
>
>>      .has_vnet_hdr = netmap_has_vnet_hdr,
>>      .has_vnet_hdr_len = netmap_has_vnet_hdr_len,
>>      .using_vnet_hdr = netmap_using_vnet_hdr,
>

Yes, I know it sounds weird in general, but a netmap port using
virtio-net headers always provides TCPv4, TCPv6, UDP and ECN
offloadings (done in software inside netmap).

This patch already provides a little comment about UFO support on the
netmap_has_vnet_hdr() function. Do you want me to move it here, or is
the comment not understandable enough?

Thanks for your review,
  Vincenzo

Re: [Qemu-devel] [PATCH] net: netmap: probe netmap interface for virtio-net header

[Jason Wang]

On 02/18/2016 09:34 PM, Vincenzo Maffione wrote:
>>>  static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
>>> >> @@ -376,7 +394,7 @@ static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
>>> >>       * enables the offloadings.
>>> >>       */
>>> >>      if (!s->vnet_hdr_len) {
>>> >> -        netmap_set_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
>>> >> +        netmap_do_set_vnet_hdr_len(s, sizeof(struct virtio_net_hdr), true);
>>> >>      }
>>> >>  }
>>> >>
>>> >> @@ -388,7 +406,7 @@ static NetClientInfo net_netmap_info = {
>>> >>      .receive_iov = netmap_receive_iov,
>>> >>      .poll = netmap_poll,
>>> >>      .cleanup = netmap_cleanup,
>>> >> -    .has_ufo = netmap_has_ufo,
>>> >> +    .has_ufo = netmap_has_vnet_hdr,
>> >
>> > This look suspicious, I'm not sure this is correct for netmap. May need
>> > a comment to explain. Usually vnet hdr does not imply ufo.
>> >
>>> >>      .has_vnet_hdr = netmap_has_vnet_hdr,
>>> >>      .has_vnet_hdr_len = netmap_has_vnet_hdr_len,
>>> >>      .using_vnet_hdr = netmap_using_vnet_hdr,
>> >
> Yes, I know it sounds weird in general, but a netmap port using
> virtio-net headers always provides TCPv4, TCPv6, UDP and ECN
> offloadings (done in software inside netmap).
>
> This patch already provides a little comment about UFO support on the
> netmap_has_vnet_hdr() function. 

Just saw that.

> Do you want me to move it here, or is
> the comment not understandable enough?
>
> Thanks for your review,
>   Vincenzo

Nope, it's ok.

Thanks