2016-02-24 9:21 GMT+01:00 Jason Wang <jasow...@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.maffi...@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,
>
--
Vincenzo Maffione
