Do not close the tap fds when disabling queue pairs, instead, we just need to unbind the backend. Otherwise, tap port can be destroyed unexpectedly. Fixes: e3b434818bbb ("net/virtio-user: support kernel vhost") Cc: stable@dpdk.org Reported-by: Stephen Hemminger <stephen@networkplumber.org> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com> --- drivers/net/virtio/virtio_user/vhost_kernel.c | 34 +++++++++++--- .../net/virtio/virtio_user/vhost_kernel_tap.c | 44 +++++++++++++------ .../net/virtio/virtio_user/vhost_kernel_tap.h | 3 ++ drivers/net/virtio/virtio_user/vhost_user.c | 4 ++ .../net/virtio/virtio_user/virtio_user_dev.c | 5 ++- .../net/virtio/virtio_user/virtio_user_dev.h | 1 + 6 files changed, 70 insertions(+), 21 deletions(-) diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c b/drivers/net/virtio/virtio_user/vhost_kernel.c index 5c81e8dd9..d48d284d4 100644 --- a/drivers/net/virtio/virtio_user/vhost_kernel.c +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c @@ -330,16 +330,34 @@ vhost_kernel_enable_queue_pair(struct virtio_user_dev *dev, vhostfd = dev->vhostfds[pair_idx]; + if (dev->qp_enabled[pair_idx] == enable) + return 0; + if (!enable) { - if (dev->tapfds[pair_idx] >= 0) { - close(dev->tapfds[pair_idx]); - dev->tapfds[pair_idx] = -1; + tapfd = dev->tapfds[pair_idx]; + if (vhost_kernel_set_backend(vhostfd, -1) < 0) { + PMD_DRV_LOG(ERR, "fail to set backend for vhost kernel"); + return -1; } - return vhost_kernel_set_backend(vhostfd, -1); - } else if (dev->tapfds[pair_idx] >= 0) { + if (req_mq && vhost_kernel_tap_detach_queue(tapfd) < 0) { + PMD_DRV_LOG(ERR, "fail to disable tap for vhost kernel"); + return -1; + } + dev->qp_enabled[pair_idx] = false; return 0; } + if (dev->tapfds[pair_idx] >= 0) { + tapfd = dev->tapfds[pair_idx]; + if (vhost_kernel_tap_set_offload(tapfd, dev->features) == -1) + return -1; + if (req_mq && vhost_kernel_tap_attach_queue(tapfd) < 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); @@ -353,13 +371,15 @@ vhost_kernel_enable_queue_pair(struct virtio_user_dev *dev, return -1; } + dev->tapfds[pair_idx] = tapfd; + +set_backend: if (vhost_kernel_set_backend(vhostfd, tapfd) < 0) { PMD_DRV_LOG(ERR, "fail to set backend for vhost kernel"); - close(tapfd); return -1; } - dev->tapfds[pair_idx] = tapfd; + dev->qp_enabled[pair_idx] = true; return 0; } diff --git a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c index 76bf75423..529f81133 100644 --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c @@ -18,7 +18,27 @@ #include "../virtio_logs.h" #include "../virtio_pci.h" -static int +int +vhost_kernel_tap_attach_queue(int fd) +{ + struct ifreq ifr; + + memset(&ifr, 0, sizeof(ifr)); + ifr.ifr_flags = IFF_ATTACH_QUEUE; + return ioctl(fd, TUNSETQUEUE, (void *)&ifr); +} + +int +vhost_kernel_tap_detach_queue(int fd) +{ + struct ifreq ifr; + + memset(&ifr, 0, sizeof(ifr)); + ifr.ifr_flags = IFF_DETACH_QUEUE; + return ioctl(fd, TUNSETQUEUE, (void *)&ifr); +} + +int vhost_kernel_tap_set_offload(int fd, uint64_t features) { unsigned int offload = 0; @@ -37,20 +57,18 @@ vhost_kernel_tap_set_offload(int fd, uint64_t features) offload |= TUN_F_UFO; } - 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; - } + /* 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) { - offload &= ~TUN_F_UFO; - if (ioctl(fd, TUNSETOFFLOAD, offload) != 0) { - PMD_DRV_LOG(ERR, "TUNSETOFFLOAD ioctl() failed: %s\n", - strerror(errno)); - return -1; - } + PMD_DRV_LOG(ERR, "TUNSETOFFLOAD ioctl() failed: %s\n", + strerror(errno)); + return -1; } } diff --git a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h index e0e95b4f5..1202932f7 100644 --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h @@ -37,3 +37,6 @@ int vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq, const char *mac, uint64_t features); +int vhost_kernel_tap_attach_queue(int fd); +int vhost_kernel_tap_detach_queue(int fd); +int vhost_kernel_tap_set_offload(int fd, uint64_t features); diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c index a4b5c25cd..d8e083ba8 100644 --- a/drivers/net/virtio/virtio_user/vhost_user.c +++ b/drivers/net/virtio/virtio_user/vhost_user.c @@ -456,6 +456,9 @@ vhost_user_enable_queue_pair(struct virtio_user_dev *dev, { int i; + if (dev->qp_enabled[pair_idx] == enable) + return 0; + for (i = 0; i < 2; ++i) { struct vhost_vring_state state = { .index = pair_idx * 2 + i, @@ -466,6 +469,7 @@ vhost_user_enable_queue_pair(struct virtio_user_dev *dev, return -1; } + dev->qp_enabled[pair_idx] = enable; return 0; } diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c index a4400e772..177e829c9 100644 --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c @@ -545,8 +545,11 @@ virtio_user_dev_uninit(struct virtio_user_dev *dev) } if (dev->vhostfds) { - for (i = 0; i < dev->max_queue_pairs; ++i) + for (i = 0; i < dev->max_queue_pairs; ++i) { close(dev->vhostfds[i]); + if (dev->tapfds[i] >= 0) + close(dev->tapfds[i]); + } free(dev->vhostfds); free(dev->tapfds); } diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h index ad8683771..3b6b6065a 100644 --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h @@ -49,6 +49,7 @@ struct virtio_user_dev { struct vring_packed packed_vrings[VIRTIO_MAX_VIRTQUEUES]; }; struct virtio_user_queue packed_queues[VIRTIO_MAX_VIRTQUEUES]; + bool qp_enabled[VIRTIO_MAX_VIRTQUEUE_PAIRS]; struct virtio_user_backend_ops *ops; pthread_mutex_t mutex; -- 2.23.0
On Tue, 19 Nov 2019 14:29:48 +0800
Tiwei Bie <tiwei.bie@intel.com> wrote:
> -static int
> +int
> +vhost_kernel_tap_attach_queue(int fd)
> +{
> + struct ifreq ifr;
> +
> + memset(&ifr, 0, sizeof(ifr));
> + ifr.ifr_flags = IFF_ATTACH_QUEUE;
> + return ioctl(fd, TUNSETQUEUE, (void *)&ifr);
> +}
> +
> +int
> +vhost_kernel_tap_detach_queue(int fd)
> +{
> + struct ifreq ifr;
> +
> + memset(&ifr, 0, sizeof(ifr));
> + ifr.ifr_flags = IFF_DETACH_QUEUE;
> + return ioctl(fd, TUNSETQUEUE, (void *)&ifr);
> +}
> +
These both could be squashed into one routine.
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);
}
1. Use initializer instead of memset
2. Cast to void * is unnecessary
On Tue, Nov 19, 2019 at 08:36:38AM -0800, Stephen Hemminger wrote:
> On Tue, 19 Nov 2019 14:29:48 +0800
> Tiwei Bie <tiwei.bie@intel.com> wrote:
>
> > -static int
> > +int
> > +vhost_kernel_tap_attach_queue(int fd)
> > +{
> > + struct ifreq ifr;
> > +
> > + memset(&ifr, 0, sizeof(ifr));
> > + ifr.ifr_flags = IFF_ATTACH_QUEUE;
> > + return ioctl(fd, TUNSETQUEUE, (void *)&ifr);
> > +}
> > +
> > +int
> > +vhost_kernel_tap_detach_queue(int fd)
> > +{
> > + struct ifreq ifr;
> > +
> > + memset(&ifr, 0, sizeof(ifr));
> > + ifr.ifr_flags = IFF_DETACH_QUEUE;
> > + return ioctl(fd, TUNSETQUEUE, (void *)&ifr);
> > +}
> > +
>
> These both could be squashed into one routine.
>
> 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);
> }
>
> 1. Use initializer instead of memset
> 2. Cast to void * is unnecessary
Thanks! Will do it in v2.
Regards,
Tiwei
Do not close the tap fds when disabling queue pairs, instead, we just need to unbind the backend. Otherwise, tap port can be destroyed unexpectedly. Fixes: e3b434818bbb ("net/virtio-user: support kernel vhost") Cc: stable@dpdk.org Reported-by: Stephen Hemminger <stephen@networkplumber.org> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com> --- v2: - Squash attach_queue/detach_queue into one routine (Stephen); - Use initializer instead of memset (Stephen); - Drop the cast to void * (Stephen); drivers/net/virtio/virtio_user/vhost_kernel.c | 34 +++++++++++++++---- .../net/virtio/virtio_user/vhost_kernel_tap.c | 34 ++++++++++++------- .../net/virtio/virtio_user/vhost_kernel_tap.h | 8 +++++ drivers/net/virtio/virtio_user/vhost_user.c | 4 +++ .../net/virtio/virtio_user/virtio_user_dev.c | 5 ++- .../net/virtio/virtio_user/virtio_user_dev.h | 1 + 6 files changed, 65 insertions(+), 21 deletions(-) diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c b/drivers/net/virtio/virtio_user/vhost_kernel.c index 5c81e8dd9..2c805077a 100644 --- a/drivers/net/virtio/virtio_user/vhost_kernel.c +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c @@ -330,16 +330,34 @@ vhost_kernel_enable_queue_pair(struct virtio_user_dev *dev, vhostfd = dev->vhostfds[pair_idx]; + if (dev->qp_enabled[pair_idx] == enable) + return 0; + if (!enable) { - if (dev->tapfds[pair_idx] >= 0) { - close(dev->tapfds[pair_idx]); - dev->tapfds[pair_idx] = -1; + tapfd = dev->tapfds[pair_idx]; + if (vhost_kernel_set_backend(vhostfd, -1) < 0) { + PMD_DRV_LOG(ERR, "fail to set backend for vhost kernel"); + return -1; } - return vhost_kernel_set_backend(vhostfd, -1); - } else if (dev->tapfds[pair_idx] >= 0) { + 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 (dev->tapfds[pair_idx] >= 0) { + tapfd = dev->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); @@ -353,13 +371,15 @@ vhost_kernel_enable_queue_pair(struct virtio_user_dev *dev, return -1; } + dev->tapfds[pair_idx] = tapfd; + +set_backend: if (vhost_kernel_set_backend(vhostfd, tapfd) < 0) { PMD_DRV_LOG(ERR, "fail to set backend for vhost kernel"); - close(tapfd); return -1; } - dev->tapfds[pair_idx] = tapfd; + dev->qp_enabled[pair_idx] = true; return 0; } diff --git a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c index 76bf75423..2fbfecba1 100644 --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c @@ -18,7 +18,7 @@ #include "../virtio_logs.h" #include "../virtio_pci.h" -static int +int vhost_kernel_tap_set_offload(int fd, uint64_t features) { unsigned int offload = 0; @@ -37,26 +37,34 @@ vhost_kernel_tap_set_offload(int fd, uint64_t features) offload |= TUN_F_UFO; } - 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; - } + /* 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) { - offload &= ~TUN_F_UFO; - if (ioctl(fd, TUNSETOFFLOAD, offload) != 0) { - PMD_DRV_LOG(ERR, "TUNSETOFFLOAD ioctl() failed: %s\n", - strerror(errno)); - return -1; - } + PMD_DRV_LOG(ERR, "TUNSETOFFLOAD ioctl() failed: %s\n", + strerror(errno)); + return -1; } } return 0; } +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) diff --git a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h index e0e95b4f5..5c4447b29 100644 --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h @@ -2,6 +2,10 @@ * Copyright(c) 2016 Intel Corporation */ +#ifndef _VHOST_KERNEL_TAP_H +#define _VHOST_KERNEL_TAP_H + +#include <stdbool.h> #include <sys/ioctl.h> /* TUN ioctls */ @@ -37,3 +41,7 @@ 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); + +#endif diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c index a4b5c25cd..d8e083ba8 100644 --- a/drivers/net/virtio/virtio_user/vhost_user.c +++ b/drivers/net/virtio/virtio_user/vhost_user.c @@ -456,6 +456,9 @@ vhost_user_enable_queue_pair(struct virtio_user_dev *dev, { int i; + if (dev->qp_enabled[pair_idx] == enable) + return 0; + for (i = 0; i < 2; ++i) { struct vhost_vring_state state = { .index = pair_idx * 2 + i, @@ -466,6 +469,7 @@ vhost_user_enable_queue_pair(struct virtio_user_dev *dev, return -1; } + dev->qp_enabled[pair_idx] = enable; return 0; } diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c index a4400e772..177e829c9 100644 --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c @@ -545,8 +545,11 @@ virtio_user_dev_uninit(struct virtio_user_dev *dev) } if (dev->vhostfds) { - for (i = 0; i < dev->max_queue_pairs; ++i) + for (i = 0; i < dev->max_queue_pairs; ++i) { close(dev->vhostfds[i]); + if (dev->tapfds[i] >= 0) + close(dev->tapfds[i]); + } free(dev->vhostfds); free(dev->tapfds); } diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h index ad8683771..3b6b6065a 100644 --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h @@ -49,6 +49,7 @@ struct virtio_user_dev { struct vring_packed packed_vrings[VIRTIO_MAX_VIRTQUEUES]; }; struct virtio_user_queue packed_queues[VIRTIO_MAX_VIRTQUEUES]; + bool qp_enabled[VIRTIO_MAX_VIRTQUEUE_PAIRS]; struct virtio_user_backend_ops *ops; pthread_mutex_t mutex; -- 2.24.0
On 11/25/19 9:14 AM, Tiwei Bie wrote:
> Do not close the tap fds when disabling queue pairs, instead,
> we just need to unbind the backend. Otherwise, tap port can be
> destroyed unexpectedly.
>
> Fixes: e3b434818bbb ("net/virtio-user: support kernel vhost")
> Cc: stable@dpdk.org
>
> Reported-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
> v2:
> - Squash attach_queue/detach_queue into one routine (Stephen);
> - Use initializer instead of memset (Stephen);
> - Drop the cast to void * (Stephen);
>
> drivers/net/virtio/virtio_user/vhost_kernel.c | 34 +++++++++++++++----
> .../net/virtio/virtio_user/vhost_kernel_tap.c | 34 ++++++++++++-------
> .../net/virtio/virtio_user/vhost_kernel_tap.h | 8 +++++
> drivers/net/virtio/virtio_user/vhost_user.c | 4 +++
> .../net/virtio/virtio_user/virtio_user_dev.c | 5 ++-
> .../net/virtio/virtio_user/virtio_user_dev.h | 1 +
> 6 files changed, 65 insertions(+), 21 deletions(-)
>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
On 11/25/19 9:14 AM, Tiwei Bie wrote:
> Do not close the tap fds when disabling queue pairs, instead,
> we just need to unbind the backend. Otherwise, tap port can be
> destroyed unexpectedly.
>
> Fixes: e3b434818bbb ("net/virtio-user: support kernel vhost")
> Cc: stable@dpdk.org
>
> Reported-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
> v2:
> - Squash attach_queue/detach_queue into one routine (Stephen);
> - Use initializer instead of memset (Stephen);
> - Drop the cast to void * (Stephen);
>
> drivers/net/virtio/virtio_user/vhost_kernel.c | 34 +++++++++++++++----
> .../net/virtio/virtio_user/vhost_kernel_tap.c | 34 ++++++++++++-------
> .../net/virtio/virtio_user/vhost_kernel_tap.h | 8 +++++
> drivers/net/virtio/virtio_user/vhost_user.c | 4 +++
> .../net/virtio/virtio_user/virtio_user_dev.c | 5 ++-
> .../net/virtio/virtio_user/virtio_user_dev.h | 1 +
> 6 files changed, 65 insertions(+), 21 deletions(-)
Applied to dpdk-next-virtio/master
Thanks,
Maxime