From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id 8A12BDED for ; Tue, 28 Aug 2018 09:08:20 +0200 (CEST) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Aug 2018 00:08:19 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.53,298,1531810800"; d="scan'208";a="85525561" Received: from fbsd.sh.intel.com ([10.67.104.231]) by orsmga001.jf.intel.com with ESMTP; 28 Aug 2018 00:06:22 -0700 Date: Tue, 28 Aug 2018 15:04:05 +0800 From: Tiwei Bie To: eric zhang Cc: maxime.coquelin@redhat.com, zhihong.wang@intel.com, dev@dpdk.org, Allain.Legacy@windriver.com, Matt.Peters@windriver.com Message-ID: <20180828070404.GC80296@fbsd.sh.intel.com> References: <1535395044-28925-1-git-send-email-eric.zhang@windriver.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1535395044-28925-1-git-send-email-eric.zhang@windriver.com> User-Agent: Mutt/1.10.1 (2018-07-13) Subject: Re: [dpdk-dev] [PATCH v2] net/virtio-user: check negotiated features before set X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Aug 2018 07:08:21 -0000 On Mon, Aug 27, 2018 at 02:37:24PM -0400, eric zhang wrote: > This patch checks negotiated features to see if necessary to offload > before set the tap device offload capabilities. It also checks if kernel > support the TUNSETOFFLOAD operation. > > Signed-off-by: eric zhang > > --- > v2: > * don't return failure when failed to set offload to tap > * check if offloads available when handling VHOST_GET_FEATURES > --- > drivers/net/virtio/virtio_user/vhost_kernel.c | 8 ++-- > drivers/net/virtio/virtio_user/vhost_kernel_tap.c | 55 +++++++++++++++++------ > drivers/net/virtio/virtio_user/vhost_kernel_tap.h | 2 +- > 3 files changed, 47 insertions(+), 18 deletions(-) > > diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c b/drivers/net/virtio/virtio_user/vhost_kernel.c > index dd24b6b..5c39f26 100644 > --- a/drivers/net/virtio/virtio_user/vhost_kernel.c > +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c > @@ -278,9 +278,11 @@ struct vhost_memory_kernel { > if (!ret && req_kernel == VHOST_GET_FEATURES) { > /* with tap as the backend, all these features are supported > * but not claimed by vhost-net, so we add them back when > - * reporting to upper layer. > + * reporting to upper layer. For guest offloads we check if > + * they are available in the negotiated features. > */ > - *((uint64_t *)arg) |= VHOST_KERNEL_GUEST_OFFLOADS_MASK; > + *((uint64_t *)arg) |= > + (dev->features & VHOST_KERNEL_GUEST_OFFLOADS_MASK); VHOST_GET_FEATURES returns the features supported by backend instead of the negotiated features. We need to check whether tap support vnet_hdr etc to know whether these features are available. > *((uint64_t *)arg) |= VHOST_KERNEL_HOST_OFFLOADS_MASK; > > /* vhost_kernel will not declare this feature, but it does > @@ -381,7 +383,7 @@ struct vhost_memory_kernel { > hdr_size = sizeof(struct virtio_net_hdr); > > tapfd = vhost_kernel_open_tap(&dev->ifname, hdr_size, req_mq, > - (char *)dev->mac_addr); > + (char *)dev->mac_addr, dev->features); > if (tapfd < 0) { > PMD_DRV_LOG(ERR, "fail to open tap for vhost kernel"); > return -1; > diff --git a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c > index d036428..5e86404 100644 > --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c > +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c > @@ -45,21 +45,54 @@ > > #include "vhost_kernel_tap.h" > #include "../virtio_logs.h" > +#include "../virtio_pci.h" > + > +static int > +vhost_kernel_tap_set_offload(int fd, uint64_t feature) s/feature/features/ > +{ > + unsigned int offload = 0; > + > + if (feature & (1ULL << VIRTIO_NET_F_GUEST_CSUM)) > + offload |= TUN_F_CSUM; > + if (feature & (1ULL << VIRTIO_NET_F_GUEST_TSO4)) > + offload |= TUN_F_TSO4; > + if (feature & (1ULL << VIRTIO_NET_F_GUEST_TSO6)) > + offload |= TUN_F_TSO6; > + if (feature & ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | > + (1ULL << VIRTIO_NET_F_GUEST_TSO6)) && > + (feature & (1ULL << VIRTIO_NET_F_GUEST_ECN))) > + offload |= TUN_F_TSO_ECN; > + if (feature & (1ULL << VIRTIO_NET_F_GUEST_UFO)) > + offload |= TUN_F_UFO; The features other than CSUM feature depends on the CSUM feature. Maybe it's better to do what QEMU does, i.e. announce these offloads in tap only when the CSUM feature is available: https://github.com/qemu/qemu/blob/6ad90805383e/net/tap-linux.c#L247-L257 Thanks > + > + if (offload != 0) { > + /* Check if our kernel supports TUNSETOFFLOAD */ > + if (ioctl(fd, TUNSETOFFLOAD, 0) != 0 && errno == EINVAL) { > + PMD_DRV_LOG(ERR, "Kernel does't support TUNSETOFFLOAD\n"); > + return -ENOTSUP; > + } > + > + if (ioctl(fd, TUNSETOFFLOAD, offload) != 0) { > + offload &= ~TUN_F_UFO; > + if (ioctl(fd, TUNSETOFFLOAD, offload) != 0) { > + PMD_DRV_LOG(ERR, "TUNSETOFFLOAD ioctl() failed: %s\n", > + strerror(errno)); > + return -1; > + } > + } > + } > + > + return 0; > +} > > int > vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq, > - const char *mac) > + const char *mac, uint64_t features) > { > unsigned int tap_features; > int sndbuf = INT_MAX; > struct ifreq ifr; > int tapfd; > - unsigned int offload = > - TUN_F_CSUM | > - TUN_F_TSO4 | > - TUN_F_TSO6 | > - TUN_F_TSO_ECN | > - TUN_F_UFO; > > /* TODO: > * 1. verify we can get/set vnet_hdr_len, tap_probe_vnet_hdr_len > @@ -119,13 +152,7 @@ > goto error; > } > > - /* TODO: before set the offload capabilities, we'd better (1) check > - * negotiated features to see if necessary to offload; (2) query tap > - * to see if it supports the offload capabilities. > - */ > - if (ioctl(tapfd, TUNSETOFFLOAD, offload) != 0) > - PMD_DRV_LOG(ERR, "TUNSETOFFLOAD ioctl() failed: %s", > - strerror(errno)); > + vhost_kernel_tap_set_offload(tapfd, features); > > memset(&ifr, 0, sizeof(ifr)); > ifr.ifr_hwaddr.sa_family = ARPHRD_ETHER; > diff --git a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h > index 402f964..ea7a6c9 100644 > --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h > +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h > @@ -65,4 +65,4 @@ > #define PATH_NET_TUN "/dev/net/tun" > > int vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq, > - const char *mac); > + const char *mac, uint64_t features); > -- > 1.8.3.1 >