DPDK patches and discussions
 help / color / mirror / Atom feed
From: Maxime Coquelin <maxime.coquelin@redhat.com>
To: dev@dpdk.org, chenbo.xia@intel.com, olivier.matz@6wind.com,
	amorenoz@redhat.com, david.marchand@redhat.com
Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
Subject: [dpdk-dev] [PATCH v4 37/44] net/virtio: make server mode blocking
Date: Tue, 26 Jan 2021 11:16:32 +0100
Message-ID: <20210126101639.250481-38-maxime.coquelin@redhat.com> (raw)
In-Reply-To: <20210126101639.250481-1-maxime.coquelin@redhat.com>

This patch makes the Vhost-user backend server mode
blocking at init, waiting for the client connection.

The goal is to make the driver more reliable, as without
waiting for client connection, the Virtio driver has to
assume the Vhost-user backend will support all the
features it has advertized, which could lead to undefined
behaviour.

For example, without this patch, if the user enables packed
ring Virtio feature but the backend does not support it,
the ring initialized by the driver will not be compatible
with the backend.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
---
 drivers/net/virtio/virtio_user/vhost_user.c   |   9 +-
 .../net/virtio/virtio_user/virtio_user_dev.c  | 121 +++++++-----------
 drivers/net/virtio/virtio_user_ethdev.c       |   5 -
 3 files changed, 55 insertions(+), 80 deletions(-)

diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c
index ba02fdcb65..90fed6fe97 100644
--- a/drivers/net/virtio/virtio_user/vhost_user.c
+++ b/drivers/net/virtio/virtio_user/vhost_user.c
@@ -692,6 +692,14 @@ virtio_user_start_server(struct virtio_user_dev *dev, struct sockaddr_un *un)
 	if (ret < 0)
 		return -1;
 
+	PMD_DRV_LOG(NOTICE, "(%s) waiting for client connection...", dev->path);
+	dev->vhostfd = accept(fd, NULL, NULL);
+	if (dev->vhostfd < 0) {
+		PMD_DRV_LOG(ERR, "Failed to accept initial client connection (%s)",
+				strerror(errno));
+		return -1;
+	}
+
 	flag = fcntl(fd, F_GETFL);
 	if (fcntl(fd, F_SETFL, flag | O_NONBLOCK) < 0) {
 		PMD_DRV_LOG(ERR, "fcntl failed, %s", strerror(errno));
@@ -736,7 +744,6 @@ vhost_user_setup(struct virtio_user_dev *dev)
 			close(fd);
 			return -1;
 		}
-		dev->vhostfd = -1;
 	} else {
 		if (connect(fd, (struct sockaddr *)&un, sizeof(un)) < 0) {
 			PMD_DRV_LOG(ERR, "connect error, %s", strerror(errno));
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 95204ea955..c2a41fe3a0 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -144,10 +144,6 @@ virtio_user_dev_set_features(struct virtio_user_dev *dev)
 
 	pthread_mutex_lock(&dev->mutex);
 
-	if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER &&
-			dev->vhostfd < 0)
-		goto error;
-
 	/* Step 0: tell vhost to create queues */
 	if (virtio_user_queue_setup(dev, virtio_user_create_queue) < 0)
 		goto error;
@@ -190,11 +186,6 @@ virtio_user_start_device(struct virtio_user_dev *dev)
 	rte_mcfg_mem_read_lock();
 	pthread_mutex_lock(&dev->mutex);
 
-	/* Vhost-user client not connected yet, will start later */
-	if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER &&
-			dev->vhostfd < 0)
-		goto out;
-
 	/* Step 2: share memory regions */
 	ret = dev->ops->set_memory_table(dev);
 	if (ret < 0)
@@ -213,7 +204,7 @@ virtio_user_start_device(struct virtio_user_dev *dev)
 		goto error;
 
 	dev->started = true;
-out:
+
 	pthread_mutex_unlock(&dev->mutex);
 	rte_mcfg_mem_read_unlock();
 
@@ -422,36 +413,36 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
 			PMD_DRV_LOG(ERR, "Server mode only supports vhost-user!");
 			return -1;
 		}
