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 <address@hidden>
> ---
>  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,