net/tap: Bug fix to populate fds in secondary process

PATCH Superseded Kumara Parameshwaran 3 participants 19 messages Jan 21, 2022 – Feb 1, 2022 Network drivers

lore

Share & tools

lore

Kumara Parameshwaran

author

2022-01-21 04:29 UTC #0 lore raw

PATCH 1/1 net/tap: Bug fix to populate fds in secondary process

From: Kumara Parameshwaran <[email protected]>

When a tap device is hotplugged to primary process which in turn adds the device to all secondary process, the secondary process does a tap_mp_attach_queues, but the fds are not populated in the primary during the probe they are populated during the queue_setup, added a fix to sync the queues during rte_eth_dev_start

Signed-off-by Kumara Parameshwaran <[email protected]>

---
 drivers/net/tap/rte_eth_tap.c | 196 +++++++++++++---------------------
 lib/ethdev/rte_ethdev.c       |  11 ++
 lib/ethdev/rte_ethdev.h       |  17 +++
 lib/ethdev/version.map        |   2 +
 4 files changed, 102 insertions(+), 124 deletions(-)

drivers/net/tap/rte_eth_tap.c +72 ~~-124~~

index f1b48cae82..f6c25d7e21 100644
@@ -66,7 +66,7 @@
 	(TAP_GSO_MBUFS_PER_CORE * TAP_GSO_MBUF_CACHE_SIZE)
 
 /* IPC key for queue fds sync */
-#define TAP_MP_KEY "tap_mp_sync_queues"
+#define TAP_MP_REQ_START_RXTX "tap_mp_req_start_rxtx"
 
 #define TAP_IOV_DEFAULT_MAX 1024

@@ -880,11 +880,48 @@ tap_link_set_up(struct rte_eth_dev *dev)
 	return tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
 }
 
+static int tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
+{
+	struct rte_mp_msg msg;
+	struct ipc_queues *request_param = (struct ipc_queues *)msg.param;
+	int err;
+	int fd_iterator = 0;
+	struct pmd_process_private *process_private = dev->process_private;
+	int i;
+
+	memset(&msg, 0, sizeof(msg));
+	strlcpy(msg.name, TAP_MP_REQ_START_RXTX, sizeof(msg.name));
+	strlcpy(request_param->port_name, dev->data->name, sizeof(request_param->port_name));
+	msg.len_param = sizeof(*request_param);
+	for (i = 0; i < dev->data->nb_tx_queues; i++) {
+		msg.fds[fd_iterator++] = process_private->txq_fds[i];
+		msg.num_fds++;
+		request_param->txq_count++;
+	}
+	for (i = 0; i < dev->data->nb_rx_queues; i++) {
+		msg.fds[fd_iterator++] = process_private->rxq_fds[i];
+		msg.num_fds++;
+		request_param->rxq_count++;
+	}
+
+	err = rte_mp_sendmsg(&msg);
+	if (err < 0) {
+		TAP_LOG(ERR, "Failed to send start req to secondary %d",
+			rte_errno);
+		return -1;
+	}
+
+	return 0;
+}
+
 static int
 tap_dev_start(struct rte_eth_dev *dev)
 {
 	int err, i;
 
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+		tap_mp_req_on_rxtx(dev);
+
 	err = tap_intr_handle_set(dev, 1);
 	if (err)
 		return err;

@@ -901,6 +938,34 @@ tap_dev_start(struct rte_eth_dev *dev)
 	return err;
 }
 
+static int
+tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused const void *peer)
+{
+	struct rte_eth_dev *dev;
+	const struct ipc_queues *request_param =
+		(const struct ipc_queues *)request->param;
+	int fd_iterator;
+	int queue;
+	struct pmd_process_private *process_private;
+
+	dev = rte_get_eth_dev_by_name(request_param->port_name);
+	if (!dev) {
+		TAP_LOG(ERR, "Failed to get dev for %s",
+			request_param->port_name);
+		return -1;
+	}
+	process_private = dev->process_private;
+	fd_iterator = 0;
+	TAP_LOG(DEBUG, "tap_attach rx_q:%d tx_q:%d\n", request_param->rxq_count,
+		request_param->txq_count);
+	for (queue = 0; queue < request_param->txq_count; queue++)
+		process_private->txq_fds[queue] = request->fds[fd_iterator++];
+	for (queue = 0; queue < request_param->rxq_count; queue++)
+		process_private->rxq_fds[queue] = request->fds[fd_iterator++];
+
+	return 0;
+}
+
 /* This function gets called when the current port gets stopped.
  */
 static int

@@ -1084,6 +1149,7 @@ tap_dev_close(struct rte_eth_dev *dev)
 
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
 		rte_free(dev->process_private);
+		rte_mp_action_unregister(TAP_MP_REQ_START_RXTX);
 		return 0;
 	}

@@ -1140,8 +1206,6 @@ tap_dev_close(struct rte_eth_dev *dev)
 		internals->ioctl_sock = -1;
 	}
 	rte_free(dev->process_private);
-	if (tap_devices_count == 1)
-		rte_mp_action_unregister(TAP_MP_KEY);
 	tap_devices_count--;
 	/*
 	 * Since TUN device has no more opened file descriptors

@@ -2292,113 +2356,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
 	return ret;
 }
 
-/* Request queue file descriptors from secondary to primary. */
-static int
-tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
-{
-	int ret;
-	struct timespec timeout = {.tv_sec = 1, .tv_nsec = 0};
-	struct rte_mp_msg request, *reply;
-	struct rte_mp_reply replies;
-	struct ipc_queues *request_param = (struct ipc_queues *)request.param;
-	struct ipc_queues *reply_param;
-	struct pmd_process_private *process_private = dev->process_private;
-	int queue, fd_iterator;
-
-	/* Prepare the request */
-	memset(&request, 0, sizeof(request));
-	strlcpy(request.name, TAP_MP_KEY, sizeof(request.name));
-	strlcpy(request_param->port_name, port_name,
-		sizeof(request_param->port_name));
-	request.len_param = sizeof(*request_param);
-	/* Send request and receive reply */
-	ret = rte_mp_request_sync(&request, &replies, &timeout);
-	if (ret < 0 || replies.nb_received != 1) {
-		TAP_LOG(ERR, "Failed to request queues from primary: %d",
-			rte_errno);
-		return -1;
-	}
-	reply = &replies.msgs[0];
-	reply_param = (struct ipc_queues *)reply->param;
-	TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
-
-	/* Attach the queues from received file descriptors */
-	if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds) {
-		TAP_LOG(ERR, "Unexpected number of fds received");
-		return -1;
-	}
-
-	dev->data->nb_rx_queues = reply_param->rxq_count;
-	dev->data->nb_tx_queues = reply_param->txq_count;
-	fd_iterator = 0;
-	for (queue = 0; queue < reply_param->rxq_count; queue++)
-		process_private->rxq_fds[queue] = reply->fds[fd_iterator++];
-	for (queue = 0; queue < reply_param->txq_count; queue++)
-		process_private->txq_fds[queue] = reply->fds[fd_iterator++];
-	free(reply);
-	return 0;
-}
-
-/* Send the queue file descriptors from the primary process to secondary. */
-static int
-tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
-{
-	struct rte_eth_dev *dev;
-	struct pmd_process_private *process_private;
-	struct rte_mp_msg reply;
-	const struct ipc_queues *request_param =
-		(const struct ipc_queues *)request->param;
-	struct ipc_queues *reply_param =
-		(struct ipc_queues *)reply.param;
-	uint16_t port_id;
-	int queue;
-	int ret;
-
-	/* Get requested port */
-	TAP_LOG(DEBUG, "Received IPC request for %s", request_param->port_name);
-	ret = rte_eth_dev_get_port_by_name(request_param->port_name, &port_id);
-	if (ret) {
-		TAP_LOG(ERR, "Failed to get port id for %s",
-			request_param->port_name);
-		return -1;
-	}
-	dev = &rte_eth_devices[port_id];
-	process_private = dev->process_private;
-
-	/* Fill file descriptors for all queues */
-	reply.num_fds = 0;
-	reply_param->rxq_count = 0;
-	if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
-			RTE_MP_MAX_FD_NUM){
-		TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of fds");
-		return -1;
-	}
-
-	for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
-		reply.fds[reply.num_fds++] = process_private->rxq_fds[queue];
-		reply_param->rxq_count++;
-	}
-	RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
-
-	reply_param->txq_count = 0;
-	for (queue = 0; queue < dev->data->nb_tx_queues; queue++) {
-		reply.fds[reply.num_fds++] = process_private->txq_fds[queue];
-		reply_param->txq_count++;
-	}
-	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
-
-	/* Send reply */
-	strlcpy(reply.name, request->name, sizeof(reply.name));
-	strlcpy(reply_param->port_name, request_param->port_name,
-		sizeof(reply_param->port_name));
-	reply.len_param = sizeof(*reply_param);
-	if (rte_mp_reply(&reply, peer) < 0) {
-		TAP_LOG(ERR, "Failed to reply an IPC request to sync queues");
-		return -1;
-	}
-	return 0;
-}
-
 /* Open a TAP interface device.
  */
 static int

@@ -2442,9 +2399,11 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 			return -1;
 		}
 
-		ret = tap_mp_attach_queues(name, eth_dev);
-		if (ret != 0)
-			return -1;
+		ret = rte_mp_action_register(TAP_MP_REQ_START_RXTX, tap_mp_req_start_rxtx);
+		if (ret < 0 && rte_errno != ENOTSUP)
+			TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
+				strerror(rte_errno));
+
 		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
 	}

@@ -2492,15 +2451,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 
 	TAP_LOG(DEBUG, "Initializing pmd_tap for %s", name);
 
-	/* Register IPC feed callback */
-	if (!tap_devices_count) {
-		ret = rte_mp_action_register(TAP_MP_KEY, tap_mp_sync_queues);
-		if (ret < 0 && rte_errno != ENOTSUP) {
-			TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
-				strerror(rte_errno));
-			goto leave;
-		}
-	}
 	tap_devices_count++;
 	tap_devices_count_increased = 1;
 	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,

@@ -2511,8 +2461,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 		TAP_LOG(ERR, "Failed to create pmd for %s as %s",
 			name, tap_name);
 		if (tap_devices_count_increased == 1) {
-			if (tap_devices_count == 1)
-				rte_mp_action_unregister(TAP_MP_KEY);
 			tap_devices_count--;
 		}
 	}

lib/ethdev/rte_ethdev.c +11 -0

index a1d475a292..9192b0d664 100644
@@ -894,6 +894,17 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
 	return -ENODEV;
 }
 
+struct rte_eth_dev *
+rte_get_eth_dev_by_name(const char *name)
+{
+	uint16_t pid;
+
+	if (rte_eth_dev_get_port_by_name(name, &pid))
+		return NULL;
+
+	return &rte_eth_devices[pid];
+}
+
 static int
 eth_err(uint16_t port_id, int ret)
 {

lib/ethdev/rte_ethdev.h +17 -0

index 096b676fc1..ca16839c53 100644
@@ -4987,6 +4987,23 @@ rte_eth_read_clock(uint16_t port_id, uint64_t *clock);
 int
 rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id);
 
+/**
+* Get rte_eth_dev from device name. The device name should be specified
+* as below:
+* - PCIe address (Domain:Bus:Device.Function), for example- 0000:2:00.0
+* - SoC device name, for example- fsl-gmac0
+* - vdev dpdk name, for example- net_[pcap0|null0|tap0]
+*
+* @param name
+*  pci address or name of the device
+* @return
+*   - rte_eth_dev if successful
+*   - NULL on failure
+*/
+__rte_experimental
+struct rte_eth_dev*
+rte_get_eth_dev_by_name(const char *name);
+
 /**
 * Get the device name from port ID. The device name is specified as below:
 * - PCIe address (Domain:Bus:Device.Function), for example- 0000:02:00.0

lib/ethdev/version.map +2 -0

index c2fb0669a4..fdfdf68c3d 100644
@@ -256,6 +256,8 @@ EXPERIMENTAL {
 	rte_flow_flex_item_create;
 	rte_flow_flex_item_release;
 	rte_flow_pick_transfer_proxy;
+
+	rte_get_eth_dev_by_name;
 };
 
 INTERNAL {

Signature · 1 line

2.17.1

Ferruh Yigit

2022-01-24 09:48 UTC #1 lore raw

Quoted 8 lines · Kumara Parameshwaran

From: Kumara Parameshwaran<[email protected]>
When a tap device is hotplugged to primary process which in turn adds the device to all secondary process, the secondary process does a tap_mp_attach_queues, but the fds are not populated in the primary during the probe they are populated during the queue_setup, added a fix to sync the queues during rte_eth_dev_start

Quoted 7 lines

Signed-off-by Kumara Parameshwaran<[email protected]>

---
  drivers/net/tap/rte_eth_tap.c | 196 +++++++++++++---------------------
  lib/ethdev/rte_ethdev.c       |  11 ++
  lib/ethdev/rte_ethdev.h       |  17 +++
  lib/ethdev/version.map        |   2 +
  4 files changed, 102 insertions(+), 124 deletions(-)

Hi Kumara,

I see you have sent multiple version of the patch, but it is very hard to follow them in the way you did it, can you please check:
https://doc.dpdk.org/guides/contributing/patches.html

And briefly:
- Please put a version information in the patch title, like:
[PATCH v2] ....
There are "git format-patch" or "git send-email" argument to do this easily

- Keep a changelog in the patch, as a note (after "---" in the commit log), like:

Signed-off-by: ...
---
V3:
* Added this, removed that

v2:
* Rewrite from scratch

- Keep all version of your patches in same email thread, this can be done via "git sent-email", '--in-reply-to' argument, please check documents.

This helps to keep all version and change requests and changes grouped, so makes it easy to manage. Also for archives, it makes easier to find out all versions and see the stages of improvement, otherwise it is very hard to trace a past feature from archives.

And as far as I got, last patch you send makes the API experimental, but the API is for drivers (not applications), so it should be defined in 'ethdev_driver.h' and marked as internal, also in INTERNAL group. (I already commented this one of the other version of the patch)

Another thing is, there are some checkpatch warnings, you can see check results in the "Checks" box of:
https://patches.dpdk.org/project/dpdk/patch/[email protected]/
The "ci/checkpatch" test is colored as yellow, if you click it you will see the warning.

And I am pretty sure internal tool './devtools/check-git-log.sh' will have a few things to complain.
Can you please solve above checkpatch and check-git-log warnings in next version?

Thanks,
ferruh

Kumara Parameshwaran

author

2022-01-24 12:13 UTC #2 lore raw

PATCH v2 1/1 net/tap: fix to populate fds in secondary process

In reply to Kumara Parameshwaran

From: Kumara Parameshwaran <[email protected]>

Fixes 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")

Cc [email protected]

Signed-off-by Kumara Parameshwaran <[email protected]>

---

v2:
* Addressed review comments to move the function declaration and version map

 drivers/net/tap/rte_eth_tap.c | 196 +++++++++++++---------------------
 lib/ethdev/ethdev_driver.h    |  17 +++
 lib/ethdev/rte_ethdev.c       |  11 ++
 lib/ethdev/version.map        |   2 +
 4 files changed, 102 insertions(+), 124 deletions(-)

drivers/net/tap/rte_eth_tap.c +72 ~~-124~~

index f1b48cae82..f6c25d7e21 100644
@@ -66,7 +66,7 @@
 	(TAP_GSO_MBUFS_PER_CORE * TAP_GSO_MBUF_CACHE_SIZE)
 
 /* IPC key for queue fds sync */
-#define TAP_MP_KEY "tap_mp_sync_queues"
+#define TAP_MP_REQ_START_RXTX "tap_mp_req_start_rxtx"
 
 #define TAP_IOV_DEFAULT_MAX 1024

@@ -880,11 +880,48 @@ tap_link_set_up(struct rte_eth_dev *dev)
 	return tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
 }
 
+static int tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
+{
+	struct rte_mp_msg msg;
+	struct ipc_queues *request_param = (struct ipc_queues *)msg.param;
+	int err;
+	int fd_iterator = 0;
+	struct pmd_process_private *process_private = dev->process_private;
+	int i;
+
+	memset(&msg, 0, sizeof(msg));
+	strlcpy(msg.name, TAP_MP_REQ_START_RXTX, sizeof(msg.name));
+	strlcpy(request_param->port_name, dev->data->name, sizeof(request_param->port_name));
+	msg.len_param = sizeof(*request_param);
+	for (i = 0; i < dev->data->nb_tx_queues; i++) {
+		msg.fds[fd_iterator++] = process_private->txq_fds[i];
+		msg.num_fds++;
+		request_param->txq_count++;
+	}
+	for (i = 0; i < dev->data->nb_rx_queues; i++) {
+		msg.fds[fd_iterator++] = process_private->rxq_fds[i];
+		msg.num_fds++;
+		request_param->rxq_count++;
+	}
+
+	err = rte_mp_sendmsg(&msg);
+	if (err < 0) {
+		TAP_LOG(ERR, "Failed to send start req to secondary %d",
+			rte_errno);
+		return -1;
+	}
+
+	return 0;
+}
+
 static int
 tap_dev_start(struct rte_eth_dev *dev)
 {
 	int err, i;
 
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+		tap_mp_req_on_rxtx(dev);
+
 	err = tap_intr_handle_set(dev, 1);
 	if (err)
 		return err;

@@ -901,6 +938,34 @@ tap_dev_start(struct rte_eth_dev *dev)
 	return err;
 }
 
+static int
+tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused const void *peer)
+{
+	struct rte_eth_dev *dev;
+	const struct ipc_queues *request_param =
+		(const struct ipc_queues *)request->param;
+	int fd_iterator;
+	int queue;
+	struct pmd_process_private *process_private;
+
+	dev = rte_get_eth_dev_by_name(request_param->port_name);
+	if (!dev) {
+		TAP_LOG(ERR, "Failed to get dev for %s",
+			request_param->port_name);
+		return -1;
+	}
+	process_private = dev->process_private;
+	fd_iterator = 0;
+	TAP_LOG(DEBUG, "tap_attach rx_q:%d tx_q:%d\n", request_param->rxq_count,
+		request_param->txq_count);
+	for (queue = 0; queue < request_param->txq_count; queue++)
+		process_private->txq_fds[queue] = request->fds[fd_iterator++];
+	for (queue = 0; queue < request_param->rxq_count; queue++)
+		process_private->rxq_fds[queue] = request->fds[fd_iterator++];
+
+	return 0;
+}
+
 /* This function gets called when the current port gets stopped.
  */
 static int

