[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 | 79 +++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index f1b48cae82..98791f8dbe 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,50 @@ 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;
+
+	memset(&msg, 0, sizeof(msg));
+	strcpy(msg.name, TAP_MP_REQ_START_RXTX);
+	strlcpy(request_param->port_name, dev->data->name,
+		sizeof(request_param->port_name));
+	msg.len_param = sizeof(*request_param);
+	for (int 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 (int 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++;
+	}
+
+	RTE_ASSERT(request_param->txq_count == dev->data->nb_tx_queues);
+	RTE_ASSERT(request_param->rxq_count == dev->data->nb_rx_queues);
+
+	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;
 
+	tap_mp_req_on_rxtx(dev);
+
 	err = tap_intr_handle_set(dev, 1);
 	if (err)
 		return err;
@@ -901,6 +941,39 @@ 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;
+	int ret;
+	uint16_t port_id;
+	const struct ipc_queues *request_param =
+		(const struct ipc_queues *)request->param;
+	int fd_iterator;
+	int queue;
+	struct pmd_process_private *process_private;
+
+	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;
+	dev->data->nb_rx_queues = request_param->rxq_count;
+	dev->data->nb_tx_queues = request_param->txq_count;
+	fd_iterator = 0;
+	RTE_LOG(DEBUG, PMD, "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
@@ -2445,6 +2518,12 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 		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;
 	}
-- 
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

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

[Ferruh Yigit]

On 1/20/2022 11:12 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
> 

Can you please make this a two patches set?
First one is the new API patch,
second one is the tap patch that uses the new API.

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

> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 096b676fc1..d9594c0460 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -4987,6 +4987,22 @@ 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 sucessful
> +*   - NULL on failure
> +*/
> +struct rte_eth_dev*
> +rte_get_eth_dev_by_name(const char *name);
> +


Can you please move this to 'ethdev_driver.h'?

>   /**
>   * 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..168898a27c 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -128,6 +128,7 @@ DPDK_22 {
>   	rte_flow_isolate;
>   	rte_flow_query;
>   	rte_flow_validate;
> +	rte_get_eth_dev_by_name;
>   

And move this to 'INTERNAL' block?

>   	local: *;
>   };

[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

[Stephen Hemminger]

On Thu, 20 Jan 2022 16:42:15 +0530
Kumara Parameshwaran <kumaraparamesh92@gmail.com> wrote:

> +/**
> +* 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 sucessful 
> +*   - NULL on failure
> +*/
> +struct rte_eth_dev*
> +rte_get_eth_dev_by_name(const char *name);

Needs to be marked internal and/or experimental.

[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       |  16 +++
 lib/ethdev/version.map        |   2 +
 4 files changed, 101 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..f9a108faaa 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -4987,6 +4987,22 @@ 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
+*/
+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

[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       |  16 +++
 lib/ethdev/version.map        |   1 +
 4 files changed, 100 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..f9a108faaa 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -4987,6 +4987,22 @@ 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
+*/
+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..168898a27c 100644
--- a/lib/ethdev/version.map
+++ b/lib/ethdev/version.map
@@ -128,6 +128,7 @@ DPDK_22 {
 	rte_flow_isolate;
 	rte_flow_query;
 	rte_flow_validate;
+	rte_get_eth_dev_by_name;
 
 	local: *;
 };
-- 
2.17.1

[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 | 197 +++++++++++++---------------------
 lib/ethdev/rte_ethdev.c       |  11 ++
 lib/ethdev/rte_ethdev.h       |  16 +++
 lib/ethdev/version.map        |   1 +
 4 files changed, 101 insertions(+), 124 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index f1b48cae82..821b08e050 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,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 +939,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 +1150,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 +1207,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 +2357,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 +2400,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 +2452,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 +2462,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..d9594c0460 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -4987,6 +4987,22 @@ 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 sucessful 
+*   - NULL on failure
+*/
+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..168898a27c 100644
--- a/lib/ethdev/version.map
+++ b/lib/ethdev/version.map
@@ -128,6 +128,7 @@ DPDK_22 {
 	rte_flow_isolate;
 	rte_flow_query;
 	rte_flow_validate;
+	rte_get_eth_dev_by_name;
 
 	local: *;
 };
-- 
2.17.1

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

[Stephen Hemminger]

On Wed, 19 Jan 2022 10:03:49 +0530
kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com> wrote:

> > > Why is this necessary?  dev->data is already in memory shared between  
> > primary  
> > > and secondary process.  
> >  
> > > The question is about the two assignments that happen in secondary proces
> > > that change dev->data->nb_rx_queues and dev->data->nb_tx_queues.  These  
> > are  
> > > shared and should not need to be modified here.  
> >  
>  Sure my bad. Misunderstood the comment. Yes, this should not be done, will
> remove the assignments.
 
No problem, primary/secondary process support is a bug farm because it
is too easy to have pointer that is from primary process leak into secondary.

I just wanted to make sure there wasn't something wrong in the infrastructure.

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

[kumaraparameshwaran rathinavel]

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

On Tue, Jan 18, 2022 at 9:51 PM Stephen Hemminger <
stephen@networkplumber.org> wrote:

> On Tue, 18 Jan 2022 05:22:19 +0000
> Kumara Parameshwaran <kparameshwar@vmware.com> wrote:
>
> > @Stephen Hemminger<mailto:stephen@networkplumber.org> This is process
> private as the tap fds are maintained in per process data structures. In
> existing scheme, the fds are opened by the primary during queue setup and
> exchanged to during secondary probe where the send_msg using SOL_SOCKET and
> SCM_RIGHTS would remap the corresponding fds to the secondary process. If
> the secondary process is coming up once the primary is initialised things
> would work fine, but it's a problem during hotplug of the tap device.
> >
> > Thanks,
> > Param.
> > ________________________________
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: 18 January 2022 03:46
> > To: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> > Cc: keith.wiles@intel.com <keith.wiles@intel.com>; dev@dpdk.org <
> dev@dpdk.org>; Kumara Parameshwaran <kparameshwar@vmware.com>
> > Subject: Re: [PATCH] net/tap: Bug fix to populate fds in secondary
> process
> >
> > On Fri, 26 Nov 2021 09:45:15 +0530
> > Kumara Parameshwaran <kumaraparamesh92@gmail.com> wrote:
> >
> > > +     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;
> > > +     dev->data->nb_rx_queues = request_param->rxq_count;
> > > +     dev->data->nb_tx_queues = request_param->txq_count;
> >
> > Why is this necessary?  dev->data is already in memory shared between
> primary
> > and secondary process.
>
> > The question is about the two assignments that happen in secondary proces
> > that change dev->data->nb_rx_queues and dev->data->nb_tx_queues.  These
> are
> > shared and should not need to be modified here.
>
 Sure my bad. Misunderstood the comment. Yes, this should not be done, will
remove the assignments.

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

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

[Stephen Hemminger]

On Tue, 18 Jan 2022 05:22:19 +0000
Kumara Parameshwaran <kparameshwar@vmware.com> wrote:

> @Stephen Hemminger<mailto:stephen@networkplumber.org> This is process private as the tap fds are maintained in per process data structures. In existing scheme, the fds are opened by the primary during queue setup and exchanged to during secondary probe where the send_msg using SOL_SOCKET and SCM_RIGHTS would remap the corresponding fds to the secondary process. If the secondary process is coming up once the primary is initialised things would work fine, but it's a problem during hotplug of the tap device.
> 
> Thanks,
> Param.
> ________________________________
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: 18 January 2022 03:46
> To: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> Cc: keith.wiles@intel.com <keith.wiles@intel.com>; dev@dpdk.org <dev@dpdk.org>; Kumara Parameshwaran <kparameshwar@vmware.com>
> Subject: Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
> 
> On Fri, 26 Nov 2021 09:45:15 +0530
> Kumara Parameshwaran <kumaraparamesh92@gmail.com> wrote:
> 
> > +     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;
> > +     dev->data->nb_rx_queues = request_param->rxq_count;
> > +     dev->data->nb_tx_queues = request_param->txq_count;  
> 
> Why is this necessary?  dev->data is already in memory shared between primary
> and secondary process.

The question is about the two assignments that happen in secondary proces
that change dev->data->nb_rx_queues and dev->data->nb_tx_queues.  These are
shared and should not need to be modified here.

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

[Thomas Monjalon]

18/01/2022 13:12, Ferruh Yigit:
> On 1/18/2022 11:21 AM, kumaraparameshwaran rathinavel wrote:
> 
> Comment moved down.
> 
> Please don't top post, it makes very hard to follow the discussion and bad
> for archives to visit discussion later.
> 
> > 
> > On Tue, Jan 18, 2022 at 3:17 PM Ferruh Yigit <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>> wrote:
> > 
> >     On 1/17/2022 6:33 PM, Thomas Monjalon wrote:
> >      > 17/01/2022 19:28, Ferruh Yigit:
> >      >>> +   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];
> >      >>
> >      >> Since this is not really related with your patch, I want to have a separate thread for it.
> >      >>
> >      >> It is not good to access the 'rte_eth_devices' global variable directly from a driver, that
> >      >> is error prone.
> >      >>
> >      >> Btw, what 'peer' supposed to contain?
> >      >>
> >      >> It can be solved by adding an internal API, only for drivers to get eth_dev from the name,
> >      >> like: 'rte_eth_dev_get_by_name()'.
> >      >> This way a few other usage can be converted to this API.
> >      >>
> >      >> @Thomas and @Andrew what do you think about the new API proposal?
> >      >
> >      > It looks similar to rte_eth_dev_get_port_by_name() which returns a port_id.
> > 
> >     Exactly, but get eth_dev directly for drivers. For drivers no need to work with port_id
> >     handler, they can use eth_dev directly.
> > 
> >     Another solution can be an getter function for drivers, which gets port_id and returns
> >     the eth_dev.
> > 
> >      > It is a bit strange for an ethdev driver to not have access to its own ethdev struct.
> >      > Isn't there something broken in the logic?
> >      >
> > 
> >     This is callback function between primary and secondary applications sync. So port name
> >     will be same for both, but eth_dev will be different and port_id may be different.
> >     Driver finds its own eth_dev from the shared port name.
> > 
> 
> > Just wanted to bring it to your attention,
> > 
> > In Mellanox driver there is a requirement to exchange fds between primary and secondary and similar usage is seen, the primary sends the port_id and the secondary refers to the rte_eth_devices in the driver,
> > The functions are
> >             - mlx5_mp_secondary_handle in secondary
> >             - mlx5_mp_req_start_rxtx in primary which is invoked from mlx5_dev_start.
> > 
> > In my implementation I have used the name and invoked get_port_by_name, I can also pass the port_id from the primary to make it uniform. So with similar usage in Mellanox is there a problem there as well on referring to the rte_eth_devices from the PMD ?
> > 
> 
> It would be same, still will be accessing to the 'rte_eth_devices'.
> That is why a new API for drivers may help.

I agree to add a new API if needed to remove those direct access to rte_eth_devices.

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

[Ferruh Yigit]

On 1/18/2022 10:52 AM, kumaraparameshwaran rathinavel wrote:

Comment moved down, please avoid top posting.

> 
> On Tue, Jan 18, 2022 at 2:40 PM Ferruh Yigit <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>> wrote:
> 
>     On 1/18/2022 4:39 AM, Kumara Parameshwaran wrote:
>      >>   static int
>      >>   tap_dev_start(struct rte_eth_dev *dev)
>      >>   {
>      >>        int err, i;
>      >>
>      >> +     tap_mp_req_on_rxtx(dev);
>      >> +
>      >
>      > As for as I understand your logic is primary sends the message to the secondar(y|ies),
>      > so what happens first secondary is started?
>      > ​In case of TAP PMD looks like there is an assumption where primary should be started first. There is an existing check below during the probe function call.
>      > if (!rte_eal_primary_proc_alive(NULL)) {
>      >       TAP_LOG(ERR, "Primary process is missing");
>      >        return -1;
>      > }
>      >
>      > What about secondary sends the message when they are started?
>      > ​IMHO, since primary process setups the queue it should be sufficient for the primary processes to the send the message and secondary need not send anything.
>      >
>      > Also above functions is called by both primary and secondary, what happens when it is
>      > called by secondary? And the logic is not clear, it can be good to add a process type
>      > check to clarify.
>      > ​Sure, these are for tap_intr_handle_set and tap_dev_start functions?
> 
>     I was thinking within the 'tap_dev_start()' function, for 'tap_mp_req_on_rxtx()' call.
> 
>     Not sure how 'tap_intr_handle_set()' is involved, am I missing something.
>> Yes, even I was confused if it  had been the tap_intr_handle_set function.
> 
> In general the tap_dev_start should not be invoked by the secondary and only primary should do it. I referred it to a couple of PMDs and that was the case. Please let me know if I am missing something in my understanding.
> 


Yes, that is the intended usecase, that primary does the start.
But having the check can help documenting the intention, that it is only for primary.

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

[Ferruh Yigit]

On 1/18/2022 11:21 AM, kumaraparameshwaran rathinavel wrote:

Comment moved down.

Please don't top post, it makes very hard to follow the discussion and bad
for archives to visit discussion later.

> 
> On Tue, Jan 18, 2022 at 3:17 PM Ferruh Yigit <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>> wrote:
> 
>     On 1/17/2022 6:33 PM, Thomas Monjalon wrote:
>      > 17/01/2022 19:28, Ferruh Yigit:
>      >>> +   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];
>      >>
>      >> Since this is not really related with your patch, I want to have a separate thread for it.
>      >>
>      >> It is not good to access the 'rte_eth_devices' global variable directly from a driver, that
>      >> is error prone.
>      >>
>      >> Btw, what 'peer' supposed to contain?
>      >>
>      >> It can be solved by adding an internal API, only for drivers to get eth_dev from the name,
>      >> like: 'rte_eth_dev_get_by_name()'.
>      >> This way a few other usage can be converted to this API.
>      >>
>      >> @Thomas and @Andrew what do you think about the new API proposal?
>      >
>      > It looks similar to rte_eth_dev_get_port_by_name() which returns a port_id.
> 
>     Exactly, but get eth_dev directly for drivers. For drivers no need to work with port_id
>     handler, they can use eth_dev directly.
> 
>     Another solution can be an getter function for drivers, which gets port_id and returns
>     the eth_dev.
> 
>      > It is a bit strange for an ethdev driver to not have access to its own ethdev struct.
>      > Isn't there something broken in the logic?
>      >
> 
>     This is callback function between primary and secondary applications sync. So port name
>     will be same for both, but eth_dev will be different and port_id may be different.
>     Driver finds its own eth_dev from the shared port name.
> 

> Just wanted to bring it to your attention,
> 
> In Mellanox driver there is a requirement to exchange fds between primary and secondary and similar usage is seen, the primary sends the port_id and the secondary refers to the rte_eth_devices in the driver,
> The functions are
>             - mlx5_mp_secondary_handle in secondary
>             - mlx5_mp_req_start_rxtx in primary which is invoked from mlx5_dev_start.
> 
> In my implementation I have used the name and invoked get_port_by_name, I can also pass the port_id from the primary to make it uniform. So with similar usage in Mellanox is there a problem there as well on referring to the rte_eth_devices from the PMD ?
> 

It would be same, still will be accessing to the 'rte_eth_devices'.
That is why a new API for drivers may help.

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

[kumaraparameshwaran rathinavel]

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

Just wanted to bring it to your attention,

In Mellanox driver there is a requirement to exchange fds between primary
and secondary and similar usage is seen, the primary sends the port_id and
the secondary refers to the rte_eth_devices in the driver,
The functions are
           - mlx5_mp_secondary_handle in secondary
           - mlx5_mp_req_start_rxtx in primary which is invoked from
mlx5_dev_start.

In my implementation I have used the name and invoked get_port_by_name, I
can also pass the port_id from the primary to make it uniform. So with
similar usage in Mellanox is there a problem there as well on referring to
the rte_eth_devices from the PMD ?


On Tue, Jan 18, 2022 at 3:17 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 1/17/2022 6:33 PM, Thomas Monjalon wrote:
> > 17/01/2022 19:28, Ferruh Yigit:
> >>> +   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];
> >>
> >> Since this is not really related with your patch, I want to have a
> separate thread for it.
> >>
> >> It is not good to access the 'rte_eth_devices' global variable directly
> from a driver, that
> >> is error prone.
> >>
> >> Btw, what 'peer' supposed to contain?
> >>
> >> It can be solved by adding an internal API, only for drivers to get
> eth_dev from the name,
> >> like: 'rte_eth_dev_get_by_name()'.
> >> This way a few other usage can be converted to this API.
> >>
> >> @Thomas and @Andrew what do you think about the new API proposal?
> >
> > It looks similar to rte_eth_dev_get_port_by_name() which returns a
> port_id.
>
> Exactly, but get eth_dev directly for drivers. For drivers no need to work
> with port_id
> handler, they can use eth_dev directly.
>
> Another solution can be an getter function for drivers, which gets port_id
> and returns
> the eth_dev.
>
> > It is a bit strange for an ethdev driver to not have access to its own
> ethdev struct.
> > Isn't there something broken in the logic?
> >
>
> This is callback function between primary and secondary applications sync.
> So port name
> will be same for both, but eth_dev will be different and port_id may be
> different.
> Driver finds its own eth_dev from the shared port name.
>
>

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

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

[kumaraparameshwaran rathinavel]

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

Yes, even I was confused if it  had been the tap_intr_handle_set function.

In general the tap_dev_start should not be invoked by the secondary and
only primary should do it. I referred it to a couple of PMDs and that was
the case. Please let me know if I am missing something in my understanding.


On Tue, Jan 18, 2022 at 2:40 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 1/18/2022 4:39 AM, Kumara Parameshwaran wrote:
> >>   static int
> >>   tap_dev_start(struct rte_eth_dev *dev)
> >>   {
> >>        int err, i;
> >>
> >> +     tap_mp_req_on_rxtx(dev);
> >> +
> >
> > As for as I understand your logic is primary sends the message to the
> secondar(y|ies),
> > so what happens first secondary is started?
> > ​In case of TAP PMD looks like there is an assumption where primary
> should be started first. There is an existing check below during the probe
> function call.
> > if (!rte_eal_primary_proc_alive(NULL)) {
> >       TAP_LOG(ERR, "Primary process is missing");
> >        return -1;
> > }
> >
> > What about secondary sends the message when they are started?
> > ​IMHO, since primary process setups the queue it should be sufficient
> for the primary processes to the send the message and secondary need not
> send anything.
> >
> > Also above functions is called by both primary and secondary, what
> happens when it is
> > called by secondary? And the logic is not clear, it can be good to add a
> process type
> > check to clarify.
> > ​Sure, these are for tap_intr_handle_set and tap_dev_start functions?
>
> I was thinking within the 'tap_dev_start()' function, for
> 'tap_mp_req_on_rxtx()' call.
>
> Not sure how 'tap_intr_handle_set()' is involved, am I missing something.
>

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

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

[Ferruh Yigit]

On 1/17/2022 6:33 PM, Thomas Monjalon wrote:
> 17/01/2022 19:28, Ferruh Yigit:
>>> +	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];
>>
>> Since this is not really related with your patch, I want to have a separate thread for it.
>>
>> It is not good to access the 'rte_eth_devices' global variable directly from a driver, that
>> is error prone.
>>
>> Btw, what 'peer' supposed to contain?
>>
>> It can be solved by adding an internal API, only for drivers to get eth_dev from the name,
>> like: 'rte_eth_dev_get_by_name()'.
>> This way a few other usage can be converted to this API.
>>
>> @Thomas and @Andrew what do you think about the new API proposal?
> 
> It looks similar to rte_eth_dev_get_port_by_name() which returns a port_id.

Exactly, but get eth_dev directly for drivers. For drivers no need to work with port_id
handler, they can use eth_dev directly.

Another solution can be an getter function for drivers, which gets port_id and returns
the eth_dev.

> It is a bit strange for an ethdev driver to not have access to its own ethdev struct.
> Isn't there something broken in the logic?
> 

This is callback function between primary and secondary applications sync. So port name
will be same for both, but eth_dev will be different and port_id may be different.
Driver finds its own eth_dev from the shared port name.

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

[Ferruh Yigit]

On 1/18/2022 4:39 AM, Kumara Parameshwaran wrote:
>>   static int
>>   tap_dev_start(struct rte_eth_dev *dev)
>>   {
>>        int err, i;
>>   
>> +     tap_mp_req_on_rxtx(dev);
>> +
> 
> As for as I understand your logic is primary sends the message to the secondar(y|ies),
> so what happens first secondary is started?
> ​In case of TAP PMD looks like there is an assumption where primary should be started first. There is an existing check below during the probe function call.
> if (!rte_eal_primary_proc_alive(NULL)) {
>       TAP_LOG(ERR, "Primary process is missing");
>        return -1;
> }
> 
> What about secondary sends the message when they are started?
> ​IMHO, since primary process setups the queue it should be sufficient for the primary processes to the send the message and secondary need not send anything.
> 
> Also above functions is called by both primary and secondary, what happens when it is
> called by secondary? And the logic is not clear, it can be good to add a process type
> check to clarify.
> ​Sure, these are for tap_intr_handle_set and tap_dev_start functions?

I was thinking within the 'tap_dev_start()' function, for 'tap_mp_req_on_rxtx()' call.

Not sure how 'tap_intr_handle_set()' is involved, am I missing something.

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

[Kumara Parameshwaran]

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

@Stephen Hemminger<mailto:stephen@networkplumber.org> This is process private as the tap fds are maintained in per process data structures. In existing scheme, the fds are opened by the primary during queue setup and exchanged to during secondary probe where the send_msg using SOL_SOCKET and SCM_RIGHTS would remap the corresponding fds to the secondary process. If the secondary process is coming up once the primary is initialised things would work fine, but it's a problem during hotplug of the tap device.

Thanks,
Param.
________________________________
From: Stephen Hemminger <stephen@networkplumber.org>
Sent: 18 January 2022 03:46
To: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
Cc: keith.wiles@intel.com <keith.wiles@intel.com>; dev@dpdk.org <dev@dpdk.org>; Kumara Parameshwaran <kparameshwar@vmware.com>
Subject: Re: [PATCH] net/tap: Bug fix to populate fds in secondary process

On Fri, 26 Nov 2021 09:45:15 +0530
Kumara Parameshwaran <kumaraparamesh92@gmail.com> wrote:

> +     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;
> +     dev->data->nb_rx_queues = request_param->rxq_count;
> +     dev->data->nb_tx_queues = request_param->txq_count;

Why is this necessary?  dev->data is already in memory shared between primary
and secondary process.

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

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

[Kumara Parameshwaran]

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



________________________________
From: Ferruh Yigit <ferruh.yigit@intel.com>
Sent: 17 January 2022 23:52
To: Kumara Parameshwaran <kumaraparamesh92@gmail.com>; keith.wiles@intel.com <keith.wiles@intel.com>
Cc: dev@dpdk.org <dev@dpdk.org>; Kumara Parameshwaran <kparameshwar@vmware.com>; Raslan Darawsheh <rasland@nvidia.com>
Subject: Re: [PATCH] net/tap: Bug fix to populate fds in secondary process

On 11/26/2021 4:15 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
>

Hi Kumara,

Original intention seems first running primary application, later secondary,
so yes it doesn't looks like covering the hotplug case.
​Thanks. The issue is when we use the vdev_netvsc driver which uses TAP interface,  in multiprocess and RSS mode things are broken. We encountered this issue while using Azure DPDK PMD.

I have a few questions below, can you please check?

> Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
> ---
>   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..8e7915656b 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,51 @@ 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));
> +     strcpy(msg.name, TAP_MP_REQ_START_RXTX);
> +     strlcpy(request_param->port_name, dev->data->name,
> +             sizeof(request_param->port_name));

Why one use 'strcpy' and other 'strlcpy', can you please unify both to 'strlcpy'?
​Sure, will unify it.

> +     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++;
> +     }
> +
> +     RTE_ASSERT(request_param->txq_count == dev->data->nb_tx_queues);
> +     RTE_ASSERT(request_param->rxq_count == dev->data->nb_rx_queues);
> +

Are these assertions really useful?
Asserts are good to verify the external assumptions, but for this case
the values are all related to above logic already.
​I agree, will remove it.

> +     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;
>
> +     tap_mp_req_on_rxtx(dev);
> +

As for as I understand your logic is primary sends the message to the secondar(y|ies),
so what happens first secondary is started?
​In case of TAP PMD looks like there is an assumption where primary should be started first. There is an existing check below during the probe function call.
if (!rte_eal_primary_proc_alive(NULL)) {
     TAP_LOG(ERR, "Primary process is missing");
      return -1;
}

What about secondary sends the message when they are started?
​IMHO, since primary process setups the queue it should be sufficient for the primary processes to the send the message and secondary need not send anything.

Also above functions is called by both primary and secondary, what happens when it is
called by secondary? And the logic is not clear, it can be good to add a process type
check to clarify.
​Sure, these are for tap_intr_handle_set and tap_dev_start functions?

>        err = tap_intr_handle_set(dev, 1);
>        if (err)
>                return err;
> @@ -901,6 +942,39 @@ 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;
> +     int ret;
> +     uint16_t port_id;
> +     const struct ipc_queues *request_param =
> +             (const struct ipc_queues *)request->param;
> +     int fd_iterator;
> +     int queue;
> +     struct pmd_process_private *process_private;
> +
> +     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;
> +     dev->data->nb_rx_queues = request_param->rxq_count;
> +     dev->data->nb_tx_queues = request_param->txq_count;
> +     fd_iterator = 0;
> +     RTE_LOG(DEBUG, PMD, "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
> @@ -2445,6 +2519,12 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
>                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));
> +

