On 1/11/21 9:02 AM, Xia, Chenbo wrote:

Hi Maxime,

quoted

-----Original Message-----
From: Maxime Coquelin <redacted>
Sent: Monday, December 21, 2020 5:14 AM
To: dev@dpdk.org; Xia, Chenbo <redacted>; olivier.matz@6wind.com;
amorenoz@redhat.com; david.marchand@redhat.com
Cc: Maxime Coquelin <redacted>
Subject: [PATCH 39/40] net/virtio: move Vhost-kernel data to its backend

As done earlier for Vhost-user, this patch moves the
Vhost-Kernel specific data to its backend file.

Signed-off-by: Maxime Coquelin <redacted>
---
 drivers/net/virtio/virtio_user/vhost_kernel.c | 78 +++++++++++++++----
 .../net/virtio/virtio_user/virtio_user_dev.c  | 43 ++--------
 .../net/virtio/virtio_user/virtio_user_dev.h  |  7 +-
 3 files changed, 72 insertions(+), 56 deletions(-)

<snip>

quoted

 vhost_kernel_setup(struct virtio_user_dev *dev)
 {
 	int vhostfd;
-	uint32_t i;
+	uint32_t q, i;
+	struct vhost_kernel_data *data;
+
+	data = malloc(sizeof(*data));
+	if (!data) {
+		PMD_INIT_LOG(ERR, "(%s) Failed to allocate Vhost-kernel data",
dev->path);
+		return -1;
+	}
+
+	data->vhostfds = malloc(dev->max_queue_pairs * sizeof(int));
+	data->tapfds = malloc(dev->max_queue_pairs * sizeof(int));
+	if (!data->vhostfds || !data->tapfds) {
+		PMD_INIT_LOG(ERR, "(%s) Failed to allocate FDs", dev->path);
+		return -1;
+	}
+
+	for (q = 0; q < dev->max_queue_pairs; ++q) {
+		data->vhostfds[q] = -1;
+		data->tapfds[q] = -1;
+	}

Forget to do 'dev->backend_data = data'? 😊

Indeed! As I mentionned in the cover letter, I didn't test last 5
patches... and we can see it confirmed right here :)

I have reworked the fucntion to handle error properly (free allocated
resources, close opened FDs...)

Besides,

When I do test 'virtio_user for exceptional path', iperf just cannot normally
send/recv pkts. Testpmd have some pkts (<10), guess tcp connection cannot be
established. I have not tried to locate the root cause. Just thinking it's
better to tell you first.

Thanks for the heads-up, I will look into it before posting v2.

Maxime

Thanks!
Chenbo

quoted

 	get_vhost_kernel_max_regions();

 	for (i = 0; i < dev->max_queue_pairs; ++i) {
 		vhostfd = open(dev->path, O_RDWR);
 		if (vhostfd < 0) {
-			PMD_DRV_LOG(ERR, "fail to open %s, %s",
-				    dev->path, strerror(errno));
+			PMD_DRV_LOG(ERR, "fail to open %s, %s", dev->path,
strerror(errno));
 			return -1;
 		}

-		dev->vhostfds[i] = vhostfd;
+		data->vhostfds[i] = vhostfd;
 	}

 	return 0;

@@ -360,6 +392,19 @@ vhost_kernel_setup(struct virtio_user_dev *dev)
 static int
 vhost_kernel_destroy(struct virtio_user_dev *dev)
 {
+	struct vhost_kernel_data *data = dev->backend_data;
+	uint32_t i;
+
+	for (i = 0; i < dev->max_queue_pairs; ++i) {
+		close(data->vhostfds[i]);
+		if (data->tapfds[i] >= 0)
+			close(data->tapfds[i]);
+	}
+
+	free(data->vhostfds);
+	free(data->tapfds);
+	free(data);
+
 	return 0;
 }

@@ -395,14 +440,15 @@ vhost_kernel_enable_queue_pair(struct virtio_user_dev

*dev,
 	int vhostfd;
 	int tapfd;
 	int req_mq = (dev->max_queue_pairs > 1);
+	struct vhost_kernel_data *data = dev->backend_data;

-	vhostfd = dev->vhostfds[pair_idx];
+	vhostfd = data->vhostfds[pair_idx];

 	if (dev->qp_enabled[pair_idx] == enable)
 		return 0;

 	if (!enable) {
-		tapfd = dev->tapfds[pair_idx];
+		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;

@@ -415,8 +461,8 @@ vhost_kernel_enable_queue_pair(struct virtio_user_dev *dev,
 		return 0;
 	}

-	if (dev->tapfds[pair_idx] >= 0) {
-		tapfd = dev->tapfds[pair_idx];
+	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) {

@@ -439,7 +485,7 @@ vhost_kernel_enable_queue_pair(struct virtio_user_dev *dev,
 		return -1;
 	}

-	dev->tapfds[pair_idx] = tapfd;
+	data->tapfds[pair_idx] = tapfd;

 set_backend:
 	if (vhost_kernel_set_backend(vhostfd, tapfd) < 0) {

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c

b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 93a9ce2cd2..154aecc209 100644

--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c

@@ -397,11 +397,6 @@ virtio_user_mem_event_cb(enum rte_mem_event type

__rte_unused,
 static int
 virtio_user_dev_setup(struct virtio_user_dev *dev)
 {
-	uint32_t q;
-
-	dev->vhostfds = NULL;
-	dev->tapfds = NULL;
-
 	if (dev->is_server) {
 		if (dev->backend_type != VIRTIO_USER_BACKEND_VHOST_USER) {
 			PMD_DRV_LOG(ERR, "Server mode only supports vhost-user!");

@@ -409,34 +404,21 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
 		}
 	}

-	if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) {
+	switch (dev->backend_type) {
+	case VIRTIO_USER_BACKEND_VHOST_USER:
 		dev->ops = &virtio_ops_user;
-	} else if (dev->backend_type ==
-			VIRTIO_USER_BACKEND_VHOST_KERNEL) {
+		break;
+	case 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) {
+		break;
+	case VIRTIO_USER_BACKEND_VHOST_VDPA:
 		dev->ops = &virtio_ops_vdpa;
-	} else {
+		break;
+	default:
 		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;

@@ -592,15 +574,6 @@ virtio_user_dev_uninit(struct virtio_user_dev *dev)
 		close(dev->callfds[i]);
 		close(dev->kickfds[i]);
 	}
-	if (dev->vhostfds) {
-		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);
-	}

 	free(dev->ifname);

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h

b/drivers/net/virtio/virtio_user/virtio_user_dev.h
index a429dcc57c..5a2c9d38dd 100644

--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h

@@ -31,11 +31,6 @@ struct virtio_user_dev {
 	/* for vhost_vdpa backend */
 	int		vhostfd;

-	/* for vhost_kernel backend */
-	char		*ifname;
-	int		*vhostfds;
-	int		*tapfds;
-
 	/* for both vhost_user and vhost_kernel */
 	int		callfds[VIRTIO_MAX_VIRTQUEUES];
 	int		kickfds[VIRTIO_MAX_VIRTQUEUES];

@@ -55,6 +50,8 @@ struct virtio_user_dev {
 	uint16_t	port_id;
 	uint8_t		mac_addr[RTE_ETHER_ADDR_LEN];
 	char		path[PATH_MAX];
+	char		*ifname;
+
 	union {
 		struct vring		vrings[VIRTIO_MAX_VIRTQUEUES];
 		struct vring_packed	packed_vrings[VIRTIO_MAX_VIRTQUEUES];
--

2.29.2