From patchwork Tue Sep 28 08:51:14 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Marchand X-Patchwork-Id: 99870 X-Patchwork-Delegate: maxime.coquelin@redhat.com Return-Path: X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 38512A0032; Tue, 28 Sep 2021 10:51:35 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id AF07D40E3C; Tue, 28 Sep 2021 10:51:34 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mails.dpdk.org (Postfix) with ESMTP id 8CFB740DF6 for ; Tue, 28 Sep 2021 10:51:33 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1632819093; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=cKzEeebmybbbVnrxSB4jcYEXkklsIYb3B2XgTU1EMnI=; b=hZPJHmtBlS//Thr5r86ehyfrxEPCVV8lnqXWviex74Vomy5Dc7dK/aX8rfqCnySjT/HOi5 HleEMyikMpDIjd4KC5BTadTvRpTObDUCzK2VtK7qm2rtOxrPxQ+mV1jXGs1ILbmZg2M6bq TKo8nHjsesu2a/BHDJOTKEEkYzaSj+0= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-431-H2U_oZmhNliyCSI7DdWMxA-1; Tue, 28 Sep 2021 04:51:29 -0400 X-MC-Unique: H2U_oZmhNliyCSI7DdWMxA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 7659E1006AA3; Tue, 28 Sep 2021 08:51:28 +0000 (UTC) Received: from dmarchan.remote.csb (unknown [10.40.192.158]) by smtp.corp.redhat.com (Postfix) with ESMTP id D362860871; Tue, 28 Sep 2021 08:51:23 +0000 (UTC) From: David Marchand To: dev@dpdk.org Cc: olivier.matz@6wind.com, Maxime Coquelin , Chenbo Xia Date: Tue, 28 Sep 2021 10:51:14 +0200 Message-Id: <20210928085114.30737-1-david.marchand@redhat.com> In-Reply-To: <20210917093344.31719-1-david.marchand@redhat.com> References: <20210917093344.31719-1-david.marchand@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=david.marchand@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Subject: [dpdk-dev] [PATCH v2] net/virtio: fix virtio-user init when using existing tap X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" When attaching to an existing mono queue tap, the virtio-user was not reporting that the virtio device was not properly initialised which prevented from starting the port later. $ ip tuntap add test mode tap $ dpdk-testpmd --vdev \ net_virtio_user0,iface=test,path=/dev/vhost-net,queues=2 -- -i ... virtio_user_dev_init_mac(): (/dev/vhost-net) No valid MAC in devargs or device, use random vhost_kernel_open_tap(): TUNSETIFF failed: Invalid argument vhost_kernel_enable_queue_pair(): fail to open tap for vhost kernel virtio_user_start_device(): (/dev/vhost-net) Failed to start device ... Configuring Port 0 (socket 0) vhost_kernel_open_tap(): TUNSETIFF failed: Invalid argument vhost_kernel_enable_queue_pair(): fail to open tap for vhost kernel virtio_set_multiple_queues(): Multiqueue configured but send command failed, this is too late now... Fail to start port 0: Invalid argument Please stop the ports first Done The virtio-user with vhost-kernel backend was going through a lot of complications to initialise tap fds only when using them. For each qp enabled for the first time, a tapfd was created via TUNSETIFF with unneeded additional steps (see below) and then mapped to the right qp in the vhost-net backend. Unneeded steps (as long as it has been done once for the port): - tap features were queried while this is a constant on a running system, - the device name in DPDK was updated, - the mac address of the tap was set, On subsequent qps state change, the vhost-net backend fd mapping was updated and the associated queue/tapfd were disabled/enabled via TUNSETQUEUE. Now, this patch simplifies the whole logic by keeping all tapfds opened and in enabled state (from the tap point of view) at all time. Unused ioctl defines are removed. Tap features are validated earlier to fail initialisation asap. Tap name discovery and mac address configuration are moved when configuring qp 0. To support attaching to mono queue tap, the virtio-user driver now tries to attach in multi queue first, then fallbacks to mono queue. Finally (but this is more for consistency), VIRTIO_NET_F_MQ feature is exposed only if the underlying tap supports multi queue. Signed-off-by: David Marchand Reviewed-by: Maxime Coquelin --- Changes since v1: - refactored tap_open() following Olivier comment and updated log messages level accordingly, - added more error logs, --- drivers/net/virtio/virtio_user/vhost_kernel.c | 92 +++++---- .../net/virtio/virtio_user/vhost_kernel_tap.c | 180 +++++++++--------- .../net/virtio/virtio_user/vhost_kernel_tap.h | 16 +- 3 files changed, 153 insertions(+), 135 deletions(-) diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c b/drivers/net/virtio/virtio_user/vhost_kernel.c index d65f89e1fc..202a8cdee1 100644 --- a/drivers/net/virtio/virtio_user/vhost_kernel.c +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c @@ -120,9 +120,9 @@ vhost_kernel_set_owner(struct virtio_user_dev *dev) static int vhost_kernel_get_features(struct virtio_user_dev *dev, uint64_t *features) { - int ret; - unsigned int tap_features; struct vhost_kernel_data *data = dev->backend_data; + unsigned int tap_flags; + int ret; ret = vhost_kernel_ioctl(data->vhostfds[0], VHOST_GET_FEATURES, features); if (ret < 0) { @@ -130,7 +130,7 @@ vhost_kernel_get_features(struct virtio_user_dev *dev, uint64_t *features) return -1; } - ret = tap_support_features(&tap_features); + ret = tap_get_flags(data->tapfds[0], &tap_flags); if (ret < 0) { PMD_DRV_LOG(ERR, "Failed to get TAP features"); return -1; @@ -140,7 +140,7 @@ vhost_kernel_get_features(struct virtio_user_dev *dev, uint64_t *features) * but not claimed by vhost-net, so we add them back when * reporting to upper layer. */ - if (tap_features & IFF_VNET_HDR) { + if (tap_flags & IFF_VNET_HDR) { *features |= VHOST_KERNEL_GUEST_OFFLOADS_MASK; *features |= VHOST_KERNEL_HOST_OFFLOADS_MASK; } @@ -148,7 +148,7 @@ vhost_kernel_get_features(struct virtio_user_dev *dev, uint64_t *features) /* vhost_kernel will not declare this feature, but it does * support multi-queue. */ - if (tap_features & IFF_MULTI_QUEUE) + if (tap_flags & IFF_MULTI_QUEUE) *features |= (1ull << VIRTIO_NET_F_MQ); return 0; @@ -380,9 +380,20 @@ vhost_kernel_set_status(struct virtio_user_dev *dev __rte_unused, uint8_t status static int vhost_kernel_setup(struct virtio_user_dev *dev) { - int vhostfd; - uint32_t q, i; struct vhost_kernel_data *data; + unsigned int tap_features; + unsigned int tap_flags; + const char *ifname; + uint32_t q, i; + int vhostfd; + + if (tap_support_features(&tap_features) < 0) + return -1; + + if ((tap_features & IFF_VNET_HDR) == 0) { + PMD_INIT_LOG(ERR, "TAP does not support IFF_VNET_HDR"); + return -1; + } data = malloc(sizeof(*data)); if (!data) { @@ -414,18 +425,43 @@ vhost_kernel_setup(struct virtio_user_dev *dev) PMD_DRV_LOG(ERR, "fail to open %s, %s", dev->path, strerror(errno)); goto err_tapfds; } - data->vhostfds[i] = vhostfd; } + ifname = dev->ifname != NULL ? dev->ifname : "tap%d"; + data->tapfds[0] = tap_open(ifname, (tap_features & IFF_MULTI_QUEUE) != 0); + if (data->tapfds[0] < 0) + goto err_tapfds; + if (dev->ifname == NULL && tap_get_name(data->tapfds[0], &dev->ifname) < 0) { + PMD_DRV_LOG(ERR, "fail to get tap name (%d)", data->tapfds[0]); + goto err_tapfds; + } + if (tap_get_flags(data->tapfds[0], &tap_flags) < 0) { + PMD_DRV_LOG(ERR, "fail to get tap flags for tap %s", dev->ifname); + goto err_tapfds; + } + if ((tap_flags & IFF_MULTI_QUEUE) == 0 && dev->max_queue_pairs > 1) { + PMD_DRV_LOG(ERR, "tap %s does not support multi queue", dev->ifname); + goto err_tapfds; + } + + for (i = 1; i < dev->max_queue_pairs; i++) { + data->tapfds[i] = tap_open(dev->ifname, true); + if (data->tapfds[i] < 0) + goto err_tapfds; + } + dev->backend_data = data; return 0; err_tapfds: - for (i = 0; i < dev->max_queue_pairs; i++) + for (i = 0; i < dev->max_queue_pairs; i++) { if (data->vhostfds[i] >= 0) close(data->vhostfds[i]); + if (data->tapfds[i] >= 0) + close(data->tapfds[i]); + } free(data->tapfds); err_vhostfds: @@ -488,58 +524,42 @@ vhost_kernel_enable_queue_pair(struct virtio_user_dev *dev, uint16_t pair_idx, int enable) { + struct vhost_kernel_data *data = dev->backend_data; int hdr_size; int vhostfd; int tapfd; - int req_mq = (dev->max_queue_pairs > 1); - struct vhost_kernel_data *data = dev->backend_data; - - vhostfd = data->vhostfds[pair_idx]; if (dev->qp_enabled[pair_idx] == enable) return 0; + vhostfd = data->vhostfds[pair_idx]; + tapfd = data->tapfds[pair_idx]; + if (!enable) { - tapfd = data->tapfds[pair_idx]; if (vhost_kernel_set_backend(vhostfd, -1) < 0) { PMD_DRV_LOG(ERR, "fail to set backend for vhost kernel"); return -1; } - if (req_mq && vhost_kernel_tap_set_queue(tapfd, false) < 0) { - PMD_DRV_LOG(ERR, "fail to disable tap for vhost kernel"); - return -1; - } dev->qp_enabled[pair_idx] = false; return 0; } - if (data->tapfds[pair_idx] >= 0) { - tapfd = data->tapfds[pair_idx]; - if (vhost_kernel_tap_set_offload(tapfd, dev->features) == -1) - return -1; - if (req_mq && vhost_kernel_tap_set_queue(tapfd, true) < 0) { - PMD_DRV_LOG(ERR, "fail to enable tap for vhost kernel"); - return -1; - } - goto set_backend; - } - if ((dev->features & (1ULL << VIRTIO_NET_F_MRG_RXBUF)) || (dev->features & (1ULL << VIRTIO_F_VERSION_1))) hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf); else hdr_size = sizeof(struct virtio_net_hdr); - tapfd = vhost_kernel_open_tap(&dev->ifname, hdr_size, req_mq, - (char *)dev->mac_addr, dev->features); - if (tapfd < 0) { - PMD_DRV_LOG(ERR, "fail to open tap for vhost kernel"); + /* Set mac on tap only once when starting */ + if (!dev->started && pair_idx == 0 && + tap_set_mac(data->tapfds[pair_idx], dev->mac_addr) < 0) return -1; - } - data->tapfds[pair_idx] = tapfd; + if (vhost_kernel_tap_setup(tapfd, hdr_size, dev->features) < 0) { + PMD_DRV_LOG(ERR, "fail to setup tap for vhost kernel"); + return -1; + } -set_backend: if (vhost_kernel_set_backend(vhostfd, tapfd) < 0) { PMD_DRV_LOG(ERR, "fail to set backend 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 99096bdf39..7e2f8ab15e 100644 --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c @@ -42,6 +42,91 @@ tap_support_features(unsigned int *tap_features) } int +tap_open(const char *ifname, bool multi_queue) +{ + struct ifreq ifr; + int tapfd; + + tapfd = open(PATH_NET_TUN, O_RDWR); + if (tapfd < 0) { + PMD_DRV_LOG(ERR, "fail to open %s: %s", PATH_NET_TUN, strerror(errno)); + return -1; + } + if (fcntl(tapfd, F_SETFL, O_NONBLOCK) < 0) { + PMD_DRV_LOG(ERR, "fcntl tapfd failed: %s", strerror(errno)); + close(tapfd); + return -1; + } + +retry_mono_q: + memset(&ifr, 0, sizeof(ifr)); + strncpy(ifr.ifr_name, ifname, IFNAMSIZ - 1); + ifr.ifr_flags = IFF_TAP | IFF_NO_PI | IFF_VNET_HDR; + if (multi_queue) + ifr.ifr_flags |= IFF_MULTI_QUEUE; + if (ioctl(tapfd, TUNSETIFF, (void *)&ifr) == -1) { + if (multi_queue) { + PMD_DRV_LOG(DEBUG, + "TUNSETIFF failed (will retry without IFF_MULTI_QUEUE): %s", + strerror(errno)); + multi_queue = false; + goto retry_mono_q; + } + + PMD_DRV_LOG(ERR, "TUNSETIFF failed: %s", strerror(errno)); + close(tapfd); + tapfd = -1; + } + return tapfd; +} + +int +tap_get_name(int tapfd, char **name) +{ + struct ifreq ifr; + int ret; + + memset(&ifr, 0, sizeof(ifr)); + if (ioctl(tapfd, TUNGETIFF, (void *)&ifr) == -1) { + PMD_DRV_LOG(ERR, "TUNGETIFF failed: %s", strerror(errno)); + return -1; + } + ret = asprintf(name, "%s", ifr.ifr_name); + if (ret != -1) + ret = 0; + return ret; +} + +int +tap_get_flags(int tapfd, unsigned int *tap_flags) +{ + struct ifreq ifr; + + memset(&ifr, 0, sizeof(ifr)); + if (ioctl(tapfd, TUNGETIFF, (void *)&ifr) == -1) { + PMD_DRV_LOG(ERR, "TUNGETIFF failed: %s", strerror(errno)); + return -1; + } + *tap_flags = ifr.ifr_flags; + return 0; +} + +int +tap_set_mac(int tapfd, uint8_t *mac) +{ + struct ifreq ifr; + + memset(&ifr, 0, sizeof(ifr)); + ifr.ifr_hwaddr.sa_family = ARPHRD_ETHER; + memcpy(ifr.ifr_hwaddr.sa_data, mac, RTE_ETHER_ADDR_LEN); + if (ioctl(tapfd, SIOCSIFHWADDR, (void *)&ifr) == -1) { + PMD_DRV_LOG(ERR, "SIOCSIFHWADDR failed: %s", strerror(errno)); + return -1; + } + return 0; +} + +static int vhost_kernel_tap_set_offload(int fd, uint64_t features) { unsigned int offload = 0; @@ -79,24 +164,9 @@ vhost_kernel_tap_set_offload(int fd, uint64_t features) } int -vhost_kernel_tap_set_queue(int fd, bool attach) -{ - struct ifreq ifr = { - .ifr_flags = attach ? IFF_ATTACH_QUEUE : IFF_DETACH_QUEUE, - }; - - return ioctl(fd, TUNSETQUEUE, &ifr); -} - -int -vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq, - const char *mac, uint64_t features) +vhost_kernel_tap_setup(int tapfd, int hdr_size, uint64_t features) { - unsigned int tap_features; - char *tap_name = NULL; int sndbuf = INT_MAX; - struct ifreq ifr; - int tapfd; int ret; /* TODO: @@ -104,86 +174,18 @@ vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq, * 2. get number of memory regions from vhost module parameter * max_mem_regions, supported in newer version linux kernel */ - tapfd = open(PATH_NET_TUN, O_RDWR); - if (tapfd < 0) { - PMD_DRV_LOG(ERR, "fail to open %s: %s", - PATH_NET_TUN, strerror(errno)); - return -1; - } - - /* Construct ifr */ - memset(&ifr, 0, sizeof(ifr)); - ifr.ifr_flags = IFF_TAP | IFF_NO_PI; - - if (ioctl(tapfd, TUNGETFEATURES, &tap_features) == -1) { - PMD_DRV_LOG(ERR, "TUNGETFEATURES failed: %s", strerror(errno)); - goto error; - } - if (tap_features & IFF_ONE_QUEUE) - ifr.ifr_flags |= IFF_ONE_QUEUE; - - /* Let tap instead of vhost-net handle vnet header, as the latter does - * not support offloading. And in this case, we should not set feature - * bit VHOST_NET_F_VIRTIO_NET_HDR. - */ - if (tap_features & IFF_VNET_HDR) { - ifr.ifr_flags |= IFF_VNET_HDR; - } else { - PMD_DRV_LOG(ERR, "TAP does not support IFF_VNET_HDR"); - goto error; - } - - if (req_mq) - ifr.ifr_flags |= IFF_MULTI_QUEUE; - - if (*p_ifname) - strncpy(ifr.ifr_name, *p_ifname, IFNAMSIZ - 1); - else - strncpy(ifr.ifr_name, "tap%d", IFNAMSIZ - 1); - if (ioctl(tapfd, TUNSETIFF, (void *)&ifr) == -1) { - PMD_DRV_LOG(ERR, "TUNSETIFF failed: %s", strerror(errno)); - goto error; - } - - tap_name = strdup(ifr.ifr_name); - if (!tap_name) { - PMD_DRV_LOG(ERR, "strdup ifname failed: %s", strerror(errno)); - goto error; - } - - if (fcntl(tapfd, F_SETFL, O_NONBLOCK) < 0) { - PMD_DRV_LOG(ERR, "fcntl tapfd failed: %s", strerror(errno)); - goto error; - } - if (ioctl(tapfd, TUNSETVNETHDRSZ, &hdr_size) < 0) { PMD_DRV_LOG(ERR, "TUNSETVNETHDRSZ failed: %s", strerror(errno)); - goto error; + return -1; } if (ioctl(tapfd, TUNSETSNDBUF, &sndbuf) < 0) { PMD_DRV_LOG(ERR, "TUNSETSNDBUF failed: %s", strerror(errno)); - goto error; + return -1; } ret = vhost_kernel_tap_set_offload(tapfd, features); - if (ret < 0 && ret != -ENOTSUP) - goto error; - - memset(&ifr, 0, sizeof(ifr)); - ifr.ifr_hwaddr.sa_family = ARPHRD_ETHER; - memcpy(ifr.ifr_hwaddr.sa_data, mac, RTE_ETHER_ADDR_LEN); - if (ioctl(tapfd, SIOCSIFHWADDR, (void *)&ifr) == -1) { - PMD_DRV_LOG(ERR, "SIOCSIFHWADDR failed: %s", strerror(errno)); - goto error; - } - - free(*p_ifname); - *p_ifname = tap_name; - - return tapfd; -error: - free(tap_name); - close(tapfd); - return -1; + if (ret == -ENOTSUP) + ret = 0; + return ret; } diff --git a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h index ed03fce21e..726433c48c 100644 --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h @@ -16,18 +16,12 @@ #define TUNSETSNDBUF _IOW('T', 212, int) #define TUNGETVNETHDRSZ _IOR('T', 215, int) #define TUNSETVNETHDRSZ _IOW('T', 216, int) -#define TUNSETQUEUE _IOW('T', 217, int) -#define TUNSETVNETLE _IOW('T', 220, int) -#define TUNSETVNETBE _IOW('T', 222, int) /* TUNSETIFF ifr flags */ #define IFF_TAP 0x0002 #define IFF_NO_PI 0x1000 -#define IFF_ONE_QUEUE 0x2000 #define IFF_VNET_HDR 0x4000 #define IFF_MULTI_QUEUE 0x0100 -#define IFF_ATTACH_QUEUE 0x0200 -#define IFF_DETACH_QUEUE 0x0400 /* Features for GSO (TUNSETOFFLOAD). */ #define TUN_F_CSUM 0x01 /* You can hand me unchecksummed packets. */ @@ -39,10 +33,12 @@ /* Constants */ #define PATH_NET_TUN "/dev/net/tun" -int vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq, - const char *mac, uint64_t features); -int vhost_kernel_tap_set_offload(int fd, uint64_t features); -int vhost_kernel_tap_set_queue(int fd, bool attach); +int vhost_kernel_tap_setup(int tapfd, int hdr_size, uint64_t features); + int tap_support_features(unsigned int *tap_features); +int tap_open(const char *ifname, bool multi_queue); +int tap_get_name(int tapfd, char **ifname); +int tap_get_flags(int tapfd, unsigned int *tap_flags); +int tap_set_mac(int tapfd, uint8_t *mac); #endif