+	}
+
+	if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) {
 		dev->ops = &virtio_ops_user;
-	} else {
-		if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) {
-			dev->ops = &virtio_ops_user;
-		} else if (dev->backend_type ==
-					VIRTIO_USER_BACKEND_VHOST_KERNEL) {
-			dev->ops = &virtio_ops_kernel;
-
-			dev->vhostfds = malloc(dev->max_queue_pairs *
-					       sizeof(int));
-			dev->tapfds = malloc(dev->max_queue_pairs *
-					     sizeof(int));
-			if (!dev->vhostfds || !dev->tapfds) {
-				PMD_INIT_LOG(ERR, "(%s) Failed to allocate FDs", dev->path);
-				return -1;
-			}
-
-			for (q = 0; q < dev->max_queue_pairs; ++q) {
-				dev->vhostfds[q] = -1;
-				dev->tapfds[q] = -1;
-			}
-		} else if (dev->backend_type ==
-				VIRTIO_USER_BACKEND_VHOST_VDPA) {
-			dev->ops = &virtio_ops_vdpa;
-		} else {
-			PMD_DRV_LOG(ERR, "(%s) Unknown backend type", dev->path);
+	} else if (dev->backend_type ==
+			VIRTIO_USER_BACKEND_VHOST_KERNEL) {
+		dev->ops = &virtio_ops_kernel;
+
+		dev->vhostfds = malloc(dev->max_queue_pairs *
+				sizeof(int));
+		dev->tapfds = malloc(dev->max_queue_pairs *
+				sizeof(int));
+		if (!dev->vhostfds || !dev->tapfds) {
+			PMD_INIT_LOG(ERR, "(%s) Failed to allocate FDs", dev->path);
 			return -1;
 		}
+
+		for (q = 0; q < dev->max_queue_pairs; ++q) {
+			dev->vhostfds[q] = -1;
+			dev->tapfds[q] = -1;
+		}
+	} else if (dev->backend_type ==
+			VIRTIO_USER_BACKEND_VHOST_VDPA) {
+		dev->ops = &virtio_ops_vdpa;
+	} else {
+		PMD_DRV_LOG(ERR, "(%s) Unknown backend type", dev->path);
+		return -1;
 	}
 
+
 	if (dev->ops->setup(dev) < 0) {
 		PMD_INIT_LOG(ERR, "(%s) Failed to setup backend\n", dev->path);
 		return -1;
@@ -541,54 +532,36 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 		dev->unsupported_features |=
 			(1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
 
-	if (!dev->is_server) {
-		if (dev->ops->set_owner(dev) < 0) {
-			PMD_INIT_LOG(ERR, "(%s) Failed to set backend owner", dev->path);
-			return -1;
-		}
+	if (dev->ops->set_owner(dev) < 0) {
+		PMD_INIT_LOG(ERR, "(%s) Failed to set backend owner", dev->path);
+		return -1;
+	}
+
+	if (dev->ops->get_features(dev, &dev->device_features) < 0) {
+		PMD_INIT_LOG(ERR, "(%s) Failed to get backend features", dev->path);
+		return -1;
+	}
 
-		if (dev->ops->get_features(dev, &dev->device_features) < 0) {
-			PMD_INIT_LOG(ERR, "(%s) Failed to get backend features", dev->path);
+	if ((dev->device_features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) ||
+			dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA) {
+		if (dev->ops->get_protocol_features(dev, &protocol_features)) {
+			PMD_INIT_LOG(ERR, "(%s) Failed to get backend protocol features",
+					dev->path);
 			return -1;
 		}
 
+		dev->protocol_features &= protocol_features;
 
-		if ((dev->device_features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) ||
-				(dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA)) {
-			if (dev->ops->get_protocol_features(dev, &protocol_features)) {
-				PMD_INIT_LOG(ERR, "(%s) Failed to get backend protocol features",
-						dev->path);
-				return -1;
-			}
-
-			dev->protocol_features &= protocol_features;
-
-			if (dev->ops->set_protocol_features(dev, dev->protocol_features)) {
-				PMD_INIT_LOG(ERR, "(%s) Failed to set backend protocol features",
-						dev->path);
-				return -1;
-			}
-
-			if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_MQ)))
-				dev->unsupported_features |= (1ull << VIRTIO_NET_F_MQ);
+		if (dev->ops->set_protocol_features(dev, dev->protocol_features)) {
+			PMD_INIT_LOG(ERR, "(%s) Failed to set backend protocol features",
+					dev->path);
+			return -1;
 		}
-	} else {
-		/* We just pretend vhost-user can support all these features.
-		 * Note that this could be problematic that if some feature is
-		 * negotiated but not supported by the vhost-user which comes
-		 * later.
-		 */
-		dev->device_features = VIRTIO_USER_SUPPORTED_FEATURES;
 
-		/* We cannot assume VHOST_USER_PROTOCOL_F_STATUS is supported
-		 * until it's negotiated
-		 */
-		dev->protocol_features &=
-			~(1ULL << VHOST_USER_PROTOCOL_F_STATUS);
+		if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_MQ)))
+			dev->unsupported_features |= (1ull << VIRTIO_NET_F_MQ);
 	}
 