@@ -1084,6 +1149,7 @@ tap_dev_close(struct rte_eth_dev *dev)
 
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
 		rte_free(dev->process_private);
+		rte_mp_action_unregister(TAP_MP_REQ_START_RXTX);
 		return 0;
 	}

@@ -1140,8 +1206,6 @@ tap_dev_close(struct rte_eth_dev *dev)
 		internals->ioctl_sock = -1;
 	}
 	rte_free(dev->process_private);
-	if (tap_devices_count == 1)
-		rte_mp_action_unregister(TAP_MP_KEY);
 	tap_devices_count--;
 	/*
 	 * Since TUN device has no more opened file descriptors

@@ -2292,113 +2356,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
 	return ret;
 }
 
-/* Request queue file descriptors from secondary to primary. */
-static int
-tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
-{
-	int ret;
-	struct timespec timeout = {.tv_sec = 1, .tv_nsec = 0};
-	struct rte_mp_msg request, *reply;
-	struct rte_mp_reply replies;
-	struct ipc_queues *request_param = (struct ipc_queues *)request.param;
-	struct ipc_queues *reply_param;
-	struct pmd_process_private *process_private = dev->process_private;
-	int queue, fd_iterator;
-
-	/* Prepare the request */
-	memset(&request, 0, sizeof(request));
-	strlcpy(request.name, TAP_MP_KEY, sizeof(request.name));
-	strlcpy(request_param->port_name, port_name,
-		sizeof(request_param->port_name));
-	request.len_param = sizeof(*request_param);
-	/* Send request and receive reply */
-	ret = rte_mp_request_sync(&request, &replies, &timeout);
-	if (ret < 0 || replies.nb_received != 1) {
-		TAP_LOG(ERR, "Failed to request queues from primary: %d",
-			rte_errno);
-		return -1;
-	}
-	reply = &replies.msgs[0];
-	reply_param = (struct ipc_queues *)reply->param;
-	TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
-
-	/* Attach the queues from received file descriptors */
-	if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds) {
-		TAP_LOG(ERR, "Unexpected number of fds received");
-		return -1;
-	}
-
-	dev->data->nb_rx_queues = reply_param->rxq_count;
-	dev->data->nb_tx_queues = reply_param->txq_count;
-	fd_iterator = 0;
-	for (queue = 0; queue < reply_param->rxq_count; queue++)
-		process_private->rxq_fds[queue] = reply->fds[fd_iterator++];
-	for (queue = 0; queue < reply_param->txq_count; queue++)
-		process_private->txq_fds[queue] = reply->fds[fd_iterator++];
-	free(reply);
-	return 0;
-}
-
-/* Send the queue file descriptors from the primary process to secondary. */
-static int
-tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
-{
-	struct rte_eth_dev *dev;
-	struct pmd_process_private *process_private;
-	struct rte_mp_msg reply;
-	const struct ipc_queues *request_param =
-		(const struct ipc_queues *)request->param;
-	struct ipc_queues *reply_param =
-		(struct ipc_queues *)reply.param;
-	uint16_t port_id;
-	int queue;
-	int ret;
-
-	/* Get requested port */
-	TAP_LOG(DEBUG, "Received IPC request for %s", request_param->port_name);
-	ret = rte_eth_dev_get_port_by_name(request_param->port_name, &port_id);
-	if (ret) {
-		TAP_LOG(ERR, "Failed to get port id for %s",
-			request_param->port_name);
-		return -1;
-	}
-	dev = &rte_eth_devices[port_id];
-	process_private = dev->process_private;
-
-	/* Fill file descriptors for all queues */
-	reply.num_fds = 0;
-	reply_param->rxq_count = 0;
-	if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
-			RTE_MP_MAX_FD_NUM){
-		TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of fds");
-		return -1;
-	}
-
-	for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
-		reply.fds[reply.num_fds++] = process_private->rxq_fds[queue];
-		reply_param->rxq_count++;
-	}
-	RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
-
-	reply_param->txq_count = 0;
-	for (queue = 0; queue < dev->data->nb_tx_queues; queue++) {
-		reply.fds[reply.num_fds++] = process_private->txq_fds[queue];
-		reply_param->txq_count++;
-	}
-	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
-
-	/* Send reply */
-	strlcpy(reply.name, request->name, sizeof(reply.name));
-	strlcpy(reply_param->port_name, request_param->port_name,
-		sizeof(reply_param->port_name));
-	reply.len_param = sizeof(*reply_param);
-	if (rte_mp_reply(&reply, peer) < 0) {
-		TAP_LOG(ERR, "Failed to reply an IPC request to sync queues");
-		return -1;
-	}
-	return 0;
-}
-
 /* Open a TAP interface device.
  */
 static int

@@ -2442,9 +2399,11 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 			return -1;
 		}
 
-		ret = tap_mp_attach_queues(name, eth_dev);
-		if (ret != 0)
-			return -1;
+		ret = rte_mp_action_register(TAP_MP_REQ_START_RXTX, tap_mp_req_start_rxtx);
+		if (ret < 0 && rte_errno != ENOTSUP)
+			TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
+				strerror(rte_errno));
+
 		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
 	}

@@ -2492,15 +2451,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 
 	TAP_LOG(DEBUG, "Initializing pmd_tap for %s", name);
 
-	/* Register IPC feed callback */
-	if (!tap_devices_count) {
-		ret = rte_mp_action_register(TAP_MP_KEY, tap_mp_sync_queues);
-		if (ret < 0 && rte_errno != ENOTSUP) {
-			TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
-				strerror(rte_errno));
-			goto leave;
-		}
-	}
 	tap_devices_count++;
 	tap_devices_count_increased = 1;
 	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,

@@ -2511,8 +2461,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 		TAP_LOG(ERR, "Failed to create pmd for %s as %s",
 			name, tap_name);
 		if (tap_devices_count_increased == 1) {
-			if (tap_devices_count == 1)
-				rte_mp_action_unregister(TAP_MP_KEY);
 			tap_devices_count--;
 		}
 	}

lib/ethdev/ethdev_driver.h +17 -0

index d95605a355..a08991bcdf 100644
@@ -1629,6 +1629,23 @@ rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue,
 				struct rte_hairpin_peer_info *peer_info,
 				uint32_t direction);
 
+/**
+* Get rte_eth_dev from device name. The device name should be specified
+* as below:
+* - PCIe address (Domain:Bus:Device.Function), for example- 0000:2:00.0
+* - SoC device name, for example- fsl-gmac0
+* - vdev dpdk name, for example- net_[pcap0|null0|tap0]
+*
+* @param name
+*  pci address or name of the device
+* @return
+*   - rte_eth_dev if successful
+*   - NULL on failure
+*/
+__rte_internal
+struct rte_eth_dev*
+rte_get_eth_dev_by_name(const char *name);
+
 /**
  * @internal
  * Reset the current queue state and configuration to disconnect (unbind) it

lib/ethdev/rte_ethdev.c +11 -0

index a1d475a292..9192b0d664 100644
@@ -894,6 +894,17 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
 	return -ENODEV;
 }
 
+struct rte_eth_dev *
+rte_get_eth_dev_by_name(const char *name)
+{
+	uint16_t pid;
+
+	if (rte_eth_dev_get_port_by_name(name, &pid))
+		return NULL;
+
+	return &rte_eth_devices[pid];
+}
+
 static int
 eth_err(uint16_t port_id, int ret)
 {

lib/ethdev/version.map +2 -0

index c2fb0669a4..7e3797189b 100644
@@ -256,6 +256,7 @@ EXPERIMENTAL {
 	rte_flow_flex_item_create;
 	rte_flow_flex_item_release;
 	rte_flow_pick_transfer_proxy;
+
 };
 
 INTERNAL {

@@ -282,4 +283,5 @@ INTERNAL {
 	rte_eth_representor_id_get;
 	rte_eth_switch_domain_alloc;
 	rte_eth_switch_domain_free;
+	rte_get_eth_dev_by_name;
 };

Signature · 1 line

2.17.1

Ferruh Yigit

2022-01-24 12:35 UTC #3 lore raw

Re: [PATCH v2] net/tap: fix to populate fds in secondary process

Quoted 11 lines · Kumara Parameshwaran

From: Kumara Parameshwaran <[email protected]>
When a tap device is hotplugged to primary process which in turn adds the device to all secondary process, the secondary process does a tap_mp_attach_queues, but the fds are not populated in the primary during the probe they are populated during the queue_setup, added a fix to sync the queues during rte_eth_dev_start
Fixes 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")
Cc [email protected]

Quoted 7 lines

Signed-off-by Kumara Parameshwaran <[email protected]>
---
v2:
* Addressed review comments to move the function declaration and version map

Thanks for adding patch version.

Quoted 6 lines

  drivers/net/tap/rte_eth_tap.c | 196 +++++++++++++---------------------
  lib/ethdev/ethdev_driver.h    |  17 +++
  lib/ethdev/rte_ethdev.c       |  11 ++
  lib/ethdev/version.map        |   2 +
  4 files changed, 102 insertions(+), 124 deletions(-)

Can you please separate etdev (API) changes to another patch, so this will be a patchset with two patches,
first patch adds ethdev API
second patch is tap patch, that uses the API in the first patch

drivers/net/tap/rte_eth_tap.c line 70

Quoted 11 lines

drivers/net/tap/rte_eth_tap.c +1 -1

index f1b48cae82..f6c25d7e21 100644
@@ -66,7 +66,7 @@
  	(TAP_GSO_MBUFS_PER_CORE * TAP_GSO_MBUF_CACHE_SIZE)
 
  /* IPC key for queue fds sync */
-#define TAP_MP_KEY "tap_mp_sync_queues"
+#define TAP_MP_REQ_START_RXTX "tap_mp_req_start_rxtx"

We said we can drop "tap_mp_sync_queues", but thinking twice, will current implementation cover following usecase:

- Primary applicaiton started with tap interface, all config, setup, start done
- Secondary app started without any parameter

Since primary already started, I think secondary fds will be wrong, what do you think?

drivers/net/tap/rte_eth_tap.c tap_link_set_up(struct rte_eth_dev *dev) · line 883

Quoted 7 lines

drivers/net/tap/rte_eth_tap.c +1 -0

  #define TAP_IOV_DEFAULT_MAX 1024

@@ -880,11 +880,48 @@ tap_link_set_up(struct rte_eth_dev *dev)
  	return tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
  }
 
+static int tap_mp_req_on_rxtx(struct rte_eth_dev *dev)

Can you please follow the coding convention:

static int
tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
{

Quoted 45 lines

drivers/net/tap/rte_eth_tap.c +36 -0

+{
+	struct rte_mp_msg msg;
+	struct ipc_queues *request_param = (struct ipc_queues *)msg.param;
+	int err;
+	int fd_iterator = 0;
+	struct pmd_process_private *process_private = dev->process_private;
+	int i;
+
+	memset(&msg, 0, sizeof(msg));
+	strlcpy(msg.name, TAP_MP_REQ_START_RXTX, sizeof(msg.name));
+	strlcpy(request_param->port_name, dev->data->name, sizeof(request_param->port_name));
+	msg.len_param = sizeof(*request_param);
+	for (i = 0; i < dev->data->nb_tx_queues; i++) {
+		msg.fds[fd_iterator++] = process_private->txq_fds[i];
+		msg.num_fds++;
+		request_param->txq_count++;
+	}
+	for (i = 0; i < dev->data->nb_rx_queues; i++) {
+		msg.fds[fd_iterator++] = process_private->rxq_fds[i];
+		msg.num_fds++;
+		request_param->rxq_count++;
+	}
+
+	err = rte_mp_sendmsg(&msg);
+	if (err < 0) {
+		TAP_LOG(ERR, "Failed to send start req to secondary %d",
+			rte_errno);
+		return -1;
+	}
+
+	return 0;
+}
+
  static int
  tap_dev_start(struct rte_eth_dev *dev)
  {
  	int err, i;
 
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+		tap_mp_req_on_rxtx(dev);
+
  	err = tap_intr_handle_set(dev, 1);
  	if (err)
  		return err;

drivers/net/tap/rte_eth_tap.c tap_dev_start(struct rte_eth_dev *dev) · line 943

Quoted 6 lines

drivers/net/tap/rte_eth_tap.c +3 -0

@@ -901,6 +938,34 @@ tap_dev_start(struct rte_eth_dev *dev)
  	return err;
  }
 
+static int
+tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused const void *peer)
+{

I asked last time but I don't remember the response, what should be in the 'peer' variable?

Quoted 204 lines

drivers/net/tap/rte_eth_tap.c +31 ~~-123~~

+	struct rte_eth_dev *dev;
+	const struct ipc_queues *request_param =
+		(const struct ipc_queues *)request->param;
+	int fd_iterator;
+	int queue;
+	struct pmd_process_private *process_private;
+
+	dev = rte_get_eth_dev_by_name(request_param->port_name);
+	if (!dev) {
+		TAP_LOG(ERR, "Failed to get dev for %s",
+			request_param->port_name);
+		return -1;
+	}
+	process_private = dev->process_private;
+	fd_iterator = 0;
+	TAP_LOG(DEBUG, "tap_attach rx_q:%d tx_q:%d\n", request_param->rxq_count,
+		request_param->txq_count);
+	for (queue = 0; queue < request_param->txq_count; queue++)
+		process_private->txq_fds[queue] = request->fds[fd_iterator++];
+	for (queue = 0; queue < request_param->rxq_count; queue++)
+		process_private->rxq_fds[queue] = request->fds[fd_iterator++];
+
+	return 0;
+}
+
  /* This function gets called when the current port gets stopped.
   */
  static int

@@ -1084,6 +1149,7 @@ tap_dev_close(struct rte_eth_dev *dev)
 
  	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
  		rte_free(dev->process_private);
+		rte_mp_action_unregister(TAP_MP_REQ_START_RXTX);
  		return 0;
  	}

@@ -1140,8 +1206,6 @@ tap_dev_close(struct rte_eth_dev *dev)
  		internals->ioctl_sock = -1;
  	}
  	rte_free(dev->process_private);
-	if (tap_devices_count == 1)
-		rte_mp_action_unregister(TAP_MP_KEY);
  	tap_devices_count--;
  	/*
  	 * Since TUN device has no more opened file descriptors

@@ -2292,113 +2356,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
  	return ret;
  }
 
-/* Request queue file descriptors from secondary to primary. */
-static int
-tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
-{
-	int ret;
-	struct timespec timeout = {.tv_sec = 1, .tv_nsec = 0};
-	struct rte_mp_msg request, *reply;
-	struct rte_mp_reply replies;
-	struct ipc_queues *request_param = (struct ipc_queues *)request.param;
-	struct ipc_queues *reply_param;
-	struct pmd_process_private *process_private = dev->process_private;
-	int queue, fd_iterator;
-
-	/* Prepare the request */
-	memset(&request, 0, sizeof(request));
-	strlcpy(request.name, TAP_MP_KEY, sizeof(request.name));
-	strlcpy(request_param->port_name, port_name,
-		sizeof(request_param->port_name));
-	request.len_param = sizeof(*request_param);
-	/* Send request and receive reply */
-	ret = rte_mp_request_sync(&request, &replies, &timeout);
-	if (ret < 0 || replies.nb_received != 1) {
-		TAP_LOG(ERR, "Failed to request queues from primary: %d",
-			rte_errno);
-		return -1;
-	}
-	reply = &replies.msgs[0];
-	reply_param = (struct ipc_queues *)reply->param;
-	TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
-
-	/* Attach the queues from received file descriptors */
-	if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds) {
-		TAP_LOG(ERR, "Unexpected number of fds received");
-		return -1;
-	}
-
-	dev->data->nb_rx_queues = reply_param->rxq_count;
-	dev->data->nb_tx_queues = reply_param->txq_count;
-	fd_iterator = 0;
-	for (queue = 0; queue < reply_param->rxq_count; queue++)
-		process_private->rxq_fds[queue] = reply->fds[fd_iterator++];
-	for (queue = 0; queue < reply_param->txq_count; queue++)
-		process_private->txq_fds[queue] = reply->fds[fd_iterator++];
-	free(reply);
-	return 0;
-}
-
-/* Send the queue file descriptors from the primary process to secondary. */
-static int
-tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
-{
-	struct rte_eth_dev *dev;
-	struct pmd_process_private *process_private;
-	struct rte_mp_msg reply;
-	const struct ipc_queues *request_param =
-		(const struct ipc_queues *)request->param;
-	struct ipc_queues *reply_param =
-		(struct ipc_queues *)reply.param;
-	uint16_t port_id;
-	int queue;
-	int ret;
-
-	/* Get requested port */
-	TAP_LOG(DEBUG, "Received IPC request for %s", request_param->port_name);
-	ret = rte_eth_dev_get_port_by_name(request_param->port_name, &port_id);
-	if (ret) {
-		TAP_LOG(ERR, "Failed to get port id for %s",
-			request_param->port_name);
-		return -1;
-	}
-	dev = &rte_eth_devices[port_id];
-	process_private = dev->process_private;
-
-	/* Fill file descriptors for all queues */
-	reply.num_fds = 0;
-	reply_param->rxq_count = 0;
-	if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
-			RTE_MP_MAX_FD_NUM){
-		TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of fds");
-		return -1;
-	}
-
-	for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
-		reply.fds[reply.num_fds++] = process_private->rxq_fds[queue];
-		reply_param->rxq_count++;
-	}
-	RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
-
-	reply_param->txq_count = 0;
-	for (queue = 0; queue < dev->data->nb_tx_queues; queue++) {
-		reply.fds[reply.num_fds++] = process_private->txq_fds[queue];
-		reply_param->txq_count++;
-	}
-	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
-
-	/* Send reply */
-	strlcpy(reply.name, request->name, sizeof(reply.name));
-	strlcpy(reply_param->port_name, request_param->port_name,
-		sizeof(reply_param->port_name));
-	reply.len_param = sizeof(*reply_param);
-	if (rte_mp_reply(&reply, peer) < 0) {
-		TAP_LOG(ERR, "Failed to reply an IPC request to sync queues");
-		return -1;
-	}
-	return 0;
-}
-
  /* Open a TAP interface device.
   */
  static int