While this update fds in start logic is added, do we still need the 'tap_mp_attach_queues()'?
Can't we remove the 'tap_mp_sync_queues()' after this change?
​Agreed, will remove the old tap_mp_attach_queues and tap_mp_sync_queues.

And need to unregister the mp_action, possibly in the 'tap_dev_close()'?
​Yes, will do it.

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

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

[Stephen Hemminger]

On Fri, 26 Nov 2021 09:45:15 +0530
Kumara Parameshwaran <kumaraparamesh92@gmail.com> wrote:

> +	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;
> +	dev->data->nb_rx_queues = request_param->rxq_count;
> +	dev->data->nb_tx_queues = request_param->txq_count;

Why is this necessary?  dev->data is already in memory shared between primary
and secondary process.

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

[Thomas Monjalon]

17/01/2022 19:28, Ferruh Yigit:
> > +	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];
> 
> Since this is not really related with your patch, I want to have a separate thread for it.
> 
> It is not good to access the 'rte_eth_devices' global variable directly from a driver, that
> is error prone.
> 
> Btw, what 'peer' supposed to contain?
> 
> It can be solved by adding an internal API, only for drivers to get eth_dev from the name,
> like: 'rte_eth_dev_get_by_name()'.
> This way a few other usage can be converted to this API.
> 
> @Thomas and @Andrew what do you think about the new API proposal?

