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

[Vincenzo Maffione]

2016-02-24 9:21 GMT+01:00 Jason Wang <address@hidden>:
>
>
> 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 <address@hidden>
>
> 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,
>



-- 
Vincenzo Maffione