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

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

[Kumara Parameshwaran]

From: Kumara Parameshwaran <kparameshwar@vmware.com>

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 <kparameshwar@vmware.com>
---
 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(-)

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"
 
 #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--;
 		}
 	}
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/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 096b676fc1..ca16839c53 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -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
diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
index c2fb0669a4..fdfdf68c3d 100644
--- a/lib/ethdev/version.map
+++ b/lib/ethdev/version.map
@@ -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 {
-- 
2.17.1

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

[Ferruh Yigit]

On 1/21/2022 4:29 AM, Kumara Parameshwaran wrote:
> From: Kumara Parameshwaran<kparameshwar@vmware.com>
> 
> 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<kparameshwar@vmware.com>
> ---
>   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/20220121042944.23929-1-kumaraparamesh92@gmail.com/
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

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

[Kumara Parameshwaran]

From: Kumara Parameshwaran <kparameshwar@vmware.com>

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: stable@dpdk.org

Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
---

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(-)

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"
 
 #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--;
 		}
 	}
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);
 
+/**
+* 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
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;
+
 };
 
 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;
 };
-- 
2.17.1

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

[Ferruh Yigit]

On 1/24/2022 12:12 PM, Kumara Parameshwaran wrote:
> From: Kumara Parameshwaran <kparameshwar@vmware.com>
> 
> 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: stable@dpdk.org
> 
> Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
> ---
> 
> 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

> 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?

>   #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)
{

> +{
> +	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)
> +{

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, 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--;
>   		}
>   	}
> 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.

>   };

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

[Kumara Parameshwaran]

From: Kumara Parameshwaran <kparameshwar@vmware.com>

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: stable@dpdk.org

Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
---
 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(-)

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"
 
 #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--;
 		}
 	}
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index d95605a355..04b3a9f933 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -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
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..23ca0e8865 100644
--- a/lib/ethdev/version.map
+++ b/lib/ethdev/version.map
@@ -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;
 };
-- 
2.17.1

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

[Kumara Parameshwaran]

[-- Attachment #1: Type: text/plain, Size: 15383 bytes --]



________________________________
From: Ferruh Yigit <ferruh.yigit@intel.com>
Sent: 24 January 2022 18:05
To: Kumara Parameshwaran <kumaraparamesh92@gmail.com>; dev@dpdk.org <dev@dpdk.org>
Cc: Kumara Parameshwaran <kparameshwar@vmware.com>; stable@dpdk.org <stable@dpdk.org>
Subject: Re: [PATCH v2] net/tap: fix to populate fds in secondary process

On 1/24/2022 12:12 PM, Kumara Parameshwaran wrote:
> From: Kumara Parameshwaran <kparameshwar@vmware.com>
>
> 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: stable@dpdk.org
>
> Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
> ---
>
> 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, 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?
Yes, that is true. We should retain the current behaviour of start for the attach.

>   #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)
{

> +{
> +     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)
> +{

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, 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--;
>                }
>        }
> 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.

>   };


[-- Attachment #2: Type: text/html, Size: 28808 bytes --]

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

[kumaraparameshwaran rathinavel]

[-- Attachment #1: Type: text/plain, Size: 15533 bytes --]

On Mon, Jan 24, 2022 at 6:05 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 1/24/2022 12:12 PM, Kumara Parameshwaran wrote:
> > From: Kumara Parameshwaran <kparameshwar@vmware.com>
> >
> > 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: stable@dpdk.org
> >
> > Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
> > ---
> >
> > 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)
>
> Can you please follow the coding convention:
> sure, will do it.
> 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)
> > +{
>
> 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, 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--;
> >               }
> >       }
> > 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.
>
> >   };
>
>

[-- Attachment #2: Type: text/html, Size: 20593 bytes --]

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

[Ferruh Yigit]

On 1/24/2022 12:54 PM, kumaraparameshwaran rathinavel wrote:
> 
> 
> On Mon, Jan 24, 2022 at 6:05 PM Ferruh Yigit <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>> wrote:
> 
>     On 1/24/2022 12:12 PM, Kumara Parameshwaran wrote:
>      > From: Kumara Parameshwaran <kparameshwar@vmware.com <mailto:kparameshwar@vmware.com>>
>      >
>      > 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: stable@dpdk.org <mailto:stable@dpdk.org>
>      >
>      > Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com <mailto:kparameshwar@vmware.com>>
>      > ---
>      >
>      > 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)
> 
>     Can you please follow the coding convention:
>     sure, will do it.

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

>     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.
> 
>      >   };
> 

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

[Ferruh Yigit]

On 1/24/2022 12:37 PM, Kumara Parameshwaran wrote:
> From: Kumara Parameshwaran<kparameshwar@vmware.com>
> 
> 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:stable@dpdk.org
> 
> Signed-off-by: Kumara Parameshwaran<kparameshwar@vmware.com>

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

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

[kumaraparameshwaran rathinavel]

[-- Attachment #1: Type: text/plain, Size: 15856 bytes --]

Hi Ferruh,

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

On Mon, Jan 24, 2022 at 6:05 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 1/24/2022 12:12 PM, Kumara Parameshwaran wrote:
> > From: Kumara Parameshwaran <kparameshwar@vmware.com>
> >
> > 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: stable@dpdk.org
> >
> > Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
> > ---
> >
> > 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, 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?
> Yes, will retain the old behaviour.
> >   #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)
> {
>
> > +{
> > +     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)
> > +{
>
> 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?
>


> > +     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--;
> >               }
> >       }
> > 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.
> Sure, will do it.
> > +* 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.
> Sure, will change it.
> > +
> >   /**
> >    * @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.
> Will remove it.
> >   };
> >
> >   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.
> >   };
>
>

[-- Attachment #2: Type: text/html, Size: 21131 bytes --]

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

[Ferruh Yigit]

On 1/28/2022 9:55 AM, kumaraparameshwaran rathinavel wrote:
>  > +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.

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

[Kumara Parameshwaran]

From: Kumara Parameshwaran <kparameshwar@vmware.com>

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

Cc:stable@dpdk.org

Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
---
 lib/ethdev/ethdev_driver.h | 18 ++++++++++++++++++
 lib/ethdev/rte_ethdev.c    | 11 +++++++++++
 lib/ethdev/version.map     |  1 +
 3 files changed, 30 insertions(+)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index d95605a355..7d27781f7d 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -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
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index a1d475a292..968475d107 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_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)
 {
diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
index c2fb0669a4..1f7359c846 100644
--- a/lib/ethdev/version.map
+++ b/lib/ethdev/version.map
@@ -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;
-- 
2.17.1

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

[Kumara Parameshwaran]

From: Kumara Parameshwaran <kparameshwar@vmware.com>

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: stable@dpdk.org

Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
---
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(+)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index f1b48cae82..d13baadbe7 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -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;
 	}
-- 
2.17.1

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

[Kumara Parameshwaran]

From: Kumara Parameshwaran <kparameshwar@vmware.com>

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

Cc: stable@dpdk.org

Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
---
 lib/ethdev/ethdev_driver.h | 18 ++++++++++++++++++
 lib/ethdev/rte_ethdev.c    | 11 +++++++++++
 lib/ethdev/version.map     |  1 +
 3 files changed, 30 insertions(+)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index d95605a355..7d27781f7d 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -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
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index a1d475a292..968475d107 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_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)
 {
diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
index c2fb0669a4..1f7359c846 100644
--- a/lib/ethdev/version.map
+++ b/lib/ethdev/version.map
@@ -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;
-- 
2.17.1

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

[Kumara Parameshwaran]

From: Kumara Parameshwaran <kparameshwar@vmware.com>

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: stable@dpdk.org

Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
---
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(+)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index f1b48cae82..d13baadbe7 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -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;
 	}
-- 
2.17.1

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

[Ferruh Yigit]

On 1/31/2022 2:32 PM, Kumara Parameshwaran wrote:
> From: Kumara Parameshwaran <kparameshwar@vmware.com>
> 
> The PMDs would need a function to access the rte_eth_devices
> global array
> 
> Cc: stable@dpdk.org
> 

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.

> Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

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

[Ferruh Yigit]

On 1/31/2022 2:32 PM, Kumara Parameshwaran wrote:
> From: Kumara Parameshwaran<kparameshwar@vmware.com>
> 
> 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:stable@dpdk.org
> 
> Signed-off-by: Kumara Parameshwaran<kparameshwar@vmware.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

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

[Ferruh Yigit]

On 2/1/2022 4:56 PM, Ferruh Yigit wrote:
> On 1/31/2022 2:32 PM, Kumara Parameshwaran wrote:
>> From: Kumara Parameshwaran <kparameshwar@vmware.com>
>>
>> The PMDs would need a function to access the rte_eth_devices
>> global array
>>
>> Cc: stable@dpdk.org
>>
> 
> 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.
> 
>> Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

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);

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

[Ferruh Yigit]

On 2/1/2022 4:56 PM, Ferruh Yigit wrote:
> On 1/31/2022 2:32 PM, Kumara Parameshwaran wrote:
>> From: Kumara Parameshwaran <kparameshwar@vmware.com>
>>
>> The PMDs would need a function to access the rte_eth_devices
>> global array
>>
>> Cc: stable@dpdk.org
>>
> 
> 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.
> 
>> Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

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