@@ -2442,9 +2399,11 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
  			return -1;
  		}
 
-		ret = tap_mp_attach_queues(name, eth_dev);
-		if (ret != 0)
-			return -1;
+		ret = rte_mp_action_register(TAP_MP_REQ_START_RXTX, tap_mp_req_start_rxtx);
+		if (ret < 0 && rte_errno != ENOTSUP)
+			TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
+				strerror(rte_errno));
+
  		rte_eth_dev_probing_finish(eth_dev);
  		return 0;
  	}

@@ -2492,15 +2451,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 
  	TAP_LOG(DEBUG, "Initializing pmd_tap for %s", name);
 
-	/* Register IPC feed callback */
-	if (!tap_devices_count) {
-		ret = rte_mp_action_register(TAP_MP_KEY, tap_mp_sync_queues);
-		if (ret < 0 && rte_errno != ENOTSUP) {
-			TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
-				strerror(rte_errno));
-			goto leave;
-		}
-	}
  	tap_devices_count++;
  	tap_devices_count_increased = 1;
  	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,

@@ -2511,8 +2461,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
  		TAP_LOG(ERR, "Failed to create pmd for %s as %s",
  			name, tap_name);
  		if (tap_devices_count_increased == 1) {
-			if (tap_devices_count == 1)
-				rte_mp_action_unregister(TAP_MP_KEY);
  			tap_devices_count--;
  		}
  	}

lib/ethdev/ethdev_driver.h +0 -0

index d95605a355..a08991bcdf 100644

lib/ethdev/ethdev_driver.h rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue, · line 1632

Quoted 4 lines

lib/ethdev/ethdev_driver.h +1 -0

@@ -1629,6 +1629,23 @@ rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue,
  				struct rte_hairpin_peer_info *peer_info,
  				uint32_t direction);
 
+/**

Please add '@internal' tag into doxygen comment.

Quoted 5 lines

lib/ethdev/ethdev_driver.h +5 -0

+* Get rte_eth_dev from device name. The device name should be specified
+* as below:
+* - PCIe address (Domain:Bus:Device.Function), for example- 0000:2:00.0
+* - SoC device name, for example- fsl-gmac0
+* - vdev dpdk name, for example- net_[pcap0|null0|tap0]

lib/ethdev/ethdev_driver.h rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue, · line 1647

Quoted 10 lines

lib/ethdev/ethdev_driver.h +10 -0

+*
+* @param name
+*  pci address or name of the device
+* @return
+*   - rte_eth_dev if successful
+*   - NULL on failure
+*/
+__rte_internal
+struct rte_eth_dev*
+rte_get_eth_dev_by_name(const char *name);

As the API name, better to start with 'rte_eth_' prefix to be consistent with rest of the APIs.
I suggest 'rte_eth_dev_get_by_name' but feel free to chose better one.

Quoted 31 lines

lib/ethdev/ethdev_driver.h +1 -0

+
  /**
   * @internal
   * Reset the current queue state and configuration to disconnect (unbind) it

lib/ethdev/rte_ethdev.c +11 -0

index a1d475a292..9192b0d664 100644
@@ -894,6 +894,17 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
  	return -ENODEV;
  }
 
+struct rte_eth_dev *
+rte_get_eth_dev_by_name(const char *name)
+{
+	uint16_t pid;
+
+	if (rte_eth_dev_get_port_by_name(name, &pid))
+		return NULL;
+
+	return &rte_eth_devices[pid];
+}
+
  static int
  eth_err(uint16_t port_id, int ret)
  {

lib/ethdev/version.map +0 -0

index c2fb0669a4..7e3797189b 100644

lib/ethdev/version.map EXPERIMENTAL { · line 259

Quoted 4 lines

lib/ethdev/version.map +1 -0

@@ -256,6 +256,7 @@ EXPERIMENTAL {
  	rte_flow_flex_item_create;
  	rte_flow_flex_item_release;
  	rte_flow_pick_transfer_proxy;
+

This is unintendent change.

lib/ethdev/version.map INTERNAL { · line 286

Quoted 8 lines

lib/ethdev/version.map +1 -0

  };
 
  INTERNAL {

@@ -282,4 +283,5 @@ INTERNAL {
  	rte_eth_representor_id_get;
  	rte_eth_switch_domain_alloc;
  	rte_eth_switch_domain_free;
+	rte_get_eth_dev_by_name;

Please add in a sorted way.

Quoted 1 line

lib/ethdev/version.map +0 -0
  };

kumaraparameshwaran rathinavel

author

2022-01-24 12:55 UTC #4 lore raw

Re: [PATCH v2] net/tap: fix to populate fds in secondary process

Quoted 29 lines · Ferruh Yigit

Quoted 11 lines · Kumara Parameshwaran
From: Kumara Parameshwaran <[email protected]>
When a tap device is hotplugged to primary process which in turn adds the device to all secondary process, the secondary process does a tap_mp_attach_queues, but the fds are not populated in the primary during the probe they are populated during the queue_setup, added a fix to sync the queues during rte_eth_dev_start
Fixes 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")
Cc [email protected]
Quoted 7 lines
Signed-off-by Kumara Parameshwaran <[email protected]>
---
v2:
* Addressed review comments to move the function declaration and version map
Thanks for adding patch version.
Quoted 6 lines
  drivers/net/tap/rte_eth_tap.c | 196 +++++++++++++---------------------
  lib/ethdev/ethdev_driver.h    |  17 +++
  lib/ethdev/rte_ethdev.c       |  11 ++
  lib/ethdev/version.map        |   2 +
  4 files changed, 102 insertions(+), 124 deletions(-)

Quoted 5 lines

Can you please separate etdev (API) changes to another patch, so this will be a patchset
with two patches,
first patch adds ethdev API
second patch is tap patch, that uses the API in the first patch

Sure, I will do it.

Quoted 402 lines

Review

Quoted 2 lines

(quoted diff) +0 -0

b/drivers/net/tap/rte_eth_tap.c

drivers/net/tap/rte_eth_tap.c line 70

Quoted 10 lines

index f1b48cae82..f6c25d7e21 100644

drivers/net/tap/rte_eth_tap.c +1 -1

@@ -66,7 +66,7 @@
      (TAP_GSO_MBUFS_PER_CORE * TAP_GSO_MBUF_CACHE_SIZE)
 
  /* IPC key for queue fds sync */
-#define TAP_MP_KEY "tap_mp_sync_queues"
+#define TAP_MP_REQ_START_RXTX "tap_mp_req_start_rxtx"

We said we can drop "tap_mp_sync_queues", but thinking twice, will current implementation cover following usecase:

- Primary applicaiton started with tap interface, all config, setup, start done
- Secondary app started without any parameter

Since primary already started, I think secondary fds will be wrong, what do you think?
That is true, the fds would be unitialised, we should retain for the secondary attach case.

drivers/net/tap/rte_eth_tap.c tap_link_set_up(struct rte_eth_dev *dev) · line 883

Quoted 7 lines

drivers/net/tap/rte_eth_tap.c +1 -0

  #define TAP_IOV_DEFAULT_MAX 1024

@@ -880,11 +880,48 @@ tap_link_set_up(struct rte_eth_dev *dev)
      return tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
  }
 
+static int tap_mp_req_on_rxtx(struct rte_eth_dev *dev)

Can you please follow the coding convention:
sure, will do it.
static int
tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
{

drivers/net/tap/rte_eth_tap.c tap_link_set_up(struct rte_eth_dev *dev)

Quoted 11 lines

drivers/net/tap/rte_eth_tap.c +11 -0

+{
+     struct rte_mp_msg msg;
+     struct ipc_queues *request_param = (struct ipc_queues *)msg.param;
+     int err;
+     int fd_iterator = 0;
+     struct pmd_process_private *process_private = dev->process_private;
+     int i;
+
+     memset(&msg, 0, sizeof(msg));
+     strlcpy(msg.name, TAP_MP_REQ_START_RXTX, sizeof(msg.name));
+     strlcpy(request_param->port_name, dev->data->name,

sizeof(request_param->port_name));

Quoted 34 lines

drivers/net/tap/rte_eth_tap.c +25 -0

+     msg.len_param = sizeof(*request_param);
+     for (i = 0; i < dev->data->nb_tx_queues; i++) {
+             msg.fds[fd_iterator++] = process_private->txq_fds[i];
+             msg.num_fds++;
+             request_param->txq_count++;
+     }
+     for (i = 0; i < dev->data->nb_rx_queues; i++) {
+             msg.fds[fd_iterator++] = process_private->rxq_fds[i];
+             msg.num_fds++;
+             request_param->rxq_count++;
+     }
+
+     err = rte_mp_sendmsg(&msg);
+     if (err < 0) {
+             TAP_LOG(ERR, "Failed to send start req to secondary %d",
+                     rte_errno);
+             return -1;
+     }
+
+     return 0;
+}
+
  static int
  tap_dev_start(struct rte_eth_dev *dev)
  {
      int err, i;
 
+     if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+             tap_mp_req_on_rxtx(dev);
+
      err = tap_intr_handle_set(dev, 1);
      if (err)
              return err;

drivers/net/tap/rte_eth_tap.c tap_dev_start(struct rte_eth_dev *dev) · line 942

Quoted 5 lines

drivers/net/tap/rte_eth_tap.c +2 -0

@@ -901,6 +938,34 @@ tap_dev_start(struct rte_eth_dev *dev)
      return err;
  }
 
+static int
+tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused

const void *peer)

drivers/net/tap/rte_eth_tap.c tap_dev_start(struct rte_eth_dev *dev) · line 943

Quoted 1 line

drivers/net/tap/rte_eth_tap.c +1 -0
+{

I asked last time but I don't remember the response, what should be in the 'peer' variable?

Quoted 6 lines

drivers/net/tap/rte_eth_tap.c +6 -0

+     struct rte_eth_dev *dev;
+     const struct ipc_queues *request_param =
+             (const struct ipc_queues *)request->param;
+     int fd_iterator;
+     int queue;
+     struct pmd_process_private *process_private;

drivers/net/tap/rte_eth_tap.c tap_dev_start(struct rte_eth_dev *dev)

Quoted 10 lines

drivers/net/tap/rte_eth_tap.c +10 -0

+
+     dev = rte_get_eth_dev_by_name(request_param->port_name);
+     if (!dev) {
+             TAP_LOG(ERR, "Failed to get dev for %s",
+                     request_param->port_name);
+             return -1;
+     }
+     process_private = dev->process_private;
+     fd_iterator = 0;
+     TAP_LOG(DEBUG, "tap_attach rx_q:%d tx_q:%d\n",

request_param->rxq_count,

drivers/net/tap/rte_eth_tap.c tap_dev_start(struct rte_eth_dev *dev)

Quoted 3 lines

drivers/net/tap/rte_eth_tap.c +3 -0

+             request_param->txq_count);
+     for (queue = 0; queue < request_param->txq_count; queue++)
+             process_private->txq_fds[queue] =

request->fds[fd_iterator++];

drivers/net/tap/rte_eth_tap.c tap_dev_start(struct rte_eth_dev *dev) · line 964

Quoted 2 lines

drivers/net/tap/rte_eth_tap.c +2 -0

+     for (queue = 0; queue < request_param->rxq_count; queue++)
+             process_private->rxq_fds[queue] =

request->fds[fd_iterator++];

Quoted 27 lines

drivers/net/tap/rte_eth_tap.c +5 -2

+
+     return 0;
+}
+
  /* This function gets called when the current port gets stopped.
   */
  static int

@@ -1084,6 +1149,7 @@ tap_dev_close(struct rte_eth_dev *dev)
 
      if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
              rte_free(dev->process_private);
+             rte_mp_action_unregister(TAP_MP_REQ_START_RXTX);
              return 0;
      }

@@ -1140,8 +1206,6 @@ tap_dev_close(struct rte_eth_dev *dev)
              internals->ioctl_sock = -1;
      }
      rte_free(dev->process_private);
-     if (tap_devices_count == 1)
-             rte_mp_action_unregister(TAP_MP_KEY);
      tap_devices_count--;
      /*
       * Since TUN device has no more opened file descriptors

@@ -2292,113 +2356,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
      return ret;
  }

drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev) · line 2303

Quoted 10 lines

drivers/net/tap/rte_eth_tap.c +0 -9

 
-/* Request queue file descriptors from secondary to primary. */
-static int
-tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
-{
-     int ret;
-     struct timespec timeout = {.tv_sec = 1, .tv_nsec = 0};
-     struct rte_mp_msg request, *reply;
-     struct rte_mp_reply replies;
-     struct ipc_queues *request_param = (struct ipc_queues

*)request.param;

Quoted 10 lines

drivers/net/tap/rte_eth_tap.c +0 ~~-10~~

-     struct ipc_queues *reply_param;
-     struct pmd_process_private *process_private = dev->process_private;
-     int queue, fd_iterator;
-
-     /* Prepare the request */
-     memset(&request, 0, sizeof(request));
-     strlcpy(request.name, TAP_MP_KEY, sizeof(request.name));
-     strlcpy(request_param->port_name, port_name,
-             sizeof(request_param->port_name));
-     request.len_param = sizeof(*request_param);

drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev)

Quoted 10 lines

drivers/net/tap/rte_eth_tap.c +0 ~~-10~~

-     /* Send request and receive reply */
-     ret = rte_mp_request_sync(&request, &replies, &timeout);
-     if (ret < 0 || replies.nb_received != 1) {
-             TAP_LOG(ERR, "Failed to request queues from primary: %d",
-                     rte_errno);
-             return -1;
-     }
-     reply = &replies.msgs[0];
-     reply_param = (struct ipc_queues *)reply->param;
-     TAP_LOG(DEBUG, "Received IPC reply for %s",

reply_param->port_name);

drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev)

Quoted 3 lines

drivers/net/tap/rte_eth_tap.c +0 -3

-
-     /* Attach the queues from received file descriptors */
-     if (reply_param->rxq_count + reply_param->txq_count !=

reply->num_fds) {

drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev)

Quoted 9 lines

drivers/net/tap/rte_eth_tap.c +0 -9

-             TAP_LOG(ERR, "Unexpected number of fds received");
-             return -1;
-     }
-
-     dev->data->nb_rx_queues = reply_param->rxq_count;
-     dev->data->nb_tx_queues = reply_param->txq_count;
-     fd_iterator = 0;
-     for (queue = 0; queue < reply_param->rxq_count; queue++)
-             process_private->rxq_fds[queue] =

reply->fds[fd_iterator++];

drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev) · line 2337

Quoted 2 lines

drivers/net/tap/rte_eth_tap.c +0 -2

-     for (queue = 0; queue < reply_param->txq_count; queue++)
-             process_private->txq_fds[queue] =

reply->fds[fd_iterator++];

drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev)

Quoted 5 lines

drivers/net/tap/rte_eth_tap.c +0 -5

-     free(reply);
-     return 0;
-}
-
-/* Send the queue file descriptors from the primary process to

secondary. */

Quoted 6 lines

drivers/net/tap/rte_eth_tap.c +0 -6

-static int
-tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
-{
-     struct rte_eth_dev *dev;
-     struct pmd_process_private *process_private;
-     struct rte_mp_msg reply;

drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev)

Quoted 10 lines

drivers/net/tap/rte_eth_tap.c +0 ~~-10~~

-     const struct ipc_queues *request_param =
-             (const struct ipc_queues *)request->param;
-     struct ipc_queues *reply_param =
-             (struct ipc_queues *)reply.param;
-     uint16_t port_id;
-     int queue;
-     int ret;
-
-     /* Get requested port */
-     TAP_LOG(DEBUG, "Received IPC request for %s",

request_param->port_name);

drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev) · line 2359

Quoted 1 line

drivers/net/tap/rte_eth_tap.c +0 -1
-     ret = rte_eth_dev_get_port_by_name(request_param->port_name,

&port_id);

Quoted 4 lines

drivers/net/tap/rte_eth_tap.c +0 -4

-     if (ret) {
-             TAP_LOG(ERR, "Failed to get port id for %s",
-                     request_param->port_name);
-             return -1;

drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev)

Quoted 10 lines

drivers/net/tap/rte_eth_tap.c +0 ~~-10~~

-     }
-     dev = &rte_eth_devices[port_id];
-     process_private = dev->process_private;
-
-     /* Fill file descriptors for all queues */
-     reply.num_fds = 0;
-     reply_param->rxq_count = 0;
-     if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
-                     RTE_MP_MAX_FD_NUM){
-             TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of

fds");

drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev)

Quoted 5 lines

drivers/net/tap/rte_eth_tap.c +0 -5

-             return -1;
-     }
-
-     for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
-             reply.fds[reply.num_fds++] =

process_private->rxq_fds[queue];

drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev)

Quoted 7 lines

drivers/net/tap/rte_eth_tap.c +0 -7

-             reply_param->rxq_count++;
-     }
-     RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
-
-     reply_param->txq_count = 0;
-     for (queue = 0; queue < dev->data->nb_tx_queues; queue++) {
-             reply.fds[reply.num_fds++] =

process_private->txq_fds[queue];

drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev)

Quoted 11 lines

drivers/net/tap/rte_eth_tap.c +0 ~~-11~~