It looks similar to rte_eth_dev_get_port_by_name() which returns a port_id.
It is a bit strange for an ethdev driver to not have access to its own ethdev struct.
Isn't there something broken in the logic?

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

[Ferruh Yigit]

On 11/26/2021 4:15 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>

<...>

>   
> +static int
> +tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused const void *peer)
> +{
> +	struct rte_eth_dev *dev;
> +	int ret;
> +	uint16_t port_id;
> +	const struct ipc_queues *request_param =
> +		(const struct ipc_queues *)request->param;
> +	int fd_iterator;
> +	int queue;
> +	struct pmd_process_private *process_private;
> +
> +	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];

Since this is not really related with your patch, I want to have a separate thread for it.

It is not good to access the 'rte_eth_devices' global variable directly from a driver, that
is error prone.

Btw, what 'peer' supposed to contain?

It can be solved by adding an internal API, only for drivers to get eth_dev from the name,
like: 'rte_eth_dev_get_by_name()'.
This way a few other usage can be converted to this API.

@Thomas and @Andrew what do you think about the new API proposal?

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

[Ferruh Yigit]

On 11/26/2021 4:15 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
> 

Hi Kumara,

Original intention seems first running primary application, later secondary,
so yes it doesn't looks like covering the hotplug case.

I have a few questions below, can you please check?

> Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
> ---
>   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..8e7915656b 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,51 @@ 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));
> +	strcpy(msg.name, TAP_MP_REQ_START_RXTX);
> +	strlcpy(request_param->port_name, dev->data->name,
> +		sizeof(request_param->port_name));

Why one use 'strcpy' and other 'strlcpy', can you please unify both to 'strlcpy'?

> +	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++;
> +	}
> +
> +	RTE_ASSERT(request_param->txq_count == dev->data->nb_tx_queues);
> +	RTE_ASSERT(request_param->rxq_count == dev->data->nb_rx_queues);
> +

Are these assertions really useful?
Asserts are good to verify the external assumptions, but for this case
the values are all related to above logic already.

> +	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;
>   
> +	tap_mp_req_on_rxtx(dev);
> +

As for as I understand your logic is primary sends the message to the secondar(y|ies),
so what happens first secondary is started?

What about secondary sends the message when they are started?

Also above functions is called by both primary and secondary, what happens when it is
called by secondary? And the logic is not clear, it can be good to add a process type
check to clarify.

>   	err = tap_intr_handle_set(dev, 1);
>   	if (err)
>   		return err;
> @@ -901,6 +942,39 @@ 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;
> +	int ret;
> +	uint16_t port_id;
> +	const struct ipc_queues *request_param =
> +		(const struct ipc_queues *)request->param;
> +	int fd_iterator;
> +	int queue;
> +	struct pmd_process_private *process_private;
> +
> +	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;
> +	dev->data->nb_rx_queues = request_param->rxq_count;
> +	dev->data->nb_tx_queues = request_param->txq_count;
> +	fd_iterator = 0;
> +	RTE_LOG(DEBUG, PMD, "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
> @@ -2445,6 +2519,12 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
>   		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));
> +