-
-
 	if (!mrg_rxbuf)
 		dev->unsupported_features |= (1ull << VIRTIO_NET_F_MRG_RXBUF);
 
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 5e256f4e6d..e822d93690 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -174,11 +174,6 @@ virtio_user_delayed_handler(void *param)
 		if (dev->vhostfd >= 0) {
 			close(dev->vhostfd);
 			dev->vhostfd = -1;
-			/* Until the featuers are negotiated again, don't assume
-			 * the backend supports VHOST_USER_PROTOCOL_F_STATUS
-			 */
-			dev->protocol_features &=
-				~(1ULL << VHOST_USER_PROTOCOL_F_STATUS);
 		}
 		eth_dev->intr_handle->fd = dev->listenfd;
 		rte_intr_callback_register(eth_dev->intr_handle,
-- 
2.29.2


  parent reply	other threads:[~2021-01-26 10:23 UTC|newest]

Thread overview: 62+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-26 10:15 [dpdk-dev] [PATCH v4 00/44] net/virtio: Virtio PMD rework Maxime Coquelin
2021-01-26 10:15 ` [dpdk-dev] [PATCH v4 01/44] bus/vdev: add helper to get vdev from ethdev Maxime Coquelin
2021-01-26 10:15 ` [dpdk-dev] [PATCH v4 02/44] bus/vdev: add driver IOVA VA mode requirement Maxime Coquelin
2021-01-26 11:50   ` Xia, Chenbo
2021-01-26 12:50   ` David Marchand
2021-01-26 13:23     ` Kinsella, Ray
2021-01-26 14:40       ` David Marchand
2021-01-26 15:28         ` Kinsella, Ray
2021-01-27  8:23   ` David Marchand
2021-01-27  8:25     ` Maxime Coquelin
2021-01-26 10:15 ` [dpdk-dev] [PATCH v4 03/44] net/virtio: fix getting old status on reconnect Maxime Coquelin
2021-01-26 10:15 ` [dpdk-dev] [PATCH v4 04/44] net/virtio: introduce Virtio bus type Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 05/44] net/virtio: refactor virtio-user device Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 06/44] net/virtio: introduce PCI device metadata Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 07/44] net/virtio: move PCI device init in dedicated file Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 08/44] net/virtio: move PCI specific dev init to PCI ethdev init Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 09/44] net/virtio: move MSIX detection to PCI ethdev Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 10/44] net/virtio: force IOVA as VA mode for Virtio-user Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 11/44] net/virtio: store PCI type in Virtio device metadata Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 12/44] net/virtio: add callback for device closing Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 13/44] net/virtio: validate features at bus level Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 14/44] net/virtio: remove bus type enum Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 15/44] net/virtio: move PCI-specific fields to PCI device Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 16/44] net/virtio: pack virtio HW struct Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 17/44] net/virtio: move legacy IO to Virtio PCI Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 18/44] net/virtio: introduce generic virtio header Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 19/44] net/virtio: move features definition to generic header Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 20/44] net/virtio: move virtqueue defines in " Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 21/44] net/virtio: move config definitions to " Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 22/44] net/virtio: make interrupt handling more generic Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 23/44] net/virtio: move vring alignment to generic header Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 24/44] net/virtio: remove last PCI refs in non-PCI code Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 25/44] net/virtio: make Vhost-user request sender consistent Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 26/44] net/virtio: add Virtio-user ops to set owner Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 27/44] net/virtio: add Virtio-user features ops Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 28/44] net/virtio: add Virtio-user protocol " Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 29/44] net/virtio: add Virtio-user memory tables ops Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 30/44] net/virtio: add Virtio-user vring setting ops Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 31/44] net/virtio: add Virtio-user vring file ops Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 32/44] net/virtio: add Virtio-user vring address ops Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 33/44] net/virtio: add Virtio-user status ops Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 34/44] net/virtio: remove useless request ops Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 35/44] net/virtio: improve Virtio-user errors handling Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 36/44] net/virtio: move Vhost-user requests to Vhost-user backend Maxime Coquelin
2021-01-26 10:16 ` Maxime Coquelin [this message]
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 38/44] net/virtio: move protocol features to Vhost-user Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 39/44] net/virtio: introduce backend data Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 40/44] net/virtio: move Vhost-user specifics to its backend Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 41/44] net/virtio: move Vhost-kernel data " Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 42/44] net/virtio: move Vhost-vDPA " Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 43/44] net/virtio: improve Vhost-user error logging Maxime Coquelin
2021-01-26 10:16 ` [dpdk-dev] [PATCH v4 44/44] net/virtio: handle Virtio-user setup failure properly Maxime Coquelin
2021-01-26 12:02   ` Xia, Chenbo
2021-01-26 12:59     ` Maxime Coquelin
2021-01-27 11:59 ` [dpdk-dev] [PATCH v4 00/44] net/virtio: Virtio PMD rework Maxime Coquelin
2021-02-01  8:44 ` Wang, Yinan
2021-02-01  8:49   ` Maxime Coquelin
2021-02-01 13:00 ` Ilya Maximets
2021-02-01 13:03   ` Ilya Maximets
2021-02-01 13:16     ` Maxime Coquelin
2021-02-01 13:42       ` Ilya Maximets
2021-02-01 13:51         ` Maxime Coquelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210126101639.250481-38-maxime.coquelin@redhat.com \
    --to=maxime.coquelin@redhat.com \
    --cc=amorenoz@redhat.com \
    --cc=chenbo.xia@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=olivier.matz@6wind.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git