-             reply_param->txq_count++;
-     }
-     RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
-
-     /* Send reply */
-     strlcpy(reply.name, request->name, sizeof(reply.name));
-     strlcpy(reply_param->port_name, request_param->port_name,
-             sizeof(reply_param->port_name));
-     reply.len_param = sizeof(*reply_param);
-     if (rte_mp_reply(&reply, peer) < 0) {
-             TAP_LOG(ERR, "Failed to reply an IPC request to sync

queues");

Quoted 9 lines

drivers/net/tap/rte_eth_tap.c +0 -5

-             return -1;
-     }
-     return 0;
-}
-
  /* Open a TAP interface device.
   */
  static int

drivers/net/tap/rte_eth_tap.c rte_pmd_tap_probe(struct rte_vdev_device *dev) · line 2402

Quoted 7 lines

drivers/net/tap/rte_eth_tap.c +1 -3

@@ -2442,9 +2399,11 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
                      return -1;
              }
 
-             ret = tap_mp_attach_queues(name, eth_dev);
-             if (ret != 0)
-                     return -1;
+             ret = rte_mp_action_register(TAP_MP_REQ_START_RXTX,

tap_mp_req_start_rxtx);

drivers/net/tap/rte_eth_tap.c rte_pmd_tap_probe(struct rte_vdev_device *dev) · line 2404

Quoted 2 lines

drivers/net/tap/rte_eth_tap.c +2 -0

+             if (ret < 0 && rte_errno != ENOTSUP)
+                     TAP_LOG(ERR, "tap: Failed to register IPC

callback: %s",

drivers/net/tap/rte_eth_tap.c rte_pmd_tap_probe(struct rte_vdev_device *dev) · line 2497

Quoted 12 lines

drivers/net/tap/rte_eth_tap.c +2 -3

+                             strerror(rte_errno));
+
              rte_eth_dev_probing_finish(eth_dev);
              return 0;
      }

@@ -2492,15 +2451,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 
      TAP_LOG(DEBUG, "Initializing pmd_tap for %s", name);
 
-     /* Register IPC feed callback */
-     if (!tap_devices_count) {
-             ret = rte_mp_action_register(TAP_MP_KEY,

tap_mp_sync_queues);

drivers/net/tap/rte_eth_tap.c rte_pmd_tap_probe(struct rte_vdev_device *dev) · line 2499

Quoted 2 lines

drivers/net/tap/rte_eth_tap.c +0 -2

-             if (ret < 0 && rte_errno != ENOTSUP) {
-                     TAP_LOG(ERR, "tap: Failed to register IPC

callback: %s",

lib/ethdev/ethdev_driver.h rte_eth_hairpin_queue_peer_bind(uint16_t

Quoted 21 lines

drivers/net/tap/rte_eth_tap.c +0 -6

-                             strerror(rte_errno));
-                     goto leave;
-             }
-     }
      tap_devices_count++;
      tap_devices_count_increased = 1;
      ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,

@@ -2511,8 +2461,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
              TAP_LOG(ERR, "Failed to create pmd for %s as %s",
                      name, tap_name);
              if (tap_devices_count_increased == 1) {
-                     if (tap_devices_count == 1)
-                             rte_mp_action_unregister(TAP_MP_KEY);
                      tap_devices_count--;
              }
      }

lib/ethdev/ethdev_driver.h +0 -0

index d95605a355..a08991bcdf 100644
@@ -1629,6 +1629,23 @@ rte_eth_hairpin_queue_peer_bind(uint16_t

cur_port, uint16_t cur_queue,

lib/ethdev/ethdev_driver.h rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue, · line 1632

Quoted 4 lines

lib/ethdev/ethdev_driver.h +1 -0

                              struct rte_hairpin_peer_info *peer_info,
                              uint32_t direction);
 
+/**

Please add '@internal' tag into doxygen comment.

Quoted 5 lines

lib/ethdev/ethdev_driver.h +5 -0

+* Get rte_eth_dev from device name. The device name should be specified
+* as below:
+* - PCIe address (Domain:Bus:Device.Function), for example- 0000:2:00.0
+* - SoC device name, for example- fsl-gmac0
+* - vdev dpdk name, for example- net_[pcap0|null0|tap0]

lib/ethdev/ethdev_driver.h rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue, · line 1647

Quoted 10 lines

lib/ethdev/ethdev_driver.h +10 -0

+*
+* @param name
+*  pci address or name of the device
+* @return
+*   - rte_eth_dev if successful
+*   - NULL on failure
+*/
+__rte_internal
+struct rte_eth_dev*
+rte_get_eth_dev_by_name(const char *name);

As the API name, better to start with 'rte_eth_' prefix to be consistent with
rest of the APIs.
I suggest 'rte_eth_dev_get_by_name' but feel free to chose better one.

lib/ethdev/ethdev_driver.h rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue,

Quoted 4 lines

lib/ethdev/ethdev_driver.h +1 -0

+
  /**
   * @internal
   * Reset the current queue state and configuration to disconnect

(unbind) it

lib/ethdev/rte_ethdev.c rte_eth_dev_get_port_by_name(const char *name,

Quoted 5 lines

lib/ethdev/rte_ethdev.c +0 -0

index a1d475a292..9192b0d664 100644
@@ -894,6 +894,17 @@ rte_eth_dev_get_port_by_name(const char *name,

uint16_t *port_id)

Quoted 22 lines

lib/ethdev/rte_ethdev.c +11 -0

      return -ENODEV;
  }
 
+struct rte_eth_dev *
+rte_get_eth_dev_by_name(const char *name)
+{
+     uint16_t pid;
+
+     if (rte_eth_dev_get_port_by_name(name, &pid))
+             return NULL;
+
+     return &rte_eth_devices[pid];
+}
+
  static int
  eth_err(uint16_t port_id, int ret)
  {

lib/ethdev/version.map +0 -0

index c2fb0669a4..7e3797189b 100644

lib/ethdev/version.map EXPERIMENTAL { · line 259

Quoted 4 lines

lib/ethdev/version.map +1 -0

@@ -256,6 +256,7 @@ EXPERIMENTAL {
      rte_flow_flex_item_create;
      rte_flow_flex_item_release;
      rte_flow_pick_transfer_proxy;
+

This is unintendent change.

lib/ethdev/version.map INTERNAL { · line 286

Quoted 8 lines

lib/ethdev/version.map +1 -0

  };
 
  INTERNAL {

@@ -282,4 +283,5 @@ INTERNAL {
      rte_eth_representor_id_get;
      rte_eth_switch_domain_alloc;
      rte_eth_switch_domain_free;
+     rte_get_eth_dev_by_name;

Please add in a sorted way.

Quoted 1 line

lib/ethdev/version.map +0 -0
  };

Ferruh Yigit

2022-01-24 13:05 UTC #5 lore raw

Re: [PATCH v2] net/tap: fix to populate fds in secondary process

Quoted 71 lines · kumaraparameshwaran rathinavel

On Mon, Jan 24, 2022 at 6:05 PM Ferruh Yigit <[email protected] <mailto:[email protected]>> wrote:
On 1/24/2022 12:12 PM, Kumara Parameshwaran wrote:
> From: Kumara Parameshwaran <[email protected] <mailto:[email protected]>> >
> When a tap device is hotplugged to primary process which in turn
> adds the device to all secondary process, the secondary process
> does a tap_mp_attach_queues, but the fds are not populated in
> the primary during the probe they are populated during the queue_setup,
> added a fix to sync the queues during rte_eth_dev_start
>
> Fixes: 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")
> Cc: [email protected] <mailto:[email protected]>
>
> Signed-off-by: Kumara Parameshwaran <[email protected] <mailto:[email protected]>> > ---
>
> v2:
> * Addressed review comments to move the function declaration and version
> map
>
Thanks for adding patch version.
> drivers/net/tap/rte_eth_tap.c | 196 +++++++++++++--------------------- > lib/ethdev/ethdev_driver.h | 17 +++
> lib/ethdev/rte_ethdev.c | 11 ++
> lib/ethdev/version.map | 2 +
> 4 files changed, 102 insertions(+), 124 deletions(-)
>
Can you please separate etdev (API) changes to another patch, so this will be a patchset with two patches,
first patch adds ethdev API
second patch is tap patch, that uses the API in the first patch
Sure, I will do it.
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c > index f1b48cae82..f6c25d7e21 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -66,7 +66,7 @@
> (TAP_GSO_MBUFS_PER_CORE * TAP_GSO_MBUF_CACHE_SIZE)
>
> /* IPC key for queue fds sync */
> -#define TAP_MP_KEY "tap_mp_sync_queues"
> +#define TAP_MP_REQ_START_RXTX "tap_mp_req_start_rxtx"
>
We said we can drop "tap_mp_sync_queues", but thinking twice, will current implementation cover following usecase:
- Primary applicaiton started with tap interface, all config, setup, start done
- Secondary app started without any parameter
Since primary already started, I think secondary fds will be wrong,
what do you think?
That is true, the fds would be unitialised, we should retain for the secondary attach case.
> #define TAP_IOV_DEFAULT_MAX 1024
>
> @@ -880,11 +880,48 @@ tap_link_set_up(struct rte_eth_dev *dev)
> return tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
> }
>
> +static int tap_mp_req_on_rxtx(struct rte_eth_dev *dev)

Quoted 2 lines

Can you please follow the coding convention:
sure, will do it.

There are more comments/questions below, ca you please check them too?

Quoted 341 lines

    static int
    tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
    {

     > +{
     > +     struct rte_mp_msg msg;
     > +     struct ipc_queues *request_param = (struct ipc_queues *)msg.param;
     > +     int err;
     > +     int fd_iterator = 0;
     > +     struct pmd_process_private *process_private = dev->process_private;
     > +     int i;
     > +
     > +     memset(&msg, 0, sizeof(msg));
     > +     strlcpy(msg.name <http://msg.name>, TAP_MP_REQ_START_RXTX, sizeof(msg.name <http://msg.name>));
     > +     strlcpy(request_param->port_name, dev->data->name, sizeof(request_param->port_name));
     > +     msg.len_param = sizeof(*request_param);
     > +     for (i = 0; i < dev->data->nb_tx_queues; i++) {
     > +             msg.fds[fd_iterator++] = process_private->txq_fds[i];
     > +             msg.num_fds++;
     > +             request_param->txq_count++;
     > +     }
     > +     for (i = 0; i < dev->data->nb_rx_queues; i++) {
     > +             msg.fds[fd_iterator++] = process_private->rxq_fds[i];
     > +             msg.num_fds++;
     > +             request_param->rxq_count++;
     > +     }
     > +
     > +     err = rte_mp_sendmsg(&msg);
     > +     if (err < 0) {
     > +             TAP_LOG(ERR, "Failed to send start req to secondary %d",
     > +                     rte_errno);
     > +             return -1;
     > +     }
     > +
     > +     return 0;
     > +}
     > +
     >   static int
     >   tap_dev_start(struct rte_eth_dev *dev)
     >   {
     >       int err, i;
     >
     > +     if (rte_eal_process_type() == RTE_PROC_PRIMARY)
     > +             tap_mp_req_on_rxtx(dev);
     > +
     >       err = tap_intr_handle_set(dev, 1);
     >       if (err)
     >               return err;
     > @@ -901,6 +938,34 @@ tap_dev_start(struct rte_eth_dev *dev)
     >       return err;
     >   }
     >
     > +static int
     > +tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused const void *peer)
     > +{

    I asked last time but I don't remember the response,
    what should be in the 'peer' variable?

     > +     struct rte_eth_dev *dev;
     > +     const struct ipc_queues *request_param =
     > +             (const struct ipc_queues *)request->param;
     > +     int fd_iterator;
     > +     int queue;
     > +     struct pmd_process_private *process_private;
     > +
     > +     dev = rte_get_eth_dev_by_name(request_param->port_name);
     > +     if (!dev) {
     > +             TAP_LOG(ERR, "Failed to get dev for %s",
     > +                     request_param->port_name);
     > +             return -1;
     > +     }
     > +     process_private = dev->process_private;
     > +     fd_iterator = 0;
     > +     TAP_LOG(DEBUG, "tap_attach rx_q:%d tx_q:%d\n", request_param->rxq_count,
     > +             request_param->txq_count);
     > +     for (queue = 0; queue < request_param->txq_count; queue++)
     > +             process_private->txq_fds[queue] = request->fds[fd_iterator++];
     > +     for (queue = 0; queue < request_param->rxq_count; queue++)
     > +             process_private->rxq_fds[queue] = request->fds[fd_iterator++];
     > +
     > +     return 0;
     > +}
     > +
     >   /* This function gets called when the current port gets stopped.
     >    */
     >   static int
     > @@ -1084,6 +1149,7 @@ tap_dev_close(struct rte_eth_dev *dev)
     >
     >       if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
     >               rte_free(dev->process_private);
     > +             rte_mp_action_unregister(TAP_MP_REQ_START_RXTX);
     >               return 0;
     >       }
     >
     > @@ -1140,8 +1206,6 @@ tap_dev_close(struct rte_eth_dev *dev)
     >               internals->ioctl_sock = -1;
     >       }
     >       rte_free(dev->process_private);
     > -     if (tap_devices_count == 1)
     > -             rte_mp_action_unregister(TAP_MP_KEY);
     >       tap_devices_count--;
     >       /*
     >        * Since TUN device has no more opened file descriptors
     > @@ -2292,113 +2356,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
     >       return ret;
     >   }
     >
     > -/* Request queue file descriptors from secondary to primary. */
     > -static int
     > -tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
     > -{
     > -     int ret;
     > -     struct timespec timeout = {.tv_sec = 1, .tv_nsec = 0};
     > -     struct rte_mp_msg request, *reply;
     > -     struct rte_mp_reply replies;
     > -     struct ipc_queues *request_param = (struct ipc_queues *)request.param;
     > -     struct ipc_queues *reply_param;
     > -     struct pmd_process_private *process_private = dev->process_private;
     > -     int queue, fd_iterator;
     > -
     > -     /* Prepare the request */
     > -     memset(&request, 0, sizeof(request));
     > -     strlcpy(request.name <http://request.name>, TAP_MP_KEY, sizeof(request.name <http://request.name>));
     > -     strlcpy(request_param->port_name, port_name,
     > -             sizeof(request_param->port_name));
     > -     request.len_param = sizeof(*request_param);
     > -     /* Send request and receive reply */
     > -     ret = rte_mp_request_sync(&request, &replies, &timeout);
     > -     if (ret < 0 || replies.nb_received != 1) {
     > -             TAP_LOG(ERR, "Failed to request queues from primary: %d",
     > -                     rte_errno);
     > -             return -1;
     > -     }
     > -     reply = &replies.msgs[0];
     > -     reply_param = (struct ipc_queues *)reply->param;
     > -     TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
     > -
     > -     /* Attach the queues from received file descriptors */
     > -     if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds) {
     > -             TAP_LOG(ERR, "Unexpected number of fds received");
     > -             return -1;
     > -     }
     > -
     > -     dev->data->nb_rx_queues = reply_param->rxq_count;
     > -     dev->data->nb_tx_queues = reply_param->txq_count;
     > -     fd_iterator = 0;
     > -     for (queue = 0; queue < reply_param->rxq_count; queue++)
     > -             process_private->rxq_fds[queue] = reply->fds[fd_iterator++];
     > -     for (queue = 0; queue < reply_param->txq_count; queue++)
     > -             process_private->txq_fds[queue] = reply->fds[fd_iterator++];
     > -     free(reply);
     > -     return 0;
     > -}
     > -
     > -/* Send the queue file descriptors from the primary process to secondary. */
     > -static int
     > -tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
     > -{
     > -     struct rte_eth_dev *dev;
     > -     struct pmd_process_private *process_private;
     > -     struct rte_mp_msg reply;
     > -     const struct ipc_queues *request_param =
     > -             (const struct ipc_queues *)request->param;
     > -     struct ipc_queues *reply_param =
     > -             (struct ipc_queues *)reply.param;
     > -     uint16_t port_id;
     > -     int queue;
     > -     int ret;
     > -
     > -     /* Get requested port */
     > -     TAP_LOG(DEBUG, "Received IPC request for %s", request_param->port_name);
     > -     ret = rte_eth_dev_get_port_by_name(request_param->port_name, &port_id);
     > -     if (ret) {
     > -             TAP_LOG(ERR, "Failed to get port id for %s",
     > -                     request_param->port_name);
     > -             return -1;
     > -     }
     > -     dev = &rte_eth_devices[port_id];
     > -     process_private = dev->process_private;
     > -
     > -     /* Fill file descriptors for all queues */
     > -     reply.num_fds = 0;
     > -     reply_param->rxq_count = 0;
     > -     if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
     > -                     RTE_MP_MAX_FD_NUM){
     > -             TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of fds");
     > -             return -1;
     > -     }
     > -
     > -     for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
     > -             reply.fds[reply.num_fds++] = process_private->rxq_fds[queue];
     > -             reply_param->rxq_count++;
     > -     }
     > -     RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
     > -
     > -     reply_param->txq_count = 0;
     > -     for (queue = 0; queue < dev->data->nb_tx_queues; queue++) {
     > -             reply.fds[reply.num_fds++] = process_private->txq_fds[queue];
     > -             reply_param->txq_count++;
     > -     }
     > -     RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
     > -
     > -     /* Send reply */
     > -     strlcpy(reply.name <http://reply.name>, request->name, sizeof(reply.name <http://reply.name>));
     > -     strlcpy(reply_param->port_name, request_param->port_name,
     > -             sizeof(reply_param->port_name));
     > -     reply.len_param = sizeof(*reply_param);
     > -     if (rte_mp_reply(&reply, peer) < 0) {
     > -             TAP_LOG(ERR, "Failed to reply an IPC request to sync queues");
     > -             return -1;
     > -     }
     > -     return 0;
     > -}
     > -
     >   /* Open a TAP interface device.
     >    */
     >   static int
     > @@ -2442,9 +2399,11 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
     >                       return -1;
     >               }
     >
     > -             ret = tap_mp_attach_queues(name, eth_dev);
     > -             if (ret != 0)
     > -                     return -1;
     > +             ret = rte_mp_action_register(TAP_MP_REQ_START_RXTX, tap_mp_req_start_rxtx);
     > +             if (ret < 0 && rte_errno != ENOTSUP)
     > +                     TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
     > +                             strerror(rte_errno));
     > +
     >               rte_eth_dev_probing_finish(eth_dev);
     >               return 0;
     >       }
     > @@ -2492,15 +2451,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
     >
     >       TAP_LOG(DEBUG, "Initializing pmd_tap for %s", name);
     >
     > -     /* Register IPC feed callback */
     > -     if (!tap_devices_count) {
     > -             ret = rte_mp_action_register(TAP_MP_KEY, tap_mp_sync_queues);
     > -             if (ret < 0 && rte_errno != ENOTSUP) {
     > -                     TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
     > -                             strerror(rte_errno));
     > -                     goto leave;
     > -             }
     > -     }
     >       tap_devices_count++;
     >       tap_devices_count_increased = 1;
     >       ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,
     > @@ -2511,8 +2461,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
     >               TAP_LOG(ERR, "Failed to create pmd for %s as %s",
     >                       name, tap_name);
     >               if (tap_devices_count_increased == 1) {
     > -                     if (tap_devices_count == 1)
     > -                             rte_mp_action_unregister(TAP_MP_KEY);
     >                       tap_devices_count--;
     >               }
     >       }
     > diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
     > index d95605a355..a08991bcdf 100644
     > --- a/lib/ethdev/ethdev_driver.h
     > +++ b/lib/ethdev/ethdev_driver.h
     > @@ -1629,6 +1629,23 @@ rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue,
     >                               struct rte_hairpin_peer_info *peer_info,
     >                               uint32_t direction);
     >
     > +/**

    Please add '@internal' tag into doxygen comment.

     > +* Get rte_eth_dev from device name. The device name should be specified
     > +* as below:
     > +* - PCIe address (Domain:Bus:Device.Function), for example- 0000:2:00.0
     > +* - SoC device name, for example- fsl-gmac0
     > +* - vdev dpdk name, for example- net_[pcap0|null0|tap0]
     > +*
     > +* @param name
     > +*  pci address or name of the device
     > +* @return
     > +*   - rte_eth_dev if successful
     > +*   - NULL on failure
     > +*/
     > +__rte_internal
     > +struct rte_eth_dev*
     > +rte_get_eth_dev_by_name(const char *name);

    As the API name, better to start with 'rte_eth_' prefix to be consistent with
    rest of the APIs.
    I suggest 'rte_eth_dev_get_by_name' but feel free to chose better one.

     > +
     >   /**
     >    * @internal
     >    * Reset the current queue state and configuration to disconnect (unbind) it
     > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
     > index a1d475a292..9192b0d664 100644
     > --- a/lib/ethdev/rte_ethdev.c
     > +++ b/lib/ethdev/rte_ethdev.c
     > @@ -894,6 +894,17 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
     >       return -ENODEV;
     >   }
     >
     > +struct rte_eth_dev *
     > +rte_get_eth_dev_by_name(const char *name)
     > +{
     > +     uint16_t pid;
     > +
     > +     if (rte_eth_dev_get_port_by_name(name, &pid))
     > +             return NULL;
     > +
     > +     return &rte_eth_devices[pid];
     > +}
     > +
     >   static int
     >   eth_err(uint16_t port_id, int ret)
     >   {
     > diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
     > index c2fb0669a4..7e3797189b 100644
     > --- a/lib/ethdev/version.map
     > +++ b/lib/ethdev/version.map
     > @@ -256,6 +256,7 @@ EXPERIMENTAL {
     >       rte_flow_flex_item_create;
     >       rte_flow_flex_item_release;
     >       rte_flow_pick_transfer_proxy;
     > +

    This is unintendent change.

     >   };
     >
     >   INTERNAL {
     > @@ -282,4 +283,5 @@ INTERNAL {
     >       rte_eth_representor_id_get;
     >       rte_eth_switch_domain_alloc;
     >       rte_eth_switch_domain_free;
     > +     rte_get_eth_dev_by_name;

    Please add in a sorted way.

     >   };

Kumara Parameshwaran

2022-01-25 09:45 UTC #6 lore raw

Re: [PATCH v2] net/tap: fix to populate fds in secondary process

In reply to Ferruh Yigit

________________________________
From: Ferruh Yigit <[email protected]>
Sent: 24 January 2022 18:05
To: Kumara Parameshwaran <[email protected]>; [email protected] <[email protected]>
Cc: Kumara Parameshwaran <[email protected]>; [email protected] <[email protected]>
Subject: Re: [PATCH v2] net/tap: fix to populate fds in secondary process

Quoted 11 lines · Kumara Parameshwaran

From: Kumara Parameshwaran <[email protected]>
When a tap device is hotplugged to primary process which in turn adds the device to all secondary process, the secondary process does a tap_mp_attach_queues, but the fds are not populated in the primary during the probe they are populated during the queue_setup, added a fix to sync the queues during rte_eth_dev_start
Fixes 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")
Cc [email protected]

Quoted 7 lines

Signed-off-by Kumara Parameshwaran <[email protected]>
---
v2:
* Addressed review comments to move the function declaration and version map

Thanks for adding patch version.

Quoted 6 lines

  drivers/net/tap/rte_eth_tap.c | 196 +++++++++++++---------------------
  lib/ethdev/ethdev_driver.h    |  17 +++
  lib/ethdev/rte_ethdev.c       |  11 ++
  lib/ethdev/version.map        |   2 +
  4 files changed, 102 insertions(+), 124 deletions(-)

Can you please separate etdev (API) changes to another patch, so this will be a patchset with two patches,
first patch adds ethdev API
second patch is tap patch, that uses the API in the first patch

Sure, will do it.

drivers/net/tap/rte_eth_tap.c line 70

Quoted 11 lines

drivers/net/tap/rte_eth_tap.c +1 -1

index f1b48cae82..f6c25d7e21 100644
@@ -66,7 +66,7 @@
       (TAP_GSO_MBUFS_PER_CORE * TAP_GSO_MBUF_CACHE_SIZE)
 
  /* IPC key for queue fds sync */