While this update fds in start logic is added, do we still need the 'tap_mp_attach_queues()'?
Can't we remove the 'tap_mp_sync_queues()' after this change?


And need to unregister the mp_action, possibly in the 'tap_dev_close()'?

[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 | 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..8e7915656b 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,51 @@ 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));
+	strcpy(msg.name, TAP_MP_REQ_START_RXTX);
+	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++;
+	}
+
+	RTE_ASSERT(request_param->txq_count == dev->data->nb_tx_queues);
+	RTE_ASSERT(request_param->rxq_count == dev->data->nb_rx_queues);
+
+	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;
 
+	tap_mp_req_on_rxtx(dev);
+
 	err = tap_intr_handle_set(dev, 1);
 	if (err)
 		return err;
@@ -901,6 +942,39 @@ 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;
+	int ret;
+	uint16_t port_id;
+	const struct ipc_queues *request_param =
+		(const struct ipc_queues *)request->param;
+	int fd_iterator;
+	int queue;
+	struct pmd_process_private *process_private;
+
+	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;
+	dev->data->nb_rx_queues = request_param->rxq_count;
+	dev->data->nb_tx_queues = request_param->txq_count;
+	fd_iterator = 0;
+	RTE_LOG(DEBUG, PMD, "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
@@ -2445,6 +2519,12 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 		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;
 	}
