[v2] net: netmap: probe netmap interface for virtio-net header

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 | 62 +++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 22 deletions(-)

On 02/23/2016 04:46 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>

Looks good overall, just few nits, see below.

Thanks

> ---
>  net/netmap.c | 62 +++++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 40 insertions(+), 22 deletions(-)
>
> diff --git a/net/netmap.c b/net/netmap.c
> index 9710321..ef64be0 100644
> --- a/net/netmap.c
> +++ b/net/netmap.c
> @@ -323,20 +323,51 @@ 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)
>  {

Like tap, how about rename this function to netmap_fd_set_vnet_hdr_len?

> -    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) {
> +        return err;
> +    }
> +
> +    /* Keep track of the current length. */
> +    s->vnet_hdr_len = len;

How about move this to netmap_set_vnet_hdr_len() to let this function
only cares about ioctl?

> +
> +    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)) {
> +        return false;
> +    }
> +
> +    /* Restore the previous length. */
> +    netmap_do_set_vnet_hdr_len(s, prev_len);

Do we need to check and abort() if it fails like tap?

> +
>      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)
> @@ -347,23 +378,11 @@ 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);
> +    err = netmap_do_set_vnet_hdr_len(s, len);
>      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;
>      }
>  }
>  
> @@ -373,8 +392,7 @@ static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
>      NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
>  
>      /* Setting a virtio-net header length greater than zero automatically
> -     * enables the offloadings.
> -     */
> +     * enables the offloadings. */
>      if (!s->vnet_hdr_len) {
>          netmap_set_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
>      }
> @@ -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,

2016-02-24 9:21 GMT+01:00 Jason Wang <jasowang@redhat.com>:
>
>
> On 02/23/2016 04:46 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>
>
> Looks good overall, just few nits, see below.
>
> Thanks
>
>> ---
>>  net/netmap.c | 62 +++++++++++++++++++++++++++++++++++++++---------------------
>>  1 file changed, 40 insertions(+), 22 deletions(-)
>>
>> diff --git a/net/netmap.c b/net/netmap.c
>> index 9710321..ef64be0 100644
>> --- a/net/netmap.c
>> +++ b/net/netmap.c
>> @@ -323,20 +323,51 @@ 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)
>>  {
>
> Like tap, how about rename this function to netmap_fd_set_vnet_hdr_len?
Sure.

>
>> -    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) {
>> +        return err;
>> +    }
>> +
>> +    /* Keep track of the current length. */
>> +    s->vnet_hdr_len = len;
>
> How about move this to netmap_set_vnet_hdr_len() to let this function
> only cares about ioctl?

Agree, but it is correct only if we abort() below (and we will).

>
>> +
>> +    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)) {
>> +        return false;
>> +    }
>> +
>> +    /* Restore the previous length. */
>> +    netmap_do_set_vnet_hdr_len(s, prev_len);
>
> Do we need to check and abort() if it fails like tap?

Yes, it's better, also because this should never happen.

Thanks,
  Vincenzo

>
>> +
>>      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)
>> @@ -347,23 +378,11 @@ 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);
>> +    err = netmap_do_set_vnet_hdr_len(s, len);
>>      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;
>>      }
>>  }
>>
>> @@ -373,8 +392,7 @@ static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
>>      NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
>>
>>      /* Setting a virtio-net header length greater than zero automatically
>> -     * enables the offloadings.
>> -     */
>> +     * enables the offloadings. */
>>      if (!s->vnet_hdr_len) {
>>          netmap_set_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
>>      }
>> @@ -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,
>