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 <address@hidden>:
>
>
> 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.
>

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