-#define TAP_MP_KEY "tap_mp_sync_queues"
+#define TAP_MP_REQ_START_RXTX "tap_mp_req_start_rxtx"

We said we can drop "tap_mp_sync_queues", but thinking twice, will current implementation cover following usecase:

- Primary applicaiton started with tap interface, all config, setup, start done
- Secondary app started without any parameter

Since primary already started, I think secondary fds will be wrong, what do you think?
Yes, that is true. We should retain the current behaviour of start for the attach.

drivers/net/tap/rte_eth_tap.c tap_link_set_up(struct rte_eth_dev *dev) · line 883

Quoted 7 lines

drivers/net/tap/rte_eth_tap.c +1 -0

  #define TAP_IOV_DEFAULT_MAX 1024

@@ -880,11 +880,48 @@ tap_link_set_up(struct rte_eth_dev *dev)
       return tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
  }
 
+static int tap_mp_req_on_rxtx(struct rte_eth_dev *dev)

Can you please follow the coding convention:

static int
tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
{

Quoted 45 lines

drivers/net/tap/rte_eth_tap.c +36 -0

+{
+     struct rte_mp_msg msg;
+     struct ipc_queues *request_param = (struct ipc_queues *)msg.param;
+     int err;
+     int fd_iterator = 0;
+     struct pmd_process_private *process_private = dev->process_private;
+     int i;
+
+     memset(&msg, 0, sizeof(msg));
+     strlcpy(msg.name, TAP_MP_REQ_START_RXTX, sizeof(msg.name));
+     strlcpy(request_param->port_name, dev->data->name, sizeof(request_param->port_name));
+     msg.len_param = sizeof(*request_param);
+     for (i = 0; i < dev->data->nb_tx_queues; i++) {
+             msg.fds[fd_iterator++] = process_private->txq_fds[i];
+             msg.num_fds++;
+             request_param->txq_count++;
+     }
+     for (i = 0; i < dev->data->nb_rx_queues; i++) {
+             msg.fds[fd_iterator++] = process_private->rxq_fds[i];
+             msg.num_fds++;
+             request_param->rxq_count++;
+     }
+
+     err = rte_mp_sendmsg(&msg);
+     if (err < 0) {
+             TAP_LOG(ERR, "Failed to send start req to secondary %d",
+                     rte_errno);
+             return -1;
+     }
+
+     return 0;
+}
+
  static int
  tap_dev_start(struct rte_eth_dev *dev)
  {
       int err, i;
 
+     if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+             tap_mp_req_on_rxtx(dev);
+
       err = tap_intr_handle_set(dev, 1);
       if (err)
               return err;

drivers/net/tap/rte_eth_tap.c tap_dev_start(struct rte_eth_dev *dev) · line 943

Quoted 6 lines

drivers/net/tap/rte_eth_tap.c +3 -0

@@ -901,6 +938,34 @@ tap_dev_start(struct rte_eth_dev *dev)
       return err;
  }
 
+static int
+tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused const void *peer)
+{

I asked last time but I don't remember the response, what should be in the 'peer' variable?

Quoted 204 lines

drivers/net/tap/rte_eth_tap.c +31 ~~-123~~

+     struct rte_eth_dev *dev;
+     const struct ipc_queues *request_param =
+             (const struct ipc_queues *)request->param;
+     int fd_iterator;
+     int queue;
+     struct pmd_process_private *process_private;
+
+     dev = rte_get_eth_dev_by_name(request_param->port_name);
+     if (!dev) {
+             TAP_LOG(ERR, "Failed to get dev for %s",
+                     request_param->port_name);
+             return -1;
+     }
+     process_private = dev->process_private;
+     fd_iterator = 0;
+     TAP_LOG(DEBUG, "tap_attach rx_q:%d tx_q:%d\n", request_param->rxq_count,
+             request_param->txq_count);
+     for (queue = 0; queue < request_param->txq_count; queue++)
+             process_private->txq_fds[queue] = request->fds[fd_iterator++];
+     for (queue = 0; queue < request_param->rxq_count; queue++)
+             process_private->rxq_fds[queue] = request->fds[fd_iterator++];
+
+     return 0;
+}
+
  /* This function gets called when the current port gets stopped.
   */
  static int

@@ -1084,6 +1149,7 @@ tap_dev_close(struct rte_eth_dev *dev)
 
       if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
               rte_free(dev->process_private);
+             rte_mp_action_unregister(TAP_MP_REQ_START_RXTX);
               return 0;
       }

@@ -1140,8 +1206,6 @@ tap_dev_close(struct rte_eth_dev *dev)
               internals->ioctl_sock = -1;
       }
       rte_free(dev->process_private);
-     if (tap_devices_count == 1)
-             rte_mp_action_unregister(TAP_MP_KEY);
       tap_devices_count--;
       /*
        * Since TUN device has no more opened file descriptors

@@ -2292,113 +2356,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
       return ret;
  }
 
-/* Request queue file descriptors from secondary to primary. */
-static int
-tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
-{
-     int ret;
-     struct timespec timeout = {.tv_sec = 1, .tv_nsec = 0};
-     struct rte_mp_msg request, *reply;
-     struct rte_mp_reply replies;
-     struct ipc_queues *request_param = (struct ipc_queues *)request.param;
-     struct ipc_queues *reply_param;
-     struct pmd_process_private *process_private = dev->process_private;
-     int queue, fd_iterator;
-
-     /* Prepare the request */
-     memset(&request, 0, sizeof(request));
-     strlcpy(request.name, TAP_MP_KEY, sizeof(request.name));
-     strlcpy(request_param->port_name, port_name,
-             sizeof(request_param->port_name));
-     request.len_param = sizeof(*request_param);
-     /* Send request and receive reply */
-     ret = rte_mp_request_sync(&request, &replies, &timeout);
-     if (ret < 0 || replies.nb_received != 1) {
-             TAP_LOG(ERR, "Failed to request queues from primary: %d",
-                     rte_errno);
-             return -1;
-     }
-     reply = &replies.msgs[0];
-     reply_param = (struct ipc_queues *)reply->param;
-     TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
-
-     /* Attach the queues from received file descriptors */
-     if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds) {
-             TAP_LOG(ERR, "Unexpected number of fds received");
-             return -1;
-     }
-
-     dev->data->nb_rx_queues = reply_param->rxq_count;
-     dev->data->nb_tx_queues = reply_param->txq_count;
-     fd_iterator = 0;
-     for (queue = 0; queue < reply_param->rxq_count; queue++)
-             process_private->rxq_fds[queue] = reply->fds[fd_iterator++];
-     for (queue = 0; queue < reply_param->txq_count; queue++)
-             process_private->txq_fds[queue] = reply->fds[fd_iterator++];
-     free(reply);
-     return 0;
-}
-
-/* Send the queue file descriptors from the primary process to secondary. */
-static int
-tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
-{
-     struct rte_eth_dev *dev;
-     struct pmd_process_private *process_private;
-     struct rte_mp_msg reply;
-     const struct ipc_queues *request_param =
-             (const struct ipc_queues *)request->param;
-     struct ipc_queues *reply_param =
-             (struct ipc_queues *)reply.param;
-     uint16_t port_id;
-     int queue;
-     int ret;
-
-     /* Get requested port */
-     TAP_LOG(DEBUG, "Received IPC request for %s", request_param->port_name);
-     ret = rte_eth_dev_get_port_by_name(request_param->port_name, &port_id);
-     if (ret) {
-             TAP_LOG(ERR, "Failed to get port id for %s",
-                     request_param->port_name);
-             return -1;
-     }
-     dev = &rte_eth_devices[port_id];
-     process_private = dev->process_private;
-
-     /* Fill file descriptors for all queues */
-     reply.num_fds = 0;
-     reply_param->rxq_count = 0;
-     if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
-                     RTE_MP_MAX_FD_NUM){
-             TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of fds");
-             return -1;
-     }
-
-     for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
-             reply.fds[reply.num_fds++] = process_private->rxq_fds[queue];
-             reply_param->rxq_count++;
-     }
-     RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
-
-     reply_param->txq_count = 0;
-     for (queue = 0; queue < dev->data->nb_tx_queues; queue++) {
-             reply.fds[reply.num_fds++] = process_private->txq_fds[queue];
-             reply_param->txq_count++;
-     }
-     RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
-
-     /* Send reply */
-     strlcpy(reply.name, request->name, sizeof(reply.name));
-     strlcpy(reply_param->port_name, request_param->port_name,
-             sizeof(reply_param->port_name));
-     reply.len_param = sizeof(*reply_param);
-     if (rte_mp_reply(&reply, peer) < 0) {
-             TAP_LOG(ERR, "Failed to reply an IPC request to sync queues");
-             return -1;
-     }
-     return 0;
-}
-
  /* Open a TAP interface device.
   */
  static int

@@ -2442,9 +2399,11 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
                       return -1;
               }
 
-             ret = tap_mp_attach_queues(name, eth_dev);
-             if (ret != 0)
-                     return -1;
+             ret = rte_mp_action_register(TAP_MP_REQ_START_RXTX, tap_mp_req_start_rxtx);
+             if (ret < 0 && rte_errno != ENOTSUP)
+                     TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
+                             strerror(rte_errno));
+
               rte_eth_dev_probing_finish(eth_dev);
               return 0;
       }

@@ -2492,15 +2451,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 
       TAP_LOG(DEBUG, "Initializing pmd_tap for %s", name);
 
-     /* Register IPC feed callback */
-     if (!tap_devices_count) {
-             ret = rte_mp_action_register(TAP_MP_KEY, tap_mp_sync_queues);
-             if (ret < 0 && rte_errno != ENOTSUP) {
-                     TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
-                             strerror(rte_errno));
-                     goto leave;
-             }
-     }
       tap_devices_count++;
       tap_devices_count_increased = 1;
       ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,

@@ -2511,8 +2461,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
               TAP_LOG(ERR, "Failed to create pmd for %s as %s",
                       name, tap_name);
               if (tap_devices_count_increased == 1) {
-                     if (tap_devices_count == 1)
-                             rte_mp_action_unregister(TAP_MP_KEY);
                       tap_devices_count--;
               }
       }

lib/ethdev/ethdev_driver.h +0 -0

index d95605a355..a08991bcdf 100644

lib/ethdev/ethdev_driver.h rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue, · line 1632

Quoted 4 lines

lib/ethdev/ethdev_driver.h +1 -0

@@ -1629,6 +1629,23 @@ rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue,
                               struct rte_hairpin_peer_info *peer_info,
                               uint32_t direction);
 
+/**

Please add '@internal' tag into doxygen comment.

Quoted 5 lines

lib/ethdev/ethdev_driver.h +5 -0

+* Get rte_eth_dev from device name. The device name should be specified
+* as below:
+* - PCIe address (Domain:Bus:Device.Function), for example- 0000:2:00.0
+* - SoC device name, for example- fsl-gmac0
+* - vdev dpdk name, for example- net_[pcap0|null0|tap0]

lib/ethdev/ethdev_driver.h rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue, · line 1647

Quoted 10 lines

lib/ethdev/ethdev_driver.h +10 -0

+*
+* @param name
+*  pci address or name of the device
+* @return
+*   - rte_eth_dev if successful
+*   - NULL on failure
+*/
+__rte_internal
+struct rte_eth_dev*
+rte_get_eth_dev_by_name(const char *name);

As the API name, better to start with 'rte_eth_' prefix to be consistent with rest of the APIs.
I suggest 'rte_eth_dev_get_by_name' but feel free to chose better one.

Quoted 31 lines

lib/ethdev/ethdev_driver.h +1 -0

+
  /**
   * @internal
   * Reset the current queue state and configuration to disconnect (unbind) it

lib/ethdev/rte_ethdev.c +11 -0

index a1d475a292..9192b0d664 100644
@@ -894,6 +894,17 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
       return -ENODEV;
  }
 
+struct rte_eth_dev *
+rte_get_eth_dev_by_name(const char *name)
+{
+     uint16_t pid;
+
+     if (rte_eth_dev_get_port_by_name(name, &pid))
+             return NULL;
+
+     return &rte_eth_devices[pid];
+}
+
  static int
  eth_err(uint16_t port_id, int ret)
  {

lib/ethdev/version.map +0 -0

index c2fb0669a4..7e3797189b 100644

lib/ethdev/version.map EXPERIMENTAL { · line 259

Quoted 4 lines

lib/ethdev/version.map +1 -0

@@ -256,6 +256,7 @@ EXPERIMENTAL {
       rte_flow_flex_item_create;
       rte_flow_flex_item_release;
       rte_flow_pick_transfer_proxy;
+

This is unintendent change.

lib/ethdev/version.map INTERNAL { · line 286

Quoted 8 lines

lib/ethdev/version.map +1 -0

  };
 
  INTERNAL {

@@ -282,4 +283,5 @@ INTERNAL {
       rte_eth_representor_id_get;
       rte_eth_switch_domain_alloc;
       rte_eth_switch_domain_free;
+     rte_get_eth_dev_by_name;

Please add in a sorted way.

Quoted 1 line

lib/ethdev/version.map +0 -0
  };

kumaraparameshwaran rathinavel

author

2022-01-28 09:55 UTC #7 lore raw

Re: [PATCH v2] net/tap: fix to populate fds in secondary process

In reply to Ferruh Yigit

Hi Ferruh,

Please find the consolidated comments for the review. I had missed answering a few of them in my previous email.

Quoted 443 lines · Ferruh Yigit

Quoted 11 lines · Kumara Parameshwaran
From: Kumara Parameshwaran <[email protected]>
When a tap device is hotplugged to primary process which in turn adds the device to all secondary process, the secondary process does a tap_mp_attach_queues, but the fds are not populated in the primary during the probe they are populated during the queue_setup, added a fix to sync the queues during rte_eth_dev_start
Fixes 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")
Cc [email protected]
Quoted 7 lines
Signed-off-by Kumara Parameshwaran <[email protected]>
---
v2:
* Addressed review comments to move the function declaration and version map
Thanks for adding patch version.
Quoted 6 lines
  drivers/net/tap/rte_eth_tap.c | 196 +++++++++++++---------------------
  lib/ethdev/ethdev_driver.h    |  17 +++
  lib/ethdev/rte_ethdev.c       |  11 ++
  lib/ethdev/version.map        |   2 +
  4 files changed, 102 insertions(+), 124 deletions(-)
Can you please separate etdev (API) changes to another patch, so this will be a patchset
with two patches,
first patch adds ethdev API
second patch is tap patch, that uses the API in the first patch Sure, will do it.
Review
Quoted 1 line
(quoted diff) +0 -0
b/drivers/net/tap/rte_eth_tap.c
drivers/net/tap/rte_eth_tap.c line 70
Quoted 10 lines
index f1b48cae82..f6c25d7e21 100644
drivers/net/tap/rte_eth_tap.c +1 -1
@@ -66,7 +66,7 @@
      (TAP_GSO_MBUFS_PER_CORE * TAP_GSO_MBUF_CACHE_SIZE)
 
  /* IPC key for queue fds sync */
-#define TAP_MP_KEY "tap_mp_sync_queues"
+#define TAP_MP_REQ_START_RXTX "tap_mp_req_start_rxtx"
 
We said we can drop "tap_mp_sync_queues", but thinking twice, will current implementation cover following usecase:
- Primary applicaiton started with tap interface, all config, setup, start done
- Secondary app started without any parameter
Since primary already started, I think secondary fds will be wrong, what do you think?
Yes, will retain the old behaviour.
drivers/net/tap/rte_eth_tap.c tap_link_set_up(struct rte_eth_dev *dev) · line 883
Quoted 7 lines
drivers/net/tap/rte_eth_tap.c +1 -0
  #define TAP_IOV_DEFAULT_MAX 1024
 
@@ -880,11 +880,48 @@ tap_link_set_up(struct rte_eth_dev *dev)
      return tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
  }
 
+static int tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
Can you please follow the coding convention:
Sure, will follow it.
static int
tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
{
drivers/net/tap/rte_eth_tap.c tap_link_set_up(struct rte_eth_dev *dev)
Quoted 11 lines
drivers/net/tap/rte_eth_tap.c +11 -0
+{
+     struct rte_mp_msg msg;
+     struct ipc_queues *request_param = (struct ipc_queues *)msg.param;
+     int err;
+     int fd_iterator = 0;
+     struct pmd_process_private *process_private = dev->process_private;
+     int i;
+
+     memset(&msg, 0, sizeof(msg));
+     strlcpy(msg.name, TAP_MP_REQ_START_RXTX, sizeof(msg.name));
+     strlcpy(request_param->port_name, dev->data->name,
sizeof(request_param->port_name));
Quoted 34 lines
drivers/net/tap/rte_eth_tap.c +25 -0
+     msg.len_param = sizeof(*request_param);
+     for (i = 0; i < dev->data->nb_tx_queues; i++) {
+             msg.fds[fd_iterator++] = process_private->txq_fds[i];
+             msg.num_fds++;
+             request_param->txq_count++;
+     }
+     for (i = 0; i < dev->data->nb_rx_queues; i++) {
+             msg.fds[fd_iterator++] = process_private->rxq_fds[i];
+             msg.num_fds++;
+             request_param->rxq_count++;
+     }
+
+     err = rte_mp_sendmsg(&msg);
+     if (err < 0) {
+             TAP_LOG(ERR, "Failed to send start req to secondary %d",
+                     rte_errno);
+             return -1;
+     }
+
+     return 0;
+}
+
  static int
  tap_dev_start(struct rte_eth_dev *dev)
  {
      int err, i;
 
+     if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+             tap_mp_req_on_rxtx(dev);
+
      err = tap_intr_handle_set(dev, 1);
      if (err)
              return err;
drivers/net/tap/rte_eth_tap.c tap_dev_start(struct rte_eth_dev *dev) · line 942
Quoted 5 lines
drivers/net/tap/rte_eth_tap.c +2 -0
@@ -901,6 +938,34 @@ tap_dev_start(struct rte_eth_dev *dev)
      return err;
  }
 
+static int
+tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused
const void *peer)
drivers/net/tap/rte_eth_tap.c tap_dev_start(struct rte_eth_dev *dev) · line 943
Quoted 1 line
drivers/net/tap/rte_eth_tap.c +1 -0
+{
I asked last time but I don't remember the response,
what should be in the 'peer' variable?
The peer variable is currently unused. The primary does not expect any response from secondary processes. Should we change the model to rte_mp_request_sync rather than rte_mp_send_msg?
Quoted 6 lines
drivers/net/tap/rte_eth_tap.c +6 -0
+     struct rte_eth_dev *dev;
+     const struct ipc_queues *request_param =
+             (const struct ipc_queues *)request->param;
+     int fd_iterator;
+     int queue;
+     struct pmd_process_private *process_private;
drivers/net/tap/rte_eth_tap.c tap_dev_start(struct rte_eth_dev *dev)
Quoted 10 lines
drivers/net/tap/rte_eth_tap.c +10 -0
+
+     dev = rte_get_eth_dev_by_name(request_param->port_name);
+     if (!dev) {
+             TAP_LOG(ERR, "Failed to get dev for %s",
+                     request_param->port_name);
+             return -1;
+     }
+     process_private = dev->process_private;
+     fd_iterator = 0;
+     TAP_LOG(DEBUG, "tap_attach rx_q:%d tx_q:%d\n",
request_param->rxq_count,
drivers/net/tap/rte_eth_tap.c tap_dev_start(struct rte_eth_dev *dev)
Quoted 3 lines
drivers/net/tap/rte_eth_tap.c +3 -0
+             request_param->txq_count);
+     for (queue = 0; queue < request_param->txq_count; queue++)
+             process_private->txq_fds[queue] =
request->fds[fd_iterator++];
drivers/net/tap/rte_eth_tap.c tap_dev_start(struct rte_eth_dev *dev) · line 964
Quoted 2 lines
drivers/net/tap/rte_eth_tap.c +2 -0
+     for (queue = 0; queue < request_param->rxq_count; queue++)
+             process_private->rxq_fds[queue] =
request->fds[fd_iterator++];
Quoted 27 lines
drivers/net/tap/rte_eth_tap.c +5 -2
+
+     return 0;
+}
+
  /* This function gets called when the current port gets stopped.
   */
  static int
@@ -1084,6 +1149,7 @@ tap_dev_close(struct rte_eth_dev *dev)
 
      if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
              rte_free(dev->process_private);
+             rte_mp_action_unregister(TAP_MP_REQ_START_RXTX);
              return 0;
      }
 
@@ -1140,8 +1206,6 @@ tap_dev_close(struct rte_eth_dev *dev)
              internals->ioctl_sock = -1;
      }
      rte_free(dev->process_private);
-     if (tap_devices_count == 1)
-             rte_mp_action_unregister(TAP_MP_KEY);
      tap_devices_count--;
      /*
       * Since TUN device has no more opened file descriptors
@@ -2292,113 +2356,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
      return ret;
  }
drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev) · line 2303
Quoted 10 lines
drivers/net/tap/rte_eth_tap.c +0 -9
 
-/* Request queue file descriptors from secondary to primary. */
-static int
-tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
-{
-     int ret;
-     struct timespec timeout = {.tv_sec = 1, .tv_nsec = 0};
-     struct rte_mp_msg request, *reply;
-     struct rte_mp_reply replies;
-     struct ipc_queues *request_param = (struct ipc_queues
*)request.param;
Quoted 10 lines
drivers/net/tap/rte_eth_tap.c +0 ~~-10~~
-     struct ipc_queues *reply_param;
-     struct pmd_process_private *process_private = dev->process_private;
-     int queue, fd_iterator;
-
-     /* Prepare the request */
-     memset(&request, 0, sizeof(request));
-     strlcpy(request.name, TAP_MP_KEY, sizeof(request.name));
-     strlcpy(request_param->port_name, port_name,
-             sizeof(request_param->port_name));
-     request.len_param = sizeof(*request_param);
drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev)
Quoted 10 lines
drivers/net/tap/rte_eth_tap.c +0 ~~-10~~
-     /* Send request and receive reply */
-     ret = rte_mp_request_sync(&request, &replies, &timeout);
-     if (ret < 0 || replies.nb_received != 1) {
-             TAP_LOG(ERR, "Failed to request queues from primary: %d",
-                     rte_errno);
-             return -1;
-     }
-     reply = &replies.msgs[0];
-     reply_param = (struct ipc_queues *)reply->param;
-     TAP_LOG(DEBUG, "Received IPC reply for %s",
reply_param->port_name);
drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev)
Quoted 3 lines
drivers/net/tap/rte_eth_tap.c +0 -3
-
-     /* Attach the queues from received file descriptors */
-     if (reply_param->rxq_count + reply_param->txq_count !=
reply->num_fds) {
drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev)
Quoted 9 lines
drivers/net/tap/rte_eth_tap.c +0 -9
-             TAP_LOG(ERR, "Unexpected number of fds received");
-             return -1;
-     }
-
-     dev->data->nb_rx_queues = reply_param->rxq_count;
-     dev->data->nb_tx_queues = reply_param->txq_count;
-     fd_iterator = 0;
-     for (queue = 0; queue < reply_param->rxq_count; queue++)
-             process_private->rxq_fds[queue] =
reply->fds[fd_iterator++];
drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev) · line 2337
Quoted 2 lines
drivers/net/tap/rte_eth_tap.c +0 -2
-     for (queue = 0; queue < reply_param->txq_count; queue++)
-             process_private->txq_fds[queue] =
reply->fds[fd_iterator++];
drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev)
Quoted 5 lines
drivers/net/tap/rte_eth_tap.c +0 -5
-     free(reply);
-     return 0;
-}
-
-/* Send the queue file descriptors from the primary process to
secondary. */
Quoted 6 lines
drivers/net/tap/rte_eth_tap.c +0 -6
-static int
-tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
-{
-     struct rte_eth_dev *dev;
-     struct pmd_process_private *process_private;
-     struct rte_mp_msg reply;
drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev)
Quoted 10 lines
drivers/net/tap/rte_eth_tap.c +0 ~~-10~~
-     const struct ipc_queues *request_param =
-             (const struct ipc_queues *)request->param;
-     struct ipc_queues *reply_param =
-             (struct ipc_queues *)reply.param;
-     uint16_t port_id;
-     int queue;
-     int ret;
-
-     /* Get requested port */
-     TAP_LOG(DEBUG, "Received IPC request for %s",
request_param->port_name);
drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev) · line 2359
Quoted 1 line
drivers/net/tap/rte_eth_tap.c +0 -1
-     ret = rte_eth_dev_get_port_by_name(request_param->port_name,
&port_id);
Quoted 4 lines
drivers/net/tap/rte_eth_tap.c +0 -4
-     if (ret) {
-             TAP_LOG(ERR, "Failed to get port id for %s",
-                     request_param->port_name);
-             return -1;
drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev)
Quoted 10 lines
drivers/net/tap/rte_eth_tap.c +0 ~~-10~~
-     }
-     dev = &rte_eth_devices[port_id];
-     process_private = dev->process_private;
-
-     /* Fill file descriptors for all queues */
-     reply.num_fds = 0;
-     reply_param->rxq_count = 0;
-     if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
-                     RTE_MP_MAX_FD_NUM){
-             TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of
fds");
drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev)
Quoted 5 lines
drivers/net/tap/rte_eth_tap.c +0 -5
-             return -1;
-     }
-
-     for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
-             reply.fds[reply.num_fds++] =
process_private->rxq_fds[queue];
drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev)
Quoted 7 lines
drivers/net/tap/rte_eth_tap.c +0 -7
-             reply_param->rxq_count++;
-     }
-     RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
-
-     reply_param->txq_count = 0;
-     for (queue = 0; queue < dev->data->nb_tx_queues; queue++) {
-             reply.fds[reply.num_fds++] =
process_private->txq_fds[queue];
drivers/net/tap/rte_eth_tap.c rte_pmd_tun_probe(struct rte_vdev_device *dev)
Quoted 11 lines
drivers/net/tap/rte_eth_tap.c +0 ~~-11~~
-             reply_param->txq_count++;
-     }
-     RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
-
-     /* Send reply */
-     strlcpy(reply.name, request->name, sizeof(reply.name));
-     strlcpy(reply_param->port_name, request_param->port_name,
-             sizeof(reply_param->port_name));
-     reply.len_param = sizeof(*reply_param);
-     if (rte_mp_reply(&reply, peer) < 0) {
-             TAP_LOG(ERR, "Failed to reply an IPC request to sync
queues");
Quoted 9 lines
drivers/net/tap/rte_eth_tap.c +0 -5
-             return -1;
-     }
-     return 0;
-}
-
  /* Open a TAP interface device.
   */
  static int
drivers/net/tap/rte_eth_tap.c rte_pmd_tap_probe(struct rte_vdev_device *dev) · line 2402
Quoted 7 lines
drivers/net/tap/rte_eth_tap.c +1 -3
@@ -2442,9 +2399,11 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
                      return -1;
              }
 
-             ret = tap_mp_attach_queues(name, eth_dev);
-             if (ret != 0)
-                     return -1;
+             ret = rte_mp_action_register(TAP_MP_REQ_START_RXTX,
tap_mp_req_start_rxtx);
drivers/net/tap/rte_eth_tap.c rte_pmd_tap_probe(struct rte_vdev_device *dev) · line 2404
Quoted 2 lines
drivers/net/tap/rte_eth_tap.c +2 -0
+             if (ret < 0 && rte_errno != ENOTSUP)
+                     TAP_LOG(ERR, "tap: Failed to register IPC
callback: %s",
drivers/net/tap/rte_eth_tap.c rte_pmd_tap_probe(struct rte_vdev_device *dev) · line 2497
Quoted 12 lines
drivers/net/tap/rte_eth_tap.c +2 -3
+                             strerror(rte_errno));
+
              rte_eth_dev_probing_finish(eth_dev);
              return 0;
      }
@@ -2492,15 +2451,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 
      TAP_LOG(DEBUG, "Initializing pmd_tap for %s", name);
 
-     /* Register IPC feed callback */
-     if (!tap_devices_count) {
-             ret = rte_mp_action_register(TAP_MP_KEY,
tap_mp_sync_queues);
drivers/net/tap/rte_eth_tap.c rte_pmd_tap_probe(struct rte_vdev_device *dev) · line 2499
Quoted 2 lines
drivers/net/tap/rte_eth_tap.c +0 -2
-             if (ret < 0 && rte_errno != ENOTSUP) {
-                     TAP_LOG(ERR, "tap: Failed to register IPC
callback: %s",
lib/ethdev/ethdev_driver.h rte_eth_hairpin_queue_peer_bind(uint16_t
Quoted 21 lines
drivers/net/tap/rte_eth_tap.c +0 -6
-                             strerror(rte_errno));
-                     goto leave;
-             }
-     }
      tap_devices_count++;
      tap_devices_count_increased = 1;
      ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,
@@ -2511,8 +2461,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
              TAP_LOG(ERR, "Failed to create pmd for %s as %s",
                      name, tap_name);
              if (tap_devices_count_increased == 1) {
-                     if (tap_devices_count == 1)
-                             rte_mp_action_unregister(TAP_MP_KEY);
                      tap_devices_count--;
              }
      }
lib/ethdev/ethdev_driver.h +0 -0
index d95605a355..a08991bcdf 100644
@@ -1629,6 +1629,23 @@ rte_eth_hairpin_queue_peer_bind(uint16_t
cur_port, uint16_t cur_queue,
lib/ethdev/ethdev_driver.h rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue, · line 1632
Quoted 4 lines
lib/ethdev/ethdev_driver.h +1 -0
                              struct rte_hairpin_peer_info *peer_info,
                              uint32_t direction);
 