-- 
2.17.1

[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 | 79 +++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index f1b48cae82..98791f8dbe 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,50 @@ 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;
+
+	memset(&msg, 0, sizeof(msg));
+	strcpy(msg.name, TAP_MP_REQ_START_RXTX);
+	strlcpy(request_param->port_name, dev->data->name,
+		sizeof(request_param->port_name));
+	msg.len_param = sizeof(*request_param);
+	for (int 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 (int 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++;
+	}
+
+	RTE_ASSERT(request_param->txq_count == dev->data->nb_tx_queues);
+	RTE_ASSERT(request_param->rxq_count == dev->data->nb_rx_queues);
+
+	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;
 
+	tap_mp_req_on_rxtx(dev);
+
 	err = tap_intr_handle_set(dev, 1);
 	if (err)
 		return err;
@@ -901,6 +941,39 @@ 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;
+	int ret;
+	uint16_t port_id;
+	const struct ipc_queues *request_param =
+		(const struct ipc_queues *)request->param;
+	int fd_iterator;
+	int queue;
+	struct pmd_process_private *process_private;
+
+	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;
+	dev->data->nb_rx_queues = request_param->rxq_count;
+	dev->data->nb_tx_queues = request_param->txq_count;
+	fd_iterator = 0;
+	RTE_LOG(DEBUG, PMD, "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
@@ -2445,6 +2518,12 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 		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;
 	}
-- 
2.17.1

[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 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 | 79 +++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index f1b48cae82..42673246b2 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,50 @@ 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;
+
+	memset(&msg, 0, sizeof(msg));
+	strcpy(msg.name, TAP_MP_REQ_START_RXTX);
+	strlcpy(request_param->port_name, dev->data->name,
+		sizeof(request_param->port_name));
+	msg.len_param = sizeof(*request_param);
+	for(int 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(int 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++;
+	}
+
+	RTE_ASSERT(request_param->txq_count == dev->data->nb_tx_queues);
+	RTE_ASSERT(request_param->rxq_count == dev->data->nb_rx_queues);
+
+	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;
 
+	tap_mp_req_on_rxtx(dev);
+
 	err = tap_intr_handle_set(dev, 1);
 	if (err)
 		return err;
@@ -901,6 +941,39 @@ 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;
+	int ret;
+	uint16_t port_id;
+	const struct ipc_queues *request_param =
+		(const struct ipc_queues *)request->param;
+	int fd_iterator;
+	int queue;
+	struct pmd_process_private *process_private;
+
+	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;
+	dev->data->nb_rx_queues = request_param->rxq_count;
+	dev->data->nb_tx_queues = request_param->txq_count;
+	fd_iterator = 0;
+	RTE_LOG(DEBUG, PMD, "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
@@ -2445,6 +2518,12 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 		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;
 	}
-- 
2.17.1