+/**
Please add '@internal' tag into doxygen comment.
Sure, will do it.
Quoted 5 lines
lib/ethdev/ethdev_driver.h +5 -0
+* Get rte_eth_dev from device name. The device name should be specified
+* as below:
+* - PCIe address (Domain:Bus:Device.Function), for example- 0000:2:00.0
+* - SoC device name, for example- fsl-gmac0
+* - vdev dpdk name, for example- net_[pcap0|null0|tap0]
lib/ethdev/ethdev_driver.h rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue, · line 1647
Quoted 10 lines
lib/ethdev/ethdev_driver.h +10 -0
+*
+* @param name
+*  pci address or name of the device
+* @return
+*   - rte_eth_dev if successful
+*   - NULL on failure
+*/
+__rte_internal
+struct rte_eth_dev*
+rte_get_eth_dev_by_name(const char *name);
As the API name, better to start with 'rte_eth_' prefix to be consistent with
rest of the APIs.
I suggest 'rte_eth_dev_get_by_name' but feel free to chose better one. Sure, will change it.
lib/ethdev/ethdev_driver.h rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue,
Quoted 4 lines
lib/ethdev/ethdev_driver.h +1 -0
+
  /**
   * @internal
   * Reset the current queue state and configuration to disconnect
(unbind) it
lib/ethdev/rte_ethdev.c rte_eth_dev_get_port_by_name(const char *name,
Quoted 5 lines
lib/ethdev/rte_ethdev.c +0 -0
index a1d475a292..9192b0d664 100644
@@ -894,6 +894,17 @@ rte_eth_dev_get_port_by_name(const char *name,
uint16_t *port_id)
Quoted 22 lines
lib/ethdev/rte_ethdev.c +11 -0
      return -ENODEV;
  }
 
+struct rte_eth_dev *
+rte_get_eth_dev_by_name(const char *name)
+{
+     uint16_t pid;
+
+     if (rte_eth_dev_get_port_by_name(name, &pid))
+             return NULL;
+
+     return &rte_eth_devices[pid];
+}
+
  static int
  eth_err(uint16_t port_id, int ret)
  {
lib/ethdev/version.map +0 -0
index c2fb0669a4..7e3797189b 100644
lib/ethdev/version.map EXPERIMENTAL { · line 259
Quoted 4 lines
lib/ethdev/version.map +1 -0
@@ -256,6 +256,7 @@ EXPERIMENTAL {
      rte_flow_flex_item_create;
      rte_flow_flex_item_release;
      rte_flow_pick_transfer_proxy;
+
This is unintendent change.
Will remove it.
lib/ethdev/version.map INTERNAL { · line 286
Quoted 8 lines
lib/ethdev/version.map +1 -0
  };
 
  INTERNAL {
@@ -282,4 +283,5 @@ INTERNAL {
      rte_eth_representor_id_get;
      rte_eth_switch_domain_alloc;
      rte_eth_switch_domain_free;
+     rte_get_eth_dev_by_name;
Please add in a sorted way.
Sure.
Quoted 1 line
lib/ethdev/version.map +0 -0
  };

Ferruh Yigit

2022-01-28 12:55 UTC #8 lore raw

Re: [PATCH v2] net/tap: fix to populate fds in secondary process

Quoted 7 lines · kumaraparameshwaran rathinavel

drivers/net/tap/rte_eth_tap.c tap_dev_start(struct rte_eth_dev *dev) · line 943
Quoted 3 lines
drivers/net/tap/rte_eth_tap.c +3 -0
+static int
+tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused const void *peer)
+{
I asked last time but I don't remember the response,
what should be in the 'peer' variable?
The peer variable is currently unused. The primary does not expect any response from secondary processes. Should we change the model to rte_mp_request_sync rather than rte_mp_send_msg?

No need to change,
I asked because I wasn't clear what 'peer' is for, as checked, it is the address of the sender which is used to reply, so not useful in this case.

Kumara Parameshwaran

author

2022-01-24 12:37 UTC #9 lore raw

PATCH v2 1/1 net/tap: fix to populate fds in secondary process

In reply to Kumara Parameshwaran

From: Kumara Parameshwaran <[email protected]>

Fixes 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")

Cc [email protected]

Signed-off-by Kumara Parameshwaran <[email protected]>

---
 drivers/net/tap/rte_eth_tap.c | 196 +++++++++++++---------------------
 lib/ethdev/ethdev_driver.h    |  18 ++++
 lib/ethdev/rte_ethdev.c       |  11 ++
 lib/ethdev/version.map        |   1 +
 4 files changed, 102 insertions(+), 124 deletions(-)

drivers/net/tap/rte_eth_tap.c +72 ~~-124~~

index f1b48cae82..f6c25d7e21 100644
@@ -66,7 +66,7 @@
 	(TAP_GSO_MBUFS_PER_CORE * TAP_GSO_MBUF_CACHE_SIZE)
 
 /* IPC key for queue fds sync */
-#define TAP_MP_KEY "tap_mp_sync_queues"
+#define TAP_MP_REQ_START_RXTX "tap_mp_req_start_rxtx"
 
 #define TAP_IOV_DEFAULT_MAX 1024

@@ -880,11 +880,48 @@ tap_link_set_up(struct rte_eth_dev *dev)
 	return tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
 }
 
+static int tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
+{
+	struct rte_mp_msg msg;
+	struct ipc_queues *request_param = (struct ipc_queues *)msg.param;
+	int err;
+	int fd_iterator = 0;
+	struct pmd_process_private *process_private = dev->process_private;
+	int i;
+
+	memset(&msg, 0, sizeof(msg));
+	strlcpy(msg.name, TAP_MP_REQ_START_RXTX, sizeof(msg.name));
+	strlcpy(request_param->port_name, dev->data->name, sizeof(request_param->port_name));
+	msg.len_param = sizeof(*request_param);
+	for (i = 0; i < dev->data->nb_tx_queues; i++) {
+		msg.fds[fd_iterator++] = process_private->txq_fds[i];
+		msg.num_fds++;
+		request_param->txq_count++;
+	}
+	for (i = 0; i < dev->data->nb_rx_queues; i++) {
+		msg.fds[fd_iterator++] = process_private->rxq_fds[i];
+		msg.num_fds++;
+		request_param->rxq_count++;
+	}
+
+	err = rte_mp_sendmsg(&msg);
+	if (err < 0) {
+		TAP_LOG(ERR, "Failed to send start req to secondary %d",
+			rte_errno);
+		return -1;
+	}
+
+	return 0;
+}
+
 static int
 tap_dev_start(struct rte_eth_dev *dev)
 {
 	int err, i;
 
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+		tap_mp_req_on_rxtx(dev);
+
 	err = tap_intr_handle_set(dev, 1);
 	if (err)
 		return err;

@@ -901,6 +938,34 @@ tap_dev_start(struct rte_eth_dev *dev)
 	return err;
 }
 
+static int
+tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused const void *peer)
+{
+	struct rte_eth_dev *dev;
+	const struct ipc_queues *request_param =
+		(const struct ipc_queues *)request->param;
+	int fd_iterator;
+	int queue;
+	struct pmd_process_private *process_private;
+
+	dev = rte_get_eth_dev_by_name(request_param->port_name);
+	if (!dev) {
+		TAP_LOG(ERR, "Failed to get dev for %s",
+			request_param->port_name);
+		return -1;
+	}
+	process_private = dev->process_private;
+	fd_iterator = 0;
+	TAP_LOG(DEBUG, "tap_attach rx_q:%d tx_q:%d\n", request_param->rxq_count,
+		request_param->txq_count);
+	for (queue = 0; queue < request_param->txq_count; queue++)
+		process_private->txq_fds[queue] = request->fds[fd_iterator++];
+	for (queue = 0; queue < request_param->rxq_count; queue++)
+		process_private->rxq_fds[queue] = request->fds[fd_iterator++];
+
+	return 0;
+}
+
 /* This function gets called when the current port gets stopped.
  */
 static int

@@ -1084,6 +1149,7 @@ tap_dev_close(struct rte_eth_dev *dev)
 
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
 		rte_free(dev->process_private);
+		rte_mp_action_unregister(TAP_MP_REQ_START_RXTX);
 		return 0;
 	}

@@ -1140,8 +1206,6 @@ tap_dev_close(struct rte_eth_dev *dev)
 		internals->ioctl_sock = -1;
 	}
 	rte_free(dev->process_private);
-	if (tap_devices_count == 1)
-		rte_mp_action_unregister(TAP_MP_KEY);
 	tap_devices_count--;
 	/*
 	 * Since TUN device has no more opened file descriptors

@@ -2292,113 +2356,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
 	return ret;
 }
 
-/* Request queue file descriptors from secondary to primary. */
-static int
-tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
-{
-	int ret;
-	struct timespec timeout = {.tv_sec = 1, .tv_nsec = 0};
-	struct rte_mp_msg request, *reply;
-	struct rte_mp_reply replies;
-	struct ipc_queues *request_param = (struct ipc_queues *)request.param;
-	struct ipc_queues *reply_param;
-	struct pmd_process_private *process_private = dev->process_private;
-	int queue, fd_iterator;
-
-	/* Prepare the request */
-	memset(&request, 0, sizeof(request));
-	strlcpy(request.name, TAP_MP_KEY, sizeof(request.name));
-	strlcpy(request_param->port_name, port_name,
-		sizeof(request_param->port_name));
-	request.len_param = sizeof(*request_param);
-	/* Send request and receive reply */
-	ret = rte_mp_request_sync(&request, &replies, &timeout);
-	if (ret < 0 || replies.nb_received != 1) {
-		TAP_LOG(ERR, "Failed to request queues from primary: %d",
-			rte_errno);
-		return -1;
-	}
-	reply = &replies.msgs[0];
-	reply_param = (struct ipc_queues *)reply->param;
-	TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
-
-	/* Attach the queues from received file descriptors */
-	if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds) {
-		TAP_LOG(ERR, "Unexpected number of fds received");
-		return -1;
-	}
-
-	dev->data->nb_rx_queues = reply_param->rxq_count;
-	dev->data->nb_tx_queues = reply_param->txq_count;
-	fd_iterator = 0;
-	for (queue = 0; queue < reply_param->rxq_count; queue++)
-		process_private->rxq_fds[queue] = reply->fds[fd_iterator++];
-	for (queue = 0; queue < reply_param->txq_count; queue++)
-		process_private->txq_fds[queue] = reply->fds[fd_iterator++];
-	free(reply);
-	return 0;
-}
-
-/* Send the queue file descriptors from the primary process to secondary. */
-static int
-tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
-{
-	struct rte_eth_dev *dev;
-	struct pmd_process_private *process_private;
-	struct rte_mp_msg reply;
-	const struct ipc_queues *request_param =
-		(const struct ipc_queues *)request->param;
-	struct ipc_queues *reply_param =
-		(struct ipc_queues *)reply.param;
-	uint16_t port_id;
-	int queue;
-	int ret;
-
-	/* Get requested port */
-	TAP_LOG(DEBUG, "Received IPC request for %s", request_param->port_name);
-	ret = rte_eth_dev_get_port_by_name(request_param->port_name, &port_id);
-	if (ret) {
-		TAP_LOG(ERR, "Failed to get port id for %s",
-			request_param->port_name);
-		return -1;
-	}
-	dev = &rte_eth_devices[port_id];
-	process_private = dev->process_private;
-
-	/* Fill file descriptors for all queues */
-	reply.num_fds = 0;
-	reply_param->rxq_count = 0;
-	if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
-			RTE_MP_MAX_FD_NUM){
-		TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of fds");
-		return -1;
-	}
-
-	for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
-		reply.fds[reply.num_fds++] = process_private->rxq_fds[queue];
-		reply_param->rxq_count++;
-	}
-	RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
-
-	reply_param->txq_count = 0;
-	for (queue = 0; queue < dev->data->nb_tx_queues; queue++) {
-		reply.fds[reply.num_fds++] = process_private->txq_fds[queue];
-		reply_param->txq_count++;
-	}
-	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
-
-	/* Send reply */
-	strlcpy(reply.name, request->name, sizeof(reply.name));
-	strlcpy(reply_param->port_name, request_param->port_name,
-		sizeof(reply_param->port_name));
-	reply.len_param = sizeof(*reply_param);
-	if (rte_mp_reply(&reply, peer) < 0) {
-		TAP_LOG(ERR, "Failed to reply an IPC request to sync queues");
-		return -1;
-	}
-	return 0;
-}
-
 /* Open a TAP interface device.
  */
 static int

@@ -2442,9 +2399,11 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 			return -1;
 		}
 
-		ret = tap_mp_attach_queues(name, eth_dev);
-		if (ret != 0)
-			return -1;
+		ret = rte_mp_action_register(TAP_MP_REQ_START_RXTX, tap_mp_req_start_rxtx);
+		if (ret < 0 && rte_errno != ENOTSUP)
+			TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
+				strerror(rte_errno));
+
 		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
 	}

@@ -2492,15 +2451,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 
 	TAP_LOG(DEBUG, "Initializing pmd_tap for %s", name);
 
-	/* Register IPC feed callback */
-	if (!tap_devices_count) {
-		ret = rte_mp_action_register(TAP_MP_KEY, tap_mp_sync_queues);
-		if (ret < 0 && rte_errno != ENOTSUP) {
-			TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
-				strerror(rte_errno));
-			goto leave;
-		}
-	}
 	tap_devices_count++;
 	tap_devices_count_increased = 1;
 	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,

@@ -2511,8 +2461,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 		TAP_LOG(ERR, "Failed to create pmd for %s as %s",
 			name, tap_name);
 		if (tap_devices_count_increased == 1) {
-			if (tap_devices_count == 1)
-				rte_mp_action_unregister(TAP_MP_KEY);
 			tap_devices_count--;
 		}
 	}

lib/ethdev/ethdev_driver.h +18 -0

index d95605a355..04b3a9f933 100644
@@ -1629,6 +1629,24 @@ rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue,
 				struct rte_hairpin_peer_info *peer_info,
 				uint32_t direction);
 
+/**
+ * @internal
+ * Get rte_eth_dev from device name. The device name should be specified
+ * as below:
+ * - PCIe address (Domain:Bus:Device.Function), for example- 0000:2:00.0
+ * - SoC device name, for example- fsl-gmac0
+ * - vdev dpdk name, for example- net_[pcap0|null0|tap0]
+ *
+ * @param name
+ *  pci address or name of the device
+ * @return
+ *   - rte_eth_dev if successful
+ *   - NULL on failure
+ */
+__rte_internal
+struct rte_eth_dev*
+rte_get_eth_dev_by_name(const char *name);
+
 /**
  * @internal
  * Reset the current queue state and configuration to disconnect (unbind) it

lib/ethdev/rte_ethdev.c +11 -0

index a1d475a292..9192b0d664 100644
@@ -894,6 +894,17 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
 	return -ENODEV;
 }
 
+struct rte_eth_dev *
+rte_get_eth_dev_by_name(const char *name)
+{
+	uint16_t pid;
+
+	if (rte_eth_dev_get_port_by_name(name, &pid))
+		return NULL;
+
+	return &rte_eth_devices[pid];
+}
+
 static int
 eth_err(uint16_t port_id, int ret)
 {

lib/ethdev/version.map +1 -0

index c2fb0669a4..23ca0e8865 100644
@@ -282,4 +282,5 @@ INTERNAL {
 	rte_eth_representor_id_get;
 	rte_eth_switch_domain_alloc;
 	rte_eth_switch_domain_free;
+	rte_get_eth_dev_by_name;
 };

Signature · 1 line

2.17.1

Ferruh Yigit

2022-01-24 13:07 UTC #10 lore raw

Re: [PATCH v2] net/tap: fix to populate fds in secondary process

Quoted 8 lines · Kumara Parameshwaran

From: Kumara Parameshwaran<[email protected]>
When a tap device is hotplugged to primary process which in turn adds the device to all secondary process, the secondary process does a tap_mp_attach_queues, but the fds are not populated in the primary during the probe they are populated during the queue_setup, added a fix to sync the queues during rte_eth_dev_start

Quoted 4 lines

Fixes 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")
Cc [email protected]
Signed-off-by Kumara Parameshwaran<[email protected]>

Should this be v3?
Can you please accumulate change longs, and increase versions as it progress?

And there were more comments in v2, like splitting the patch, can you please check them all?

Thanks,
ferruh

Kumara Parameshwaran

author

2022-01-31 14:21 UTC #11 lore raw

PATCH v3 1/2 ethdev: define a function to get eth dev structure

In reply to Kumara Parameshwaran

From: Kumara Parameshwaran <[email protected]>

The PMDs would need a function to access the rte_eth_devices global array

Cc [email protected]

Signed-off-by Kumara Parameshwaran <[email protected]>

---
 lib/ethdev/ethdev_driver.h | 18 ++++++++++++++++++
 lib/ethdev/rte_ethdev.c    | 11 +++++++++++
 lib/ethdev/version.map     |  1 +
 3 files changed, 30 insertions(+)

lib/ethdev/ethdev_driver.h +18 -0

index d95605a355..7d27781f7d 100644
@@ -1629,6 +1629,24 @@ rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue,
 				struct rte_hairpin_peer_info *peer_info,
 				uint32_t direction);
 
+/**
+ * @internal
+ * Get rte_eth_dev from device name. The device name should be specified
+ * as below:
+ * - PCIe address (Domain:Bus:Device.Function), for example- 0000:2:00.0
+ * - SoC device name, for example- fsl-gmac0
+ * - vdev dpdk name, for example- net_[pcap0|null0|tap0]
+ *
+ * @param name
+ *  pci address or name of the device
+ * @return
+ *   - rte_eth_dev if successful
+ *   - NULL on failure
+ */
+__rte_internal
+struct rte_eth_dev*
+rte_eth_dev_get_by_name(const char *name);
+
 /**
  * @internal
  * Reset the current queue state and configuration to disconnect (unbind) it

lib/ethdev/rte_ethdev.c +11 -0

index a1d475a292..968475d107 100644
@@ -894,6 +894,17 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
 	return -ENODEV;
 }
 
+struct rte_eth_dev *
+rte_eth_dev_get_by_name(const char *name)
+{
+	uint16_t pid;
+
+	if (rte_eth_dev_get_port_by_name(name, &pid))
+		return NULL;
+
+	return &rte_eth_devices[pid];
+}
+
 static int
 eth_err(uint16_t port_id, int ret)
 {

lib/ethdev/version.map +1 -0

index c2fb0669a4..1f7359c846 100644
@@ -267,6 +267,7 @@ INTERNAL {
 	rte_eth_dev_callback_process;
 	rte_eth_dev_create;
 	rte_eth_dev_destroy;
+	rte_eth_dev_get_by_name;
 	rte_eth_dev_is_rx_hairpin_queue;
 	rte_eth_dev_is_tx_hairpin_queue;
 	rte_eth_dev_probing_finish;

Signature · 1 line

2.17.1

Kumara Parameshwaran

author

2022-01-31 14:21 UTC #12 lore raw

PATCH v3 2/2 net/tap: fix to populate fds in secondary process

From: Kumara Parameshwaran <[email protected]>

Fixes 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")

Cc [email protected]

Signed-off-by Kumara Parameshwaran <[email protected]>

---
v3:
* Retain tap_sync_queues to retain the attach of secondary process
* Fix coding convention for a function definition
* Renamed rte_get_eth_dev_by_name to rte_eth_dev_get_by_name in sorted in version.map
* Remove uninteded blank line

 drivers/net/tap/rte_eth_tap.c | 80 +++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

drivers/net/tap/rte_eth_tap.c +80 -0

index f1b48cae82..d13baadbe7 100644
@@ -67,6 +67,7 @@
 
 /* IPC key for queue fds sync */
 #define TAP_MP_KEY "tap_mp_sync_queues"
+#define TAP_MP_REQ_START_RXTX "tap_mp_req_start_rxtx"
 
 #define TAP_IOV_DEFAULT_MAX 1024

@@ -880,11 +881,49 @@ tap_link_set_up(struct rte_eth_dev *dev)
 	return tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
 }
 
+static int
+tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
+{
+	struct rte_mp_msg msg;
+	struct ipc_queues *request_param = (struct ipc_queues *)msg.param;
+	int err;
+	int fd_iterator = 0;
+	struct pmd_process_private *process_private = dev->process_private;
+	int i;
+
+	memset(&msg, 0, sizeof(msg));
+	strlcpy(msg.name, TAP_MP_REQ_START_RXTX, sizeof(msg.name));
+	strlcpy(request_param->port_name, dev->data->name, sizeof(request_param->port_name));
+	msg.len_param = sizeof(*request_param);
+	for (i = 0; i < dev->data->nb_tx_queues; i++) {
+		msg.fds[fd_iterator++] = process_private->txq_fds[i];
+		msg.num_fds++;
+		request_param->txq_count++;
+	}
+	for (i = 0; i < dev->data->nb_rx_queues; i++) {
+		msg.fds[fd_iterator++] = process_private->rxq_fds[i];
+		msg.num_fds++;
+		request_param->rxq_count++;
+	}
+
+	err = rte_mp_sendmsg(&msg);
+	if (err < 0) {
+		TAP_LOG(ERR, "Failed to send start req to secondary %d",
+			rte_errno);
+		return -1;
+	}
+
+	return 0;
+}
+
 static int
 tap_dev_start(struct rte_eth_dev *dev)
 {
 	int err, i;
 
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+		tap_mp_req_on_rxtx(dev);
+
 	err = tap_intr_handle_set(dev, 1);
 	if (err)
 		return err;

@@ -901,6 +940,34 @@ tap_dev_start(struct rte_eth_dev *dev)
 	return err;
 }
 
+static int
+tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused const void *peer)
+{
+	struct rte_eth_dev *dev;
+	const struct ipc_queues *request_param =
+		(const struct ipc_queues *)request->param;
+	int fd_iterator;
+	int queue;
+	struct pmd_process_private *process_private;
+
+	dev = rte_eth_dev_get_by_name(request_param->port_name);
+	if (!dev) {
+		TAP_LOG(ERR, "Failed to get dev for %s",
+			request_param->port_name);
+		return -1;
+	}
+	process_private = dev->process_private;
+	fd_iterator = 0;
+	TAP_LOG(DEBUG, "tap_attach rx_q:%d tx_q:%d\n", request_param->rxq_count,
+		request_param->txq_count);
+	for (queue = 0; queue < request_param->txq_count; queue++)
+		process_private->txq_fds[queue] = request->fds[fd_iterator++];
+	for (queue = 0; queue < request_param->rxq_count; queue++)
+		process_private->rxq_fds[queue] = request->fds[fd_iterator++];
+
+	return 0;
+}
+
 /* This function gets called when the current port gets stopped.
  */
 static int

@@ -1084,6 +1151,9 @@ tap_dev_close(struct rte_eth_dev *dev)
 
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
 		rte_free(dev->process_private);
+		if (tap_devices_count == 1)
+			rte_mp_action_unregister(TAP_MP_REQ_START_RXTX);
+		tap_devices_count--;
 		return 0;
 	}

@@ -2445,6 +2515,16 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 		ret = tap_mp_attach_queues(name, eth_dev);
 		if (ret != 0)
 			return -1;
+
+		if (!tap_devices_count) {
+			ret = rte_mp_action_register(TAP_MP_REQ_START_RXTX, tap_mp_req_start_rxtx);
+			if (ret < 0 && rte_errno != ENOTSUP) {
+				TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
+					strerror(rte_errno));
+				return -1;
+			}
+		}
+		tap_devices_count++;
 		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
 	}

Signature · 1 line

2.17.1

Kumara Parameshwaran

author

2022-01-31 14:32 UTC #13 lore raw

PATCH v3 1/2 ethdev: define a function to get eth dev structure

In reply to Kumara Parameshwaran

From: Kumara Parameshwaran <[email protected]>

The PMDs would need a function to access the rte_eth_devices global array

Cc [email protected]

Signed-off-by Kumara Parameshwaran <[email protected]>

---
 lib/ethdev/ethdev_driver.h | 18 ++++++++++++++++++
 lib/ethdev/rte_ethdev.c    | 11 +++++++++++
 lib/ethdev/version.map     |  1 +
 3 files changed, 30 insertions(+)

lib/ethdev/ethdev_driver.h +18 -0

index d95605a355..7d27781f7d 100644
@@ -1629,6 +1629,24 @@ rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue,
 				struct rte_hairpin_peer_info *peer_info,
 				uint32_t direction);
 
+/**
+ * @internal
+ * Get rte_eth_dev from device name. The device name should be specified
+ * as below:
+ * - PCIe address (Domain:Bus:Device.Function), for example- 0000:2:00.0
+ * - SoC device name, for example- fsl-gmac0
+ * - vdev dpdk name, for example- net_[pcap0|null0|tap0]
+ *
+ * @param name
+ *  pci address or name of the device
+ * @return
+ *   - rte_eth_dev if successful
+ *   - NULL on failure
+ */
+__rte_internal
+struct rte_eth_dev*
+rte_eth_dev_get_by_name(const char *name);
+
 /**
  * @internal
  * Reset the current queue state and configuration to disconnect (unbind) it

lib/ethdev/rte_ethdev.c +11 -0

index a1d475a292..968475d107 100644
@@ -894,6 +894,17 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
 	return -ENODEV;
 }
 
+struct rte_eth_dev *
+rte_eth_dev_get_by_name(const char *name)
+{
+	uint16_t pid;
+
+	if (rte_eth_dev_get_port_by_name(name, &pid))
+		return NULL;
+
+	return &rte_eth_devices[pid];
+}
+
 static int
 eth_err(uint16_t port_id, int ret)
 {

lib/ethdev/version.map +1 -0

index c2fb0669a4..1f7359c846 100644
@@ -267,6 +267,7 @@ INTERNAL {
 	rte_eth_dev_callback_process;
 	rte_eth_dev_create;
 	rte_eth_dev_destroy;
+	rte_eth_dev_get_by_name;
 	rte_eth_dev_is_rx_hairpin_queue;
 	rte_eth_dev_is_tx_hairpin_queue;
 	rte_eth_dev_probing_finish;

Signature · 1 line

2.17.1

Kumara Parameshwaran

author

2022-01-31 14:32 UTC #14 lore raw

PATCH v3 2/2 net/tap: fix to populate fds in secondary process

From: Kumara Parameshwaran <[email protected]>

Fixes 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")

Cc [email protected]

Signed-off-by Kumara Parameshwaran <[email protected]>

 drivers/net/tap/rte_eth_tap.c | 80 +++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

drivers/net/tap/rte_eth_tap.c +80 -0

index f1b48cae82..d13baadbe7 100644
@@ -67,6 +67,7 @@
 
 /* IPC key for queue fds sync */
 #define TAP_MP_KEY "tap_mp_sync_queues"
+#define TAP_MP_REQ_START_RXTX "tap_mp_req_start_rxtx"
 
 #define TAP_IOV_DEFAULT_MAX 1024

@@ -880,11 +881,49 @@ tap_link_set_up(struct rte_eth_dev *dev)
 	return tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
 }
 
+static int
+tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
+{
+	struct rte_mp_msg msg;
+	struct ipc_queues *request_param = (struct ipc_queues *)msg.param;
+	int err;
+	int fd_iterator = 0;
+	struct pmd_process_private *process_private = dev->process_private;
+	int i;
+
+	memset(&msg, 0, sizeof(msg));
+	strlcpy(msg.name, TAP_MP_REQ_START_RXTX, sizeof(msg.name));
+	strlcpy(request_param->port_name, dev->data->name, sizeof(request_param->port_name));
+	msg.len_param = sizeof(*request_param);
+	for (i = 0; i < dev->data->nb_tx_queues; i++) {
+		msg.fds[fd_iterator++] = process_private->txq_fds[i];
+		msg.num_fds++;
+		request_param->txq_count++;
+	}
+	for (i = 0; i < dev->data->nb_rx_queues; i++) {
+		msg.fds[fd_iterator++] = process_private->rxq_fds[i];
+		msg.num_fds++;
+		request_param->rxq_count++;
+	}
+
+	err = rte_mp_sendmsg(&msg);
+	if (err < 0) {
+		TAP_LOG(ERR, "Failed to send start req to secondary %d",
+			rte_errno);
+		return -1;
+	}
+
+	return 0;
+}
+
 static int
 tap_dev_start(struct rte_eth_dev *dev)
 {
 	int err, i;
 
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+		tap_mp_req_on_rxtx(dev);
+
 	err = tap_intr_handle_set(dev, 1);
 	if (err)
 		return err;

@@ -901,6 +940,34 @@ tap_dev_start(struct rte_eth_dev *dev)
 	return err;
 }
 
+static int
+tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused const void *peer)
+{
+	struct rte_eth_dev *dev;
+	const struct ipc_queues *request_param =
+		(const struct ipc_queues *)request->param;
+	int fd_iterator;
+	int queue;
+	struct pmd_process_private *process_private;
+
+	dev = rte_eth_dev_get_by_name(request_param->port_name);
+	if (!dev) {
+		TAP_LOG(ERR, "Failed to get dev for %s",
+			request_param->port_name);
+		return -1;
+	}
+	process_private = dev->process_private;
+	fd_iterator = 0;
+	TAP_LOG(DEBUG, "tap_attach rx_q:%d tx_q:%d\n", request_param->rxq_count,
+		request_param->txq_count);
+	for (queue = 0; queue < request_param->txq_count; queue++)
+		process_private->txq_fds[queue] = request->fds[fd_iterator++];
+	for (queue = 0; queue < request_param->rxq_count; queue++)
+		process_private->rxq_fds[queue] = request->fds[fd_iterator++];
+
+	return 0;
+}
+
 /* This function gets called when the current port gets stopped.
  */
 static int

@@ -1084,6 +1151,9 @@ tap_dev_close(struct rte_eth_dev *dev)
 
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
 		rte_free(dev->process_private);
+		if (tap_devices_count == 1)
+			rte_mp_action_unregister(TAP_MP_REQ_START_RXTX);
+		tap_devices_count--;
 		return 0;
 	}

@@ -2445,6 +2515,16 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 		ret = tap_mp_attach_queues(name, eth_dev);
 		if (ret != 0)
 			return -1;
+
+		if (!tap_devices_count) {
+			ret = rte_mp_action_register(TAP_MP_REQ_START_RXTX, tap_mp_req_start_rxtx);
+			if (ret < 0 && rte_errno != ENOTSUP) {
+				TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
+					strerror(rte_errno));
+				return -1;
+			}
+		}
+		tap_devices_count++;
 		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
 	}

Signature · 1 line

2.17.1

Ferruh Yigit

Reviewed-by on v3 2/2

2022-02-01 17:00 UTC #15 lore raw

Re: [PATCH v3 2/2] net/tap: fix to populate fds in secondary process

Quoted 8 lines · Kumara Parameshwaran

From: Kumara Parameshwaran<[email protected]>
When a tap device is hotplugged to primary process which in turn adds the device to all secondary process, the secondary process does a tap_mp_attach_queues, but the fds are not populated in the primary during the probe they are populated during the queue_setup, added a fix to sync the queues during rte_eth_dev_start

Quoted 4 lines

Fixes 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")
Cc [email protected]
Signed-off-by Kumara Parameshwaran<[email protected]>

Reviewed-by Ferruh Yigit <[email protected]>

Ferruh Yigit

Reviewed-by on v3 1/2

2022-02-01 16:58 UTC #16 lore raw

Re: [PATCH v3 1/2] ethdev: define a function to get eth dev structure

In reply to Kumara Parameshwaran

Quoted 7 lines · Kumara Parameshwaran

From: Kumara Parameshwaran <[email protected]>
The PMDs would need a function to access the rte_eth_devices global array
Cc [email protected]

Not sure if this patch is suitable for backport, since it introduces new internal API.
But since API is internal, perhaps it is OK to get it, specially because it is required for next patch that is a fix. Keeping the tag and leaving the decision to LTS maintainers.

Quoted 1 line

Signed-off-by Kumara Parameshwaran <[email protected]>

Reviewed-by Ferruh Yigit <[email protected]>

Ferruh Yigit

2022-02-01 17:13 UTC #17 lore raw

Re: [PATCH v3 1/2] ethdev: define a function to get eth dev structure

Quoted 15 lines · Ferruh Yigit

Quoted 7 lines · Kumara Parameshwaran
From: Kumara Parameshwaran <[email protected]>
The PMDs would need a function to access the rte_eth_devices global array
Cc [email protected]
Not sure if this patch is suitable for backport, since it introduces new internal API.
But since API is internal, perhaps it is OK to get it, specially because it is required for next patch that is a fix. Keeping the tag and leaving the decision to LTS maintainers.

Quoted 3 lines

Quoted 1 line
Signed-off-by Kumara Parameshwaran <[email protected]>
Reviewed-by Ferruh Yigit <[email protected]>

Hi Kumara,

There are some instances in the existing code that can benefit from new API [1], do you have any bandwidth to make a patch to replace 'rte_eth_dev_get_port_by_name()' with new 'rte_eth_dev_get_by_name()'?
(For the cases 'port_id' is used to get eth_dev)

Thanks,
ferruh

[1]

$ git grep rte_eth_dev_get_port_by_name drivers/net/
drivers/net/bonding/rte_eth_bond_api.c: ret = rte_eth_dev_get_port_by_name(name, &port_id);
drivers/net/ipn3ke/ipn3ke_ethdev.c:                     retval = rte_eth_dev_get_port_by_name(fvl_bdf,
drivers/net/memif/rte_eth_memif.c:      ret = rte_eth_dev_get_port_by_name(msg_param->port_name, &port_id);
drivers/net/mlx5/linux/mlx5_os.c:       if (rte_eth_dev_get_port_by_name(name, &port_id) == 0) {
drivers/net/mlx5/windows/mlx5_os.c:     if (rte_eth_dev_get_port_by_name(name, &port_id) == 0) {
drivers/net/ring/rte_eth_ring.c:        ret = rte_eth_dev_get_port_by_name(ring_name, &port_id);
drivers/net/softnic/rte_eth_softnic_cli.c:      status = rte_eth_dev_get_port_by_name(softnic->params.name, &port_id);
drivers/net/softnic/rte_eth_softnic_cli.c:      status = rte_eth_dev_get_port_by_name(softnic->params.name, &port_id);
drivers/net/softnic/rte_eth_softnic_cli.c:      status = rte_eth_dev_get_port_by_name(softnic->params.name, &port_id);
drivers/net/softnic/rte_eth_softnic_cli.c:      status = rte_eth_dev_get_port_by_name(softnic->params.name, &port_id);
drivers/net/softnic/rte_eth_softnic_cli.c:      status = rte_eth_dev_get_port_by_name(softnic->params.name, &port_id);
drivers/net/softnic/rte_eth_softnic_internals.h:        status = rte_eth_dev_get_port_by_name(softnic->params.name, &port_id);
drivers/net/softnic/rte_eth_softnic_link.c:             status = rte_eth_dev_get_port_by_name(params->dev_name,
drivers/net/softnic/rte_eth_softnic_thread.c:   status = rte_eth_dev_get_port_by_name(softnic->params.name, &port_id);
drivers/net/tap/rte_eth_tap.c:  ret = rte_eth_dev_get_port_by_name(request_param->port_name, &port_id);

Ferruh Yigit

2022-02-01 17:16 UTC #18 lore raw

Re: [PATCH v3 1/2] ethdev: define a function to get eth dev structure

In reply to Ferruh Yigit

Quoted 15 lines · Ferruh Yigit

Quoted 7 lines · Kumara Parameshwaran
From: Kumara Parameshwaran <[email protected]>
The PMDs would need a function to access the rte_eth_devices global array
Cc [email protected]
Not sure if this patch is suitable for backport, since it introduces new internal API.
But since API is internal, perhaps it is OK to get it, specially because it is required for next patch that is a fix. Keeping the tag and leaving the decision to LTS maintainers.

Quoted 3 lines

Quoted 1 line
Signed-off-by Kumara Parameshwaran <[email protected]>
Reviewed-by Ferruh Yigit <[email protected]>

Series applied to dpdk-next-net/main, thanks.