* [PATCH] net/tap: Bug fix to populate fds in secondary process
@ 2022-01-21 4:29 Kumara Parameshwaran
2022-01-24 9:47 ` Ferruh Yigit
` (4 more replies)
0 siblings, 5 replies; 44+ messages in thread
From: Kumara Parameshwaran @ 2022-01-21 4:29 UTC (permalink / raw)
To: ferruh.yigit; +Cc: dev, 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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
2022-01-21 4:29 [PATCH] net/tap: Bug fix to populate fds in secondary process Kumara Parameshwaran
@ 2022-01-24 9:47 ` Ferruh Yigit
2022-01-24 12:12 ` [PATCH v2] net/tap: " Kumara Parameshwaran
` (3 subsequent siblings)
4 siblings, 0 replies; 44+ messages in thread
From: Ferruh Yigit @ 2022-01-24 9:47 UTC (permalink / raw)
To: Kumara Parameshwaran; +Cc: dev, Kumara Parameshwaran, Stephen Hemminger
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v2] net/tap: fix to populate fds in secondary process
2022-01-21 4:29 [PATCH] net/tap: Bug fix to populate fds in secondary process Kumara Parameshwaran
2022-01-24 9:47 ` Ferruh Yigit
@ 2022-01-24 12:12 ` Kumara Parameshwaran
2022-01-24 12:35 ` Ferruh Yigit
2022-01-24 12:37 ` Kumara Parameshwaran
` (2 subsequent siblings)
4 siblings, 1 reply; 44+ messages in thread
From: Kumara Parameshwaran @ 2022-01-24 12:12 UTC (permalink / raw)
To: dev; +Cc: ferruh.yigit, Kumara Parameshwaran, stable
From: Kumara Parameshwaran <kparameshwar@vmware.com>
When a tap device is hotplugged to primary process which in turn
adds the device to all secondary process, the secondary process
does a tap_mp_attach_queues, but the fds are not populated in
the primary during the probe they are populated during the queue_setup,
added a fix to sync the queues during rte_eth_dev_start
Fixes: 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")
Cc: stable@dpdk.org
Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
---
v2:
* Addressed review comments to move the function declaration and version
map
drivers/net/tap/rte_eth_tap.c | 196 +++++++++++++---------------------
lib/ethdev/ethdev_driver.h | 17 +++
lib/ethdev/rte_ethdev.c | 11 ++
lib/ethdev/version.map | 2 +
4 files changed, 102 insertions(+), 124 deletions(-)
diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index f1b48cae82..f6c25d7e21 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -66,7 +66,7 @@
(TAP_GSO_MBUFS_PER_CORE * TAP_GSO_MBUF_CACHE_SIZE)
/* IPC key for queue fds sync */
-#define TAP_MP_KEY "tap_mp_sync_queues"
+#define TAP_MP_REQ_START_RXTX "tap_mp_req_start_rxtx"
#define TAP_IOV_DEFAULT_MAX 1024
@@ -880,11 +880,48 @@ tap_link_set_up(struct rte_eth_dev *dev)
return tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
}
+static int tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
+{
+ struct rte_mp_msg msg;
+ struct ipc_queues *request_param = (struct ipc_queues *)msg.param;
+ int err;
+ int fd_iterator = 0;
+ struct pmd_process_private *process_private = dev->process_private;
+ int i;
+
+ memset(&msg, 0, sizeof(msg));
+ strlcpy(msg.name, TAP_MP_REQ_START_RXTX, sizeof(msg.name));
+ strlcpy(request_param->port_name, dev->data->name, sizeof(request_param->port_name));
+ msg.len_param = sizeof(*request_param);
+ for (i = 0; i < dev->data->nb_tx_queues; i++) {
+ msg.fds[fd_iterator++] = process_private->txq_fds[i];
+ msg.num_fds++;
+ request_param->txq_count++;
+ }
+ for (i = 0; i < dev->data->nb_rx_queues; i++) {
+ msg.fds[fd_iterator++] = process_private->rxq_fds[i];
+ msg.num_fds++;
+ request_param->rxq_count++;
+ }
+
+ err = rte_mp_sendmsg(&msg);
+ if (err < 0) {
+ TAP_LOG(ERR, "Failed to send start req to secondary %d",
+ rte_errno);
+ return -1;
+ }
+
+ return 0;
+}
+
static int
tap_dev_start(struct rte_eth_dev *dev)
{
int err, i;
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+ tap_mp_req_on_rxtx(dev);
+
err = tap_intr_handle_set(dev, 1);
if (err)
return err;
@@ -901,6 +938,34 @@ tap_dev_start(struct rte_eth_dev *dev)
return err;
}
+static int
+tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused const void *peer)
+{
+ struct rte_eth_dev *dev;
+ const struct ipc_queues *request_param =
+ (const struct ipc_queues *)request->param;
+ int fd_iterator;
+ int queue;
+ struct pmd_process_private *process_private;
+
+ dev = rte_get_eth_dev_by_name(request_param->port_name);
+ if (!dev) {
+ TAP_LOG(ERR, "Failed to get dev for %s",
+ request_param->port_name);
+ return -1;
+ }
+ process_private = dev->process_private;
+ fd_iterator = 0;
+ TAP_LOG(DEBUG, "tap_attach rx_q:%d tx_q:%d\n", request_param->rxq_count,
+ request_param->txq_count);
+ for (queue = 0; queue < request_param->txq_count; queue++)
+ process_private->txq_fds[queue] = request->fds[fd_iterator++];
+ for (queue = 0; queue < request_param->rxq_count; queue++)
+ process_private->rxq_fds[queue] = request->fds[fd_iterator++];
+
+ return 0;
+}
+
/* This function gets called when the current port gets stopped.
*/
static int
@@ -1084,6 +1149,7 @@ tap_dev_close(struct rte_eth_dev *dev)
if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
rte_free(dev->process_private);
+ rte_mp_action_unregister(TAP_MP_REQ_START_RXTX);
return 0;
}
@@ -1140,8 +1206,6 @@ tap_dev_close(struct rte_eth_dev *dev)
internals->ioctl_sock = -1;
}
rte_free(dev->process_private);
- if (tap_devices_count == 1)
- rte_mp_action_unregister(TAP_MP_KEY);
tap_devices_count--;
/*
* Since TUN device has no more opened file descriptors
@@ -2292,113 +2356,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
return ret;
}
-/* Request queue file descriptors from secondary to primary. */
-static int
-tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
-{
- int ret;
- struct timespec timeout = {.tv_sec = 1, .tv_nsec = 0};
- struct rte_mp_msg request, *reply;
- struct rte_mp_reply replies;
- struct ipc_queues *request_param = (struct ipc_queues *)request.param;
- struct ipc_queues *reply_param;
- struct pmd_process_private *process_private = dev->process_private;
- int queue, fd_iterator;
-
- /* Prepare the request */
- memset(&request, 0, sizeof(request));
- strlcpy(request.name, TAP_MP_KEY, sizeof(request.name));
- strlcpy(request_param->port_name, port_name,
- sizeof(request_param->port_name));
- request.len_param = sizeof(*request_param);
- /* Send request and receive reply */
- ret = rte_mp_request_sync(&request, &replies, &timeout);
- if (ret < 0 || replies.nb_received != 1) {
- TAP_LOG(ERR, "Failed to request queues from primary: %d",
- rte_errno);
- return -1;
- }
- reply = &replies.msgs[0];
- reply_param = (struct ipc_queues *)reply->param;
- TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
-
- /* Attach the queues from received file descriptors */
- if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds) {
- TAP_LOG(ERR, "Unexpected number of fds received");
- return -1;
- }
-
- dev->data->nb_rx_queues = reply_param->rxq_count;
- dev->data->nb_tx_queues = reply_param->txq_count;
- fd_iterator = 0;
- for (queue = 0; queue < reply_param->rxq_count; queue++)
- process_private->rxq_fds[queue] = reply->fds[fd_iterator++];
- for (queue = 0; queue < reply_param->txq_count; queue++)
- process_private->txq_fds[queue] = reply->fds[fd_iterator++];
- free(reply);
- return 0;
-}
-
-/* Send the queue file descriptors from the primary process to secondary. */
-static int
-tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
-{
- struct rte_eth_dev *dev;
- struct pmd_process_private *process_private;
- struct rte_mp_msg reply;
- const struct ipc_queues *request_param =
- (const struct ipc_queues *)request->param;
- struct ipc_queues *reply_param =
- (struct ipc_queues *)reply.param;
- uint16_t port_id;
- int queue;
- int ret;
-
- /* Get requested port */
- TAP_LOG(DEBUG, "Received IPC request for %s", request_param->port_name);
- ret = rte_eth_dev_get_port_by_name(request_param->port_name, &port_id);
- if (ret) {
- TAP_LOG(ERR, "Failed to get port id for %s",
- request_param->port_name);
- return -1;
- }
- dev = &rte_eth_devices[port_id];
- process_private = dev->process_private;
-
- /* Fill file descriptors for all queues */
- reply.num_fds = 0;
- reply_param->rxq_count = 0;
- if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
- RTE_MP_MAX_FD_NUM){
- TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of fds");
- return -1;
- }
-
- for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
- reply.fds[reply.num_fds++] = process_private->rxq_fds[queue];
- reply_param->rxq_count++;
- }
- RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
-
- reply_param->txq_count = 0;
- for (queue = 0; queue < dev->data->nb_tx_queues; queue++) {
- reply.fds[reply.num_fds++] = process_private->txq_fds[queue];
- reply_param->txq_count++;
- }
- RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
-
- /* Send reply */
- strlcpy(reply.name, request->name, sizeof(reply.name));
- strlcpy(reply_param->port_name, request_param->port_name,
- sizeof(reply_param->port_name));
- reply.len_param = sizeof(*reply_param);
- if (rte_mp_reply(&reply, peer) < 0) {
- TAP_LOG(ERR, "Failed to reply an IPC request to sync queues");
- return -1;
- }
- return 0;
-}
-
/* Open a TAP interface device.
*/
static int
@@ -2442,9 +2399,11 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
return -1;
}
- ret = tap_mp_attach_queues(name, eth_dev);
- if (ret != 0)
- return -1;
+ ret = rte_mp_action_register(TAP_MP_REQ_START_RXTX, tap_mp_req_start_rxtx);
+ if (ret < 0 && rte_errno != ENOTSUP)
+ TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
+ strerror(rte_errno));
+
rte_eth_dev_probing_finish(eth_dev);
return 0;
}
@@ -2492,15 +2451,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
TAP_LOG(DEBUG, "Initializing pmd_tap for %s", name);
- /* Register IPC feed callback */
- if (!tap_devices_count) {
- ret = rte_mp_action_register(TAP_MP_KEY, tap_mp_sync_queues);
- if (ret < 0 && rte_errno != ENOTSUP) {
- TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
- strerror(rte_errno));
- goto leave;
- }
- }
tap_devices_count++;
tap_devices_count_increased = 1;
ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,
@@ -2511,8 +2461,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
TAP_LOG(ERR, "Failed to create pmd for %s as %s",
name, tap_name);
if (tap_devices_count_increased == 1) {
- if (tap_devices_count == 1)
- rte_mp_action_unregister(TAP_MP_KEY);
tap_devices_count--;
}
}
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index d95605a355..a08991bcdf 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1629,6 +1629,23 @@ rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue,
struct rte_hairpin_peer_info *peer_info,
uint32_t direction);
+/**
+* Get rte_eth_dev from device name. The device name should be specified
+* as below:
+* - PCIe address (Domain:Bus:Device.Function), for example- 0000:2:00.0
+* - SoC device name, for example- fsl-gmac0
+* - vdev dpdk name, for example- net_[pcap0|null0|tap0]
+*
+* @param name
+* pci address or name of the device
+* @return
+* - rte_eth_dev if successful
+* - NULL on failure
+*/
+__rte_internal
+struct rte_eth_dev*
+rte_get_eth_dev_by_name(const char *name);
+
/**
* @internal
* Reset the current queue state and configuration to disconnect (unbind) it
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index a1d475a292..9192b0d664 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -894,6 +894,17 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
return -ENODEV;
}
+struct rte_eth_dev *
+rte_get_eth_dev_by_name(const char *name)
+{
+ uint16_t pid;
+
+ if (rte_eth_dev_get_port_by_name(name, &pid))
+ return NULL;
+
+ return &rte_eth_devices[pid];
+}
+
static int
eth_err(uint16_t port_id, int ret)
{
diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
index c2fb0669a4..7e3797189b 100644
--- a/lib/ethdev/version.map
+++ b/lib/ethdev/version.map
@@ -256,6 +256,7 @@ EXPERIMENTAL {
rte_flow_flex_item_create;
rte_flow_flex_item_release;
rte_flow_pick_transfer_proxy;
+
};
INTERNAL {
@@ -282,4 +283,5 @@ INTERNAL {
rte_eth_representor_id_get;
rte_eth_switch_domain_alloc;
rte_eth_switch_domain_free;
+ rte_get_eth_dev_by_name;
};
--
2.17.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2] net/tap: fix to populate fds in secondary process
2022-01-24 12:12 ` [PATCH v2] net/tap: " Kumara Parameshwaran
@ 2022-01-24 12:35 ` Ferruh Yigit
2022-01-24 12:48 ` Kumara Parameshwaran
` (2 more replies)
0 siblings, 3 replies; 44+ messages in thread
From: Ferruh Yigit @ 2022-01-24 12:35 UTC (permalink / raw)
To: Kumara Parameshwaran, dev; +Cc: Kumara Parameshwaran, stable
On 1/24/2022 12:12 PM, Kumara Parameshwaran wrote:
> From: Kumara Parameshwaran <kparameshwar@vmware.com>
>
> When a tap device is hotplugged to primary process which in turn
> adds the device to all secondary process, the secondary process
> does a tap_mp_attach_queues, but the fds are not populated in
> the primary during the probe they are populated during the queue_setup,
> added a fix to sync the queues during rte_eth_dev_start
>
> Fixes: 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")
> Cc: stable@dpdk.org
>
> Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
> ---
>
> v2:
> * Addressed review comments to move the function declaration and version
> map
>
Thanks for adding patch version.
> drivers/net/tap/rte_eth_tap.c | 196 +++++++++++++---------------------
> lib/ethdev/ethdev_driver.h | 17 +++
> lib/ethdev/rte_ethdev.c | 11 ++
> lib/ethdev/version.map | 2 +
> 4 files changed, 102 insertions(+), 124 deletions(-)
>
Can you please separate etdev (API) changes to another patch, so this will be a patchset
with two patches,
first patch adds ethdev API
second patch is tap patch, that uses the API in the first patch
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index f1b48cae82..f6c25d7e21 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -66,7 +66,7 @@
> (TAP_GSO_MBUFS_PER_CORE * TAP_GSO_MBUF_CACHE_SIZE)
>
> /* IPC key for queue fds sync */
> -#define TAP_MP_KEY "tap_mp_sync_queues"
> +#define TAP_MP_REQ_START_RXTX "tap_mp_req_start_rxtx"
>
We said we can drop "tap_mp_sync_queues", but thinking twice,
will current implementation cover following usecase:
- Primary applicaiton started with tap interface, all config, setup,
start done
- Secondary app started without any parameter
Since primary already started, I think secondary fds will be wrong,
what do you think?
> #define TAP_IOV_DEFAULT_MAX 1024
>
> @@ -880,11 +880,48 @@ tap_link_set_up(struct rte_eth_dev *dev)
> return tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
> }
>
> +static int tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
Can you please follow the coding convention:
static int
tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
{
> +{
> + struct rte_mp_msg msg;
> + struct ipc_queues *request_param = (struct ipc_queues *)msg.param;
> + int err;
> + int fd_iterator = 0;
> + struct pmd_process_private *process_private = dev->process_private;
> + int i;
> +
> + memset(&msg, 0, sizeof(msg));
> + strlcpy(msg.name, TAP_MP_REQ_START_RXTX, sizeof(msg.name));
> + strlcpy(request_param->port_name, dev->data->name, sizeof(request_param->port_name));
> + msg.len_param = sizeof(*request_param);
> + for (i = 0; i < dev->data->nb_tx_queues; i++) {
> + msg.fds[fd_iterator++] = process_private->txq_fds[i];
> + msg.num_fds++;
> + request_param->txq_count++;
> + }
> + for (i = 0; i < dev->data->nb_rx_queues; i++) {
> + msg.fds[fd_iterator++] = process_private->rxq_fds[i];
> + msg.num_fds++;
> + request_param->rxq_count++;
> + }
> +
> + err = rte_mp_sendmsg(&msg);
> + if (err < 0) {
> + TAP_LOG(ERR, "Failed to send start req to secondary %d",
> + rte_errno);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> static int
> tap_dev_start(struct rte_eth_dev *dev)
> {
> int err, i;
>
> + if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> + tap_mp_req_on_rxtx(dev);
> +
> err = tap_intr_handle_set(dev, 1);
> if (err)
> return err;
> @@ -901,6 +938,34 @@ tap_dev_start(struct rte_eth_dev *dev)
> return err;
> }
>
> +static int
> +tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused const void *peer)
> +{
I asked last time but I don't remember the response,
what should be in the 'peer' variable?
> + struct rte_eth_dev *dev;
> + const struct ipc_queues *request_param =
> + (const struct ipc_queues *)request->param;
> + int fd_iterator;
> + int queue;
> + struct pmd_process_private *process_private;
> +
> + dev = rte_get_eth_dev_by_name(request_param->port_name);
> + if (!dev) {
> + TAP_LOG(ERR, "Failed to get dev for %s",
> + request_param->port_name);
> + return -1;
> + }
> + process_private = dev->process_private;
> + fd_iterator = 0;
> + TAP_LOG(DEBUG, "tap_attach rx_q:%d tx_q:%d\n", request_param->rxq_count,
> + request_param->txq_count);
> + for (queue = 0; queue < request_param->txq_count; queue++)
> + process_private->txq_fds[queue] = request->fds[fd_iterator++];
> + for (queue = 0; queue < request_param->rxq_count; queue++)
> + process_private->rxq_fds[queue] = request->fds[fd_iterator++];
> +
> + return 0;
> +}
> +
> /* This function gets called when the current port gets stopped.
> */
> static int
> @@ -1084,6 +1149,7 @@ tap_dev_close(struct rte_eth_dev *dev)
>
> if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> rte_free(dev->process_private);
> + rte_mp_action_unregister(TAP_MP_REQ_START_RXTX);
> return 0;
> }
>
> @@ -1140,8 +1206,6 @@ tap_dev_close(struct rte_eth_dev *dev)
> internals->ioctl_sock = -1;
> }
> rte_free(dev->process_private);
> - if (tap_devices_count == 1)
> - rte_mp_action_unregister(TAP_MP_KEY);
> tap_devices_count--;
> /*
> * Since TUN device has no more opened file descriptors
> @@ -2292,113 +2356,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
> return ret;
> }
>
> -/* Request queue file descriptors from secondary to primary. */
> -static int
> -tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
> -{
> - int ret;
> - struct timespec timeout = {.tv_sec = 1, .tv_nsec = 0};
> - struct rte_mp_msg request, *reply;
> - struct rte_mp_reply replies;
> - struct ipc_queues *request_param = (struct ipc_queues *)request.param;
> - struct ipc_queues *reply_param;
> - struct pmd_process_private *process_private = dev->process_private;
> - int queue, fd_iterator;
> -
> - /* Prepare the request */
> - memset(&request, 0, sizeof(request));
> - strlcpy(request.name, TAP_MP_KEY, sizeof(request.name));
> - strlcpy(request_param->port_name, port_name,
> - sizeof(request_param->port_name));
> - request.len_param = sizeof(*request_param);
> - /* Send request and receive reply */
> - ret = rte_mp_request_sync(&request, &replies, &timeout);
> - if (ret < 0 || replies.nb_received != 1) {
> - TAP_LOG(ERR, "Failed to request queues from primary: %d",
> - rte_errno);
> - return -1;
> - }
> - reply = &replies.msgs[0];
> - reply_param = (struct ipc_queues *)reply->param;
> - TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
> -
> - /* Attach the queues from received file descriptors */
> - if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds) {
> - TAP_LOG(ERR, "Unexpected number of fds received");
> - return -1;
> - }
> -
> - dev->data->nb_rx_queues = reply_param->rxq_count;
> - dev->data->nb_tx_queues = reply_param->txq_count;
> - fd_iterator = 0;
> - for (queue = 0; queue < reply_param->rxq_count; queue++)
> - process_private->rxq_fds[queue] = reply->fds[fd_iterator++];
> - for (queue = 0; queue < reply_param->txq_count; queue++)
> - process_private->txq_fds[queue] = reply->fds[fd_iterator++];
> - free(reply);
> - return 0;
> -}
> -
> -/* Send the queue file descriptors from the primary process to secondary. */
> -static int
> -tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
> -{
> - struct rte_eth_dev *dev;
> - struct pmd_process_private *process_private;
> - struct rte_mp_msg reply;
> - const struct ipc_queues *request_param =
> - (const struct ipc_queues *)request->param;
> - struct ipc_queues *reply_param =
> - (struct ipc_queues *)reply.param;
> - uint16_t port_id;
> - int queue;
> - int ret;
> -
> - /* Get requested port */
> - TAP_LOG(DEBUG, "Received IPC request for %s", request_param->port_name);
> - ret = rte_eth_dev_get_port_by_name(request_param->port_name, &port_id);
> - if (ret) {
> - TAP_LOG(ERR, "Failed to get port id for %s",
> - request_param->port_name);
> - return -1;
> - }
> - dev = &rte_eth_devices[port_id];
> - process_private = dev->process_private;
> -
> - /* Fill file descriptors for all queues */
> - reply.num_fds = 0;
> - reply_param->rxq_count = 0;
> - if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
> - RTE_MP_MAX_FD_NUM){
> - TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of fds");
> - return -1;
> - }
> -
> - for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
> - reply.fds[reply.num_fds++] = process_private->rxq_fds[queue];
> - reply_param->rxq_count++;
> - }
> - RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
> -
> - reply_param->txq_count = 0;
> - for (queue = 0; queue < dev->data->nb_tx_queues; queue++) {
> - reply.fds[reply.num_fds++] = process_private->txq_fds[queue];
> - reply_param->txq_count++;
> - }
> - RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
> -
> - /* Send reply */
> - strlcpy(reply.name, request->name, sizeof(reply.name));
> - strlcpy(reply_param->port_name, request_param->port_name,
> - sizeof(reply_param->port_name));
> - reply.len_param = sizeof(*reply_param);
> - if (rte_mp_reply(&reply, peer) < 0) {
> - TAP_LOG(ERR, "Failed to reply an IPC request to sync queues");
> - return -1;
> - }
> - return 0;
> -}
> -
> /* Open a TAP interface device.
> */
> static int
> @@ -2442,9 +2399,11 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> return -1;
> }
>
> - ret = tap_mp_attach_queues(name, eth_dev);
> - if (ret != 0)
> - return -1;
> + ret = rte_mp_action_register(TAP_MP_REQ_START_RXTX, tap_mp_req_start_rxtx);
> + if (ret < 0 && rte_errno != ENOTSUP)
> + TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
> + strerror(rte_errno));
> +
> rte_eth_dev_probing_finish(eth_dev);
> return 0;
> }
> @@ -2492,15 +2451,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
>
> TAP_LOG(DEBUG, "Initializing pmd_tap for %s", name);
>
> - /* Register IPC feed callback */
> - if (!tap_devices_count) {
> - ret = rte_mp_action_register(TAP_MP_KEY, tap_mp_sync_queues);
> - if (ret < 0 && rte_errno != ENOTSUP) {
> - TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
> - strerror(rte_errno));
> - goto leave;
> - }
> - }
> tap_devices_count++;
> tap_devices_count_increased = 1;
> ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,
> @@ -2511,8 +2461,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> TAP_LOG(ERR, "Failed to create pmd for %s as %s",
> name, tap_name);
> if (tap_devices_count_increased == 1) {
> - if (tap_devices_count == 1)
> - rte_mp_action_unregister(TAP_MP_KEY);
> tap_devices_count--;
> }
> }
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index d95605a355..a08991bcdf 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1629,6 +1629,23 @@ rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue,
> struct rte_hairpin_peer_info *peer_info,
> uint32_t direction);
>
> +/**
Please add '@internal' tag into doxygen comment.
> +* Get rte_eth_dev from device name. The device name should be specified
> +* as below:
> +* - PCIe address (Domain:Bus:Device.Function), for example- 0000:2:00.0
> +* - SoC device name, for example- fsl-gmac0
> +* - vdev dpdk name, for example- net_[pcap0|null0|tap0]
> +*
> +* @param name
> +* pci address or name of the device
> +* @return
> +* - rte_eth_dev if successful
> +* - NULL on failure
> +*/
> +__rte_internal
> +struct rte_eth_dev*
> +rte_get_eth_dev_by_name(const char *name);
As the API name, better to start with 'rte_eth_' prefix to be consistent with
rest of the APIs.
I suggest 'rte_eth_dev_get_by_name' but feel free to chose better one.
> +
> /**
> * @internal
> * Reset the current queue state and configuration to disconnect (unbind) it
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index a1d475a292..9192b0d664 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -894,6 +894,17 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
> return -ENODEV;
> }
>
> +struct rte_eth_dev *
> +rte_get_eth_dev_by_name(const char *name)
> +{
> + uint16_t pid;
> +
> + if (rte_eth_dev_get_port_by_name(name, &pid))
> + return NULL;
> +
> + return &rte_eth_devices[pid];
> +}
> +
> static int
> eth_err(uint16_t port_id, int ret)
> {
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index c2fb0669a4..7e3797189b 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -256,6 +256,7 @@ EXPERIMENTAL {
> rte_flow_flex_item_create;
> rte_flow_flex_item_release;
> rte_flow_pick_transfer_proxy;
> +
This is unintendent change.
> };
>
> INTERNAL {
> @@ -282,4 +283,5 @@ INTERNAL {
> rte_eth_representor_id_get;
> rte_eth_switch_domain_alloc;
> rte_eth_switch_domain_free;
> + rte_get_eth_dev_by_name;
Please add in a sorted way.
> };
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v2] net/tap: fix to populate fds in secondary process
2022-01-21 4:29 [PATCH] net/tap: Bug fix to populate fds in secondary process Kumara Parameshwaran
2022-01-24 9:47 ` Ferruh Yigit
2022-01-24 12:12 ` [PATCH v2] net/tap: " Kumara Parameshwaran
@ 2022-01-24 12:37 ` Kumara Parameshwaran
2022-01-24 13:06 ` Ferruh Yigit
2022-01-31 14:21 ` [PATCH v3 1/2] ethdev: define a function to get eth dev structure Kumara Parameshwaran
2022-01-31 14:32 ` [PATCH v3 1/2] ethdev: define a function to get eth dev structure Kumara Parameshwaran
4 siblings, 1 reply; 44+ messages in thread
From: Kumara Parameshwaran @ 2022-01-24 12:37 UTC (permalink / raw)
To: dev; +Cc: ferruh.yigit, Kumara Parameshwaran, stable
From: Kumara Parameshwaran <kparameshwar@vmware.com>
When a tap device is hotplugged to primary process which in turn
adds the device to all secondary process, the secondary process
does a tap_mp_attach_queues, but the fds are not populated in
the primary during the probe they are populated during the queue_setup,
added a fix to sync the queues during rte_eth_dev_start
Fixes: 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")
Cc: stable@dpdk.org
Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
---
drivers/net/tap/rte_eth_tap.c | 196 +++++++++++++---------------------
lib/ethdev/ethdev_driver.h | 18 ++++
lib/ethdev/rte_ethdev.c | 11 ++
lib/ethdev/version.map | 1 +
4 files changed, 102 insertions(+), 124 deletions(-)
diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index f1b48cae82..f6c25d7e21 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -66,7 +66,7 @@
(TAP_GSO_MBUFS_PER_CORE * TAP_GSO_MBUF_CACHE_SIZE)
/* IPC key for queue fds sync */
-#define TAP_MP_KEY "tap_mp_sync_queues"
+#define TAP_MP_REQ_START_RXTX "tap_mp_req_start_rxtx"
#define TAP_IOV_DEFAULT_MAX 1024
@@ -880,11 +880,48 @@ tap_link_set_up(struct rte_eth_dev *dev)
return tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
}
+static int tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
+{
+ struct rte_mp_msg msg;
+ struct ipc_queues *request_param = (struct ipc_queues *)msg.param;
+ int err;
+ int fd_iterator = 0;
+ struct pmd_process_private *process_private = dev->process_private;
+ int i;
+
+ memset(&msg, 0, sizeof(msg));
+ strlcpy(msg.name, TAP_MP_REQ_START_RXTX, sizeof(msg.name));
+ strlcpy(request_param->port_name, dev->data->name, sizeof(request_param->port_name));
+ msg.len_param = sizeof(*request_param);
+ for (i = 0; i < dev->data->nb_tx_queues; i++) {
+ msg.fds[fd_iterator++] = process_private->txq_fds[i];
+ msg.num_fds++;
+ request_param->txq_count++;
+ }
+ for (i = 0; i < dev->data->nb_rx_queues; i++) {
+ msg.fds[fd_iterator++] = process_private->rxq_fds[i];
+ msg.num_fds++;
+ request_param->rxq_count++;
+ }
+
+ err = rte_mp_sendmsg(&msg);
+ if (err < 0) {
+ TAP_LOG(ERR, "Failed to send start req to secondary %d",
+ rte_errno);
+ return -1;
+ }
+
+ return 0;
+}
+
static int
tap_dev_start(struct rte_eth_dev *dev)
{
int err, i;
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+ tap_mp_req_on_rxtx(dev);
+
err = tap_intr_handle_set(dev, 1);
if (err)
return err;
@@ -901,6 +938,34 @@ tap_dev_start(struct rte_eth_dev *dev)
return err;
}
+static int
+tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused const void *peer)
+{
+ struct rte_eth_dev *dev;
+ const struct ipc_queues *request_param =
+ (const struct ipc_queues *)request->param;
+ int fd_iterator;
+ int queue;
+ struct pmd_process_private *process_private;
+
+ dev = rte_get_eth_dev_by_name(request_param->port_name);
+ if (!dev) {
+ TAP_LOG(ERR, "Failed to get dev for %s",
+ request_param->port_name);
+ return -1;
+ }
+ process_private = dev->process_private;
+ fd_iterator = 0;
+ TAP_LOG(DEBUG, "tap_attach rx_q:%d tx_q:%d\n", request_param->rxq_count,
+ request_param->txq_count);
+ for (queue = 0; queue < request_param->txq_count; queue++)
+ process_private->txq_fds[queue] = request->fds[fd_iterator++];
+ for (queue = 0; queue < request_param->rxq_count; queue++)
+ process_private->rxq_fds[queue] = request->fds[fd_iterator++];
+
+ return 0;
+}
+
/* This function gets called when the current port gets stopped.
*/
static int
@@ -1084,6 +1149,7 @@ tap_dev_close(struct rte_eth_dev *dev)
if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
rte_free(dev->process_private);
+ rte_mp_action_unregister(TAP_MP_REQ_START_RXTX);
return 0;
}
@@ -1140,8 +1206,6 @@ tap_dev_close(struct rte_eth_dev *dev)
internals->ioctl_sock = -1;
}
rte_free(dev->process_private);
- if (tap_devices_count == 1)
- rte_mp_action_unregister(TAP_MP_KEY);
tap_devices_count--;
/*
* Since TUN device has no more opened file descriptors
@@ -2292,113 +2356,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
return ret;
}
-/* Request queue file descriptors from secondary to primary. */
-static int
-tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
-{
- int ret;
- struct timespec timeout = {.tv_sec = 1, .tv_nsec = 0};
- struct rte_mp_msg request, *reply;
- struct rte_mp_reply replies;
- struct ipc_queues *request_param = (struct ipc_queues *)request.param;
- struct ipc_queues *reply_param;
- struct pmd_process_private *process_private = dev->process_private;
- int queue, fd_iterator;
-
- /* Prepare the request */
- memset(&request, 0, sizeof(request));
- strlcpy(request.name, TAP_MP_KEY, sizeof(request.name));
- strlcpy(request_param->port_name, port_name,
- sizeof(request_param->port_name));
- request.len_param = sizeof(*request_param);
- /* Send request and receive reply */
- ret = rte_mp_request_sync(&request, &replies, &timeout);
- if (ret < 0 || replies.nb_received != 1) {
- TAP_LOG(ERR, "Failed to request queues from primary: %d",
- rte_errno);
- return -1;
- }
- reply = &replies.msgs[0];
- reply_param = (struct ipc_queues *)reply->param;
- TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
-
- /* Attach the queues from received file descriptors */
- if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds) {
- TAP_LOG(ERR, "Unexpected number of fds received");
- return -1;
- }
-
- dev->data->nb_rx_queues = reply_param->rxq_count;
- dev->data->nb_tx_queues = reply_param->txq_count;
- fd_iterator = 0;
- for (queue = 0; queue < reply_param->rxq_count; queue++)
- process_private->rxq_fds[queue] = reply->fds[fd_iterator++];
- for (queue = 0; queue < reply_param->txq_count; queue++)
- process_private->txq_fds[queue] = reply->fds[fd_iterator++];
- free(reply);
- return 0;
-}
-
-/* Send the queue file descriptors from the primary process to secondary. */
-static int
-tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
-{
- struct rte_eth_dev *dev;
- struct pmd_process_private *process_private;
- struct rte_mp_msg reply;
- const struct ipc_queues *request_param =
- (const struct ipc_queues *)request->param;
- struct ipc_queues *reply_param =
- (struct ipc_queues *)reply.param;
- uint16_t port_id;
- int queue;
- int ret;
-
- /* Get requested port */
- TAP_LOG(DEBUG, "Received IPC request for %s", request_param->port_name);
- ret = rte_eth_dev_get_port_by_name(request_param->port_name, &port_id);
- if (ret) {
- TAP_LOG(ERR, "Failed to get port id for %s",
- request_param->port_name);
- return -1;
- }
- dev = &rte_eth_devices[port_id];
- process_private = dev->process_private;
-
- /* Fill file descriptors for all queues */
- reply.num_fds = 0;
- reply_param->rxq_count = 0;
- if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
- RTE_MP_MAX_FD_NUM){
- TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of fds");
- return -1;
- }
-
- for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
- reply.fds[reply.num_fds++] = process_private->rxq_fds[queue];
- reply_param->rxq_count++;
- }
- RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
-
- reply_param->txq_count = 0;
- for (queue = 0; queue < dev->data->nb_tx_queues; queue++) {
- reply.fds[reply.num_fds++] = process_private->txq_fds[queue];
- reply_param->txq_count++;
- }
- RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
-
- /* Send reply */
- strlcpy(reply.name, request->name, sizeof(reply.name));
- strlcpy(reply_param->port_name, request_param->port_name,
- sizeof(reply_param->port_name));
- reply.len_param = sizeof(*reply_param);
- if (rte_mp_reply(&reply, peer) < 0) {
- TAP_LOG(ERR, "Failed to reply an IPC request to sync queues");
- return -1;
- }
- return 0;
-}
-
/* Open a TAP interface device.
*/
static int
@@ -2442,9 +2399,11 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
return -1;
}
- ret = tap_mp_attach_queues(name, eth_dev);
- if (ret != 0)
- return -1;
+ ret = rte_mp_action_register(TAP_MP_REQ_START_RXTX, tap_mp_req_start_rxtx);
+ if (ret < 0 && rte_errno != ENOTSUP)
+ TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
+ strerror(rte_errno));
+
rte_eth_dev_probing_finish(eth_dev);
return 0;
}
@@ -2492,15 +2451,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
TAP_LOG(DEBUG, "Initializing pmd_tap for %s", name);
- /* Register IPC feed callback */
- if (!tap_devices_count) {
- ret = rte_mp_action_register(TAP_MP_KEY, tap_mp_sync_queues);
- if (ret < 0 && rte_errno != ENOTSUP) {
- TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
- strerror(rte_errno));
- goto leave;
- }
- }
tap_devices_count++;
tap_devices_count_increased = 1;
ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,
@@ -2511,8 +2461,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
TAP_LOG(ERR, "Failed to create pmd for %s as %s",
name, tap_name);
if (tap_devices_count_increased == 1) {
- if (tap_devices_count == 1)
- rte_mp_action_unregister(TAP_MP_KEY);
tap_devices_count--;
}
}
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index d95605a355..04b3a9f933 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1629,6 +1629,24 @@ rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue,
struct rte_hairpin_peer_info *peer_info,
uint32_t direction);
+/**
+ * @internal
+ * Get rte_eth_dev from device name. The device name should be specified
+ * as below:
+ * - PCIe address (Domain:Bus:Device.Function), for example- 0000:2:00.0
+ * - SoC device name, for example- fsl-gmac0
+ * - vdev dpdk name, for example- net_[pcap0|null0|tap0]
+ *
+ * @param name
+ * pci address or name of the device
+ * @return
+ * - rte_eth_dev if successful
+ * - NULL on failure
+ */
+__rte_internal
+struct rte_eth_dev*
+rte_get_eth_dev_by_name(const char *name);
+
/**
* @internal
* Reset the current queue state and configuration to disconnect (unbind) it
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index a1d475a292..9192b0d664 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -894,6 +894,17 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
return -ENODEV;
}
+struct rte_eth_dev *
+rte_get_eth_dev_by_name(const char *name)
+{
+ uint16_t pid;
+
+ if (rte_eth_dev_get_port_by_name(name, &pid))
+ return NULL;
+
+ return &rte_eth_devices[pid];
+}
+
static int
eth_err(uint16_t port_id, int ret)
{
diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
index c2fb0669a4..23ca0e8865 100644
--- a/lib/ethdev/version.map
+++ b/lib/ethdev/version.map
@@ -282,4 +282,5 @@ INTERNAL {
rte_eth_representor_id_get;
rte_eth_switch_domain_alloc;
rte_eth_switch_domain_free;
+ rte_get_eth_dev_by_name;
};
--
2.17.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2] net/tap: fix to populate fds in secondary process
2022-01-24 12:35 ` Ferruh Yigit
@ 2022-01-24 12:48 ` Kumara Parameshwaran
2022-01-24 12:54 ` kumaraparameshwaran rathinavel
2022-01-28 9:55 ` kumaraparameshwaran rathinavel
2 siblings, 0 replies; 44+ messages in thread
From: Kumara Parameshwaran @ 2022-01-24 12:48 UTC (permalink / raw)
To: Ferruh Yigit, Kumara Parameshwaran, dev; +Cc: stable
[-- Attachment #1: Type: text/plain, Size: 15383 bytes --]
________________________________
From: Ferruh Yigit <ferruh.yigit@intel.com>
Sent: 24 January 2022 18:05
To: Kumara Parameshwaran <kumaraparamesh92@gmail.com>; dev@dpdk.org <dev@dpdk.org>
Cc: Kumara Parameshwaran <kparameshwar@vmware.com>; stable@dpdk.org <stable@dpdk.org>
Subject: Re: [PATCH v2] net/tap: fix to populate fds in secondary process
On 1/24/2022 12:12 PM, Kumara Parameshwaran wrote:
> From: Kumara Parameshwaran <kparameshwar@vmware.com>
>
> When a tap device is hotplugged to primary process which in turn
> adds the device to all secondary process, the secondary process
> does a tap_mp_attach_queues, but the fds are not populated in
> the primary during the probe they are populated during the queue_setup,
> added a fix to sync the queues during rte_eth_dev_start
>
> Fixes: 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")
> Cc: stable@dpdk.org
>
> Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
> ---
>
> v2:
> * Addressed review comments to move the function declaration and version
> map
>
Thanks for adding patch version.
> drivers/net/tap/rte_eth_tap.c | 196 +++++++++++++---------------------
> lib/ethdev/ethdev_driver.h | 17 +++
> lib/ethdev/rte_ethdev.c | 11 ++
> lib/ethdev/version.map | 2 +
> 4 files changed, 102 insertions(+), 124 deletions(-)
>
Can you please separate etdev (API) changes to another patch, so this will be a patchset
with two patches,
first patch adds ethdev API
second patch is tap patch, that uses the API in the first patch
Sure, will do it.
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index f1b48cae82..f6c25d7e21 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -66,7 +66,7 @@
> (TAP_GSO_MBUFS_PER_CORE * TAP_GSO_MBUF_CACHE_SIZE)
>
> /* IPC key for queue fds sync */
> -#define TAP_MP_KEY "tap_mp_sync_queues"
> +#define TAP_MP_REQ_START_RXTX "tap_mp_req_start_rxtx"
>
We said we can drop "tap_mp_sync_queues", but thinking twice,
will current implementation cover following usecase:
- Primary applicaiton started with tap interface, all config, setup,
start done
- Secondary app started without any parameter
Since primary already started, I think secondary fds will be wrong,
what do you think?
Yes, that is true. We should retain the current behaviour of start for the attach.
> #define TAP_IOV_DEFAULT_MAX 1024
>
> @@ -880,11 +880,48 @@ tap_link_set_up(struct rte_eth_dev *dev)
> return tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
> }
>
> +static int tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
Can you please follow the coding convention:
static int
tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
{
> +{
> + struct rte_mp_msg msg;
> + struct ipc_queues *request_param = (struct ipc_queues *)msg.param;
> + int err;
> + int fd_iterator = 0;
> + struct pmd_process_private *process_private = dev->process_private;
> + int i;
> +
> + memset(&msg, 0, sizeof(msg));
> + strlcpy(msg.name, TAP_MP_REQ_START_RXTX, sizeof(msg.name));
> + strlcpy(request_param->port_name, dev->data->name, sizeof(request_param->port_name));
> + msg.len_param = sizeof(*request_param);
> + for (i = 0; i < dev->data->nb_tx_queues; i++) {
> + msg.fds[fd_iterator++] = process_private->txq_fds[i];
> + msg.num_fds++;
> + request_param->txq_count++;
> + }
> + for (i = 0; i < dev->data->nb_rx_queues; i++) {
> + msg.fds[fd_iterator++] = process_private->rxq_fds[i];
> + msg.num_fds++;
> + request_param->rxq_count++;
> + }
> +
> + err = rte_mp_sendmsg(&msg);
> + if (err < 0) {
> + TAP_LOG(ERR, "Failed to send start req to secondary %d",
> + rte_errno);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> static int
> tap_dev_start(struct rte_eth_dev *dev)
> {
> int err, i;
>
> + if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> + tap_mp_req_on_rxtx(dev);
> +
> err = tap_intr_handle_set(dev, 1);
> if (err)
> return err;
> @@ -901,6 +938,34 @@ tap_dev_start(struct rte_eth_dev *dev)
> return err;
> }
>
> +static int
> +tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused const void *peer)
> +{
I asked last time but I don't remember the response,
what should be in the 'peer' variable?
> + struct rte_eth_dev *dev;
> + const struct ipc_queues *request_param =
> + (const struct ipc_queues *)request->param;
> + int fd_iterator;
> + int queue;
> + struct pmd_process_private *process_private;
> +
> + dev = rte_get_eth_dev_by_name(request_param->port_name);
> + if (!dev) {
> + TAP_LOG(ERR, "Failed to get dev for %s",
> + request_param->port_name);
> + return -1;
> + }
> + process_private = dev->process_private;
> + fd_iterator = 0;
> + TAP_LOG(DEBUG, "tap_attach rx_q:%d tx_q:%d\n", request_param->rxq_count,
> + request_param->txq_count);
> + for (queue = 0; queue < request_param->txq_count; queue++)
> + process_private->txq_fds[queue] = request->fds[fd_iterator++];
> + for (queue = 0; queue < request_param->rxq_count; queue++)
> + process_private->rxq_fds[queue] = request->fds[fd_iterator++];
> +
> + return 0;
> +}
> +
> /* This function gets called when the current port gets stopped.
> */
> static int
> @@ -1084,6 +1149,7 @@ tap_dev_close(struct rte_eth_dev *dev)
>
> if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> rte_free(dev->process_private);
> + rte_mp_action_unregister(TAP_MP_REQ_START_RXTX);
> return 0;
> }
>
> @@ -1140,8 +1206,6 @@ tap_dev_close(struct rte_eth_dev *dev)
> internals->ioctl_sock = -1;
> }
> rte_free(dev->process_private);
> - if (tap_devices_count == 1)
> - rte_mp_action_unregister(TAP_MP_KEY);
> tap_devices_count--;
> /*
> * Since TUN device has no more opened file descriptors
> @@ -2292,113 +2356,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
> return ret;
> }
>
> -/* Request queue file descriptors from secondary to primary. */
> -static int
> -tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
> -{
> - int ret;
> - struct timespec timeout = {.tv_sec = 1, .tv_nsec = 0};
> - struct rte_mp_msg request, *reply;
> - struct rte_mp_reply replies;
> - struct ipc_queues *request_param = (struct ipc_queues *)request.param;
> - struct ipc_queues *reply_param;
> - struct pmd_process_private *process_private = dev->process_private;
> - int queue, fd_iterator;
> -
> - /* Prepare the request */
> - memset(&request, 0, sizeof(request));
> - strlcpy(request.name, TAP_MP_KEY, sizeof(request.name));
> - strlcpy(request_param->port_name, port_name,
> - sizeof(request_param->port_name));
> - request.len_param = sizeof(*request_param);
> - /* Send request and receive reply */
> - ret = rte_mp_request_sync(&request, &replies, &timeout);
> - if (ret < 0 || replies.nb_received != 1) {
> - TAP_LOG(ERR, "Failed to request queues from primary: %d",
> - rte_errno);
> - return -1;
> - }
> - reply = &replies.msgs[0];
> - reply_param = (struct ipc_queues *)reply->param;
> - TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
> -
> - /* Attach the queues from received file descriptors */
> - if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds) {
> - TAP_LOG(ERR, "Unexpected number of fds received");
> - return -1;
> - }
> -
> - dev->data->nb_rx_queues = reply_param->rxq_count;
> - dev->data->nb_tx_queues = reply_param->txq_count;
> - fd_iterator = 0;
> - for (queue = 0; queue < reply_param->rxq_count; queue++)
> - process_private->rxq_fds[queue] = reply->fds[fd_iterator++];
> - for (queue = 0; queue < reply_param->txq_count; queue++)
> - process_private->txq_fds[queue] = reply->fds[fd_iterator++];
> - free(reply);
> - return 0;
> -}
> -
> -/* Send the queue file descriptors from the primary process to secondary. */
> -static int
> -tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
> -{
> - struct rte_eth_dev *dev;
> - struct pmd_process_private *process_private;
> - struct rte_mp_msg reply;
> - const struct ipc_queues *request_param =
> - (const struct ipc_queues *)request->param;
> - struct ipc_queues *reply_param =
> - (struct ipc_queues *)reply.param;
> - uint16_t port_id;
> - int queue;
> - int ret;
> -
> - /* Get requested port */
> - TAP_LOG(DEBUG, "Received IPC request for %s", request_param->port_name);
> - ret = rte_eth_dev_get_port_by_name(request_param->port_name, &port_id);
> - if (ret) {
> - TAP_LOG(ERR, "Failed to get port id for %s",
> - request_param->port_name);
> - return -1;
> - }
> - dev = &rte_eth_devices[port_id];
> - process_private = dev->process_private;
> -
> - /* Fill file descriptors for all queues */
> - reply.num_fds = 0;
> - reply_param->rxq_count = 0;
> - if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
> - RTE_MP_MAX_FD_NUM){
> - TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of fds");
> - return -1;
> - }
> -
> - for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
> - reply.fds[reply.num_fds++] = process_private->rxq_fds[queue];
> - reply_param->rxq_count++;
> - }
> - RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
> -
> - reply_param->txq_count = 0;
> - for (queue = 0; queue < dev->data->nb_tx_queues; queue++) {
> - reply.fds[reply.num_fds++] = process_private->txq_fds[queue];
> - reply_param->txq_count++;
> - }
> - RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
> -
> - /* Send reply */
> - strlcpy(reply.name, request->name, sizeof(reply.name));
> - strlcpy(reply_param->port_name, request_param->port_name,
> - sizeof(reply_param->port_name));
> - reply.len_param = sizeof(*reply_param);
> - if (rte_mp_reply(&reply, peer) < 0) {
> - TAP_LOG(ERR, "Failed to reply an IPC request to sync queues");
> - return -1;
> - }
> - return 0;
> -}
> -
> /* Open a TAP interface device.
> */
> static int
> @@ -2442,9 +2399,11 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> return -1;
> }
>
> - ret = tap_mp_attach_queues(name, eth_dev);
> - if (ret != 0)
> - return -1;
> + ret = rte_mp_action_register(TAP_MP_REQ_START_RXTX, tap_mp_req_start_rxtx);
> + if (ret < 0 && rte_errno != ENOTSUP)
> + TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
> + strerror(rte_errno));
> +
> rte_eth_dev_probing_finish(eth_dev);
> return 0;
> }
> @@ -2492,15 +2451,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
>
> TAP_LOG(DEBUG, "Initializing pmd_tap for %s", name);
>
> - /* Register IPC feed callback */
> - if (!tap_devices_count) {
> - ret = rte_mp_action_register(TAP_MP_KEY, tap_mp_sync_queues);
> - if (ret < 0 && rte_errno != ENOTSUP) {
> - TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
> - strerror(rte_errno));
> - goto leave;
> - }
> - }
> tap_devices_count++;
> tap_devices_count_increased = 1;
> ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,
> @@ -2511,8 +2461,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> TAP_LOG(ERR, "Failed to create pmd for %s as %s",
> name, tap_name);
> if (tap_devices_count_increased == 1) {
> - if (tap_devices_count == 1)
> - rte_mp_action_unregister(TAP_MP_KEY);
> tap_devices_count--;
> }
> }
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index d95605a355..a08991bcdf 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1629,6 +1629,23 @@ rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue,
> struct rte_hairpin_peer_info *peer_info,
> uint32_t direction);
>
> +/**
Please add '@internal' tag into doxygen comment.
> +* Get rte_eth_dev from device name. The device name should be specified
> +* as below:
> +* - PCIe address (Domain:Bus:Device.Function), for example- 0000:2:00.0
> +* - SoC device name, for example- fsl-gmac0
> +* - vdev dpdk name, for example- net_[pcap0|null0|tap0]
> +*
> +* @param name
> +* pci address or name of the device
> +* @return
> +* - rte_eth_dev if successful
> +* - NULL on failure
> +*/
> +__rte_internal
> +struct rte_eth_dev*
> +rte_get_eth_dev_by_name(const char *name);
As the API name, better to start with 'rte_eth_' prefix to be consistent with
rest of the APIs.
I suggest 'rte_eth_dev_get_by_name' but feel free to chose better one.
> +
> /**
> * @internal
> * Reset the current queue state and configuration to disconnect (unbind) it
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index a1d475a292..9192b0d664 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -894,6 +894,17 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
> return -ENODEV;
> }
>
> +struct rte_eth_dev *
> +rte_get_eth_dev_by_name(const char *name)
> +{
> + uint16_t pid;
> +
> + if (rte_eth_dev_get_port_by_name(name, &pid))
> + return NULL;
> +
> + return &rte_eth_devices[pid];
> +}
> +
> static int
> eth_err(uint16_t port_id, int ret)
> {
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index c2fb0669a4..7e3797189b 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -256,6 +256,7 @@ EXPERIMENTAL {
> rte_flow_flex_item_create;
> rte_flow_flex_item_release;
> rte_flow_pick_transfer_proxy;
> +
This is unintendent change.
> };
>
> INTERNAL {
> @@ -282,4 +283,5 @@ INTERNAL {
> rte_eth_representor_id_get;
> rte_eth_switch_domain_alloc;
> rte_eth_switch_domain_free;
> + rte_get_eth_dev_by_name;
Please add in a sorted way.
> };
[-- Attachment #2: Type: text/html, Size: 28808 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2] net/tap: fix to populate fds in secondary process
2022-01-24 12:35 ` Ferruh Yigit
2022-01-24 12:48 ` Kumara Parameshwaran
@ 2022-01-24 12:54 ` kumaraparameshwaran rathinavel
2022-01-24 13:05 ` Ferruh Yigit
2022-01-28 9:55 ` kumaraparameshwaran rathinavel
2 siblings, 1 reply; 44+ messages in thread
From: kumaraparameshwaran rathinavel @ 2022-01-24 12:54 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: dev, Kumara Parameshwaran, stable
[-- Attachment #1: Type: text/plain, Size: 15533 bytes --]
On Mon, Jan 24, 2022 at 6:05 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> On 1/24/2022 12:12 PM, Kumara Parameshwaran wrote:
> > From: Kumara Parameshwaran <kparameshwar@vmware.com>
> >
> > When a tap device is hotplugged to primary process which in turn
> > adds the device to all secondary process, the secondary process
> > does a tap_mp_attach_queues, but the fds are not populated in
> > the primary during the probe they are populated during the queue_setup,
> > added a fix to sync the queues during rte_eth_dev_start
> >
> > Fixes: 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
> > ---
> >
> > v2:
> > * Addressed review comments to move the function declaration and version
> > map
> >
>
> Thanks for adding patch version.
>
> > drivers/net/tap/rte_eth_tap.c | 196 +++++++++++++---------------------
> > lib/ethdev/ethdev_driver.h | 17 +++
> > lib/ethdev/rte_ethdev.c | 11 ++
> > lib/ethdev/version.map | 2 +
> > 4 files changed, 102 insertions(+), 124 deletions(-)
> >
>
> Can you please separate etdev (API) changes to another patch, so this will
> be a patchset
> with two patches,
> first patch adds ethdev API
> second patch is tap patch, that uses the API in the first patch
Sure, I will do it.
>
> > diff --git a/drivers/net/tap/rte_eth_tap.c
> b/drivers/net/tap/rte_eth_tap.c
> > index f1b48cae82..f6c25d7e21 100644
> > --- a/drivers/net/tap/rte_eth_tap.c
> > +++ b/drivers/net/tap/rte_eth_tap.c
> > @@ -66,7 +66,7 @@
> > (TAP_GSO_MBUFS_PER_CORE * TAP_GSO_MBUF_CACHE_SIZE)
> >
> > /* IPC key for queue fds sync */
> > -#define TAP_MP_KEY "tap_mp_sync_queues"
> > +#define TAP_MP_REQ_START_RXTX "tap_mp_req_start_rxtx"
> >
>
> We said we can drop "tap_mp_sync_queues", but thinking twice,
> will current implementation cover following usecase:
>
> - Primary applicaiton started with tap interface, all config, setup,
> start done
> - Secondary app started without any parameter
>
> Since primary already started, I think secondary fds will be wrong,
> what do you think?
> That is true, the fds would be unitialised, we should retain for the
> secondary attach case.
> > #define TAP_IOV_DEFAULT_MAX 1024
> >
> > @@ -880,11 +880,48 @@ tap_link_set_up(struct rte_eth_dev *dev)
> > return tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
> > }
> >
> > +static int tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
>
> Can you please follow the coding convention:
> sure, will do it.
> static int
> tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
> {
>
> > +{
> > + struct rte_mp_msg msg;
> > + struct ipc_queues *request_param = (struct ipc_queues *)msg.param;
> > + int err;
> > + int fd_iterator = 0;
> > + struct pmd_process_private *process_private = dev->process_private;
> > + int i;
> > +
> > + memset(&msg, 0, sizeof(msg));
> > + strlcpy(msg.name, TAP_MP_REQ_START_RXTX, sizeof(msg.name));
> > + strlcpy(request_param->port_name, dev->data->name,
> sizeof(request_param->port_name));
> > + msg.len_param = sizeof(*request_param);
> > + for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > + msg.fds[fd_iterator++] = process_private->txq_fds[i];
> > + msg.num_fds++;
> > + request_param->txq_count++;
> > + }
> > + for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > + msg.fds[fd_iterator++] = process_private->rxq_fds[i];
> > + msg.num_fds++;
> > + request_param->rxq_count++;
> > + }
> > +
> > + err = rte_mp_sendmsg(&msg);
> > + if (err < 0) {
> > + TAP_LOG(ERR, "Failed to send start req to secondary %d",
> > + rte_errno);
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int
> > tap_dev_start(struct rte_eth_dev *dev)
> > {
> > int err, i;
> >
> > + if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> > + tap_mp_req_on_rxtx(dev);
> > +
> > err = tap_intr_handle_set(dev, 1);
> > if (err)
> > return err;
> > @@ -901,6 +938,34 @@ tap_dev_start(struct rte_eth_dev *dev)
> > return err;
> > }
> >
> > +static int
> > +tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused
> const void *peer)
> > +{
>
> I asked last time but I don't remember the response,
> what should be in the 'peer' variable?
>
> > + struct rte_eth_dev *dev;
> > + const struct ipc_queues *request_param =
> > + (const struct ipc_queues *)request->param;
> > + int fd_iterator;
> > + int queue;
> > + struct pmd_process_private *process_private;
> > +
> > + dev = rte_get_eth_dev_by_name(request_param->port_name);
> > + if (!dev) {
> > + TAP_LOG(ERR, "Failed to get dev for %s",
> > + request_param->port_name);
> > + return -1;
> > + }
> > + process_private = dev->process_private;
> > + fd_iterator = 0;
> > + TAP_LOG(DEBUG, "tap_attach rx_q:%d tx_q:%d\n",
> request_param->rxq_count,
> > + request_param->txq_count);
> > + for (queue = 0; queue < request_param->txq_count; queue++)
> > + process_private->txq_fds[queue] =
> request->fds[fd_iterator++];
> > + for (queue = 0; queue < request_param->rxq_count; queue++)
> > + process_private->rxq_fds[queue] =
> request->fds[fd_iterator++];
> > +
> > + return 0;
> > +}
> > +
> > /* This function gets called when the current port gets stopped.
> > */
> > static int
> > @@ -1084,6 +1149,7 @@ tap_dev_close(struct rte_eth_dev *dev)
> >
> > if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> > rte_free(dev->process_private);
> > + rte_mp_action_unregister(TAP_MP_REQ_START_RXTX);
> > return 0;
> > }
> >
> > @@ -1140,8 +1206,6 @@ tap_dev_close(struct rte_eth_dev *dev)
> > internals->ioctl_sock = -1;
> > }
> > rte_free(dev->process_private);
> > - if (tap_devices_count == 1)
> > - rte_mp_action_unregister(TAP_MP_KEY);
> > tap_devices_count--;
> > /*
> > * Since TUN device has no more opened file descriptors
> > @@ -2292,113 +2356,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
> > return ret;
> > }
> >
> > -/* Request queue file descriptors from secondary to primary. */
> > -static int
> > -tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
> > -{
> > - int ret;
> > - struct timespec timeout = {.tv_sec = 1, .tv_nsec = 0};
> > - struct rte_mp_msg request, *reply;
> > - struct rte_mp_reply replies;
> > - struct ipc_queues *request_param = (struct ipc_queues
> *)request.param;
> > - struct ipc_queues *reply_param;
> > - struct pmd_process_private *process_private = dev->process_private;
> > - int queue, fd_iterator;
> > -
> > - /* Prepare the request */
> > - memset(&request, 0, sizeof(request));
> > - strlcpy(request.name, TAP_MP_KEY, sizeof(request.name));
> > - strlcpy(request_param->port_name, port_name,
> > - sizeof(request_param->port_name));
> > - request.len_param = sizeof(*request_param);
> > - /* Send request and receive reply */
> > - ret = rte_mp_request_sync(&request, &replies, &timeout);
> > - if (ret < 0 || replies.nb_received != 1) {
> > - TAP_LOG(ERR, "Failed to request queues from primary: %d",
> > - rte_errno);
> > - return -1;
> > - }
> > - reply = &replies.msgs[0];
> > - reply_param = (struct ipc_queues *)reply->param;
> > - TAP_LOG(DEBUG, "Received IPC reply for %s",
> reply_param->port_name);
> > -
> > - /* Attach the queues from received file descriptors */
> > - if (reply_param->rxq_count + reply_param->txq_count !=
> reply->num_fds) {
> > - TAP_LOG(ERR, "Unexpected number of fds received");
> > - return -1;
> > - }
> > -
> > - dev->data->nb_rx_queues = reply_param->rxq_count;
> > - dev->data->nb_tx_queues = reply_param->txq_count;
> > - fd_iterator = 0;
> > - for (queue = 0; queue < reply_param->rxq_count; queue++)
> > - process_private->rxq_fds[queue] =
> reply->fds[fd_iterator++];
> > - for (queue = 0; queue < reply_param->txq_count; queue++)
> > - process_private->txq_fds[queue] =
> reply->fds[fd_iterator++];
> > - free(reply);
> > - return 0;
> > -}
> > -
> > -/* Send the queue file descriptors from the primary process to
> secondary. */
> > -static int
> > -tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
> > -{
> > - struct rte_eth_dev *dev;
> > - struct pmd_process_private *process_private;
> > - struct rte_mp_msg reply;
> > - const struct ipc_queues *request_param =
> > - (const struct ipc_queues *)request->param;
> > - struct ipc_queues *reply_param =
> > - (struct ipc_queues *)reply.param;
> > - uint16_t port_id;
> > - int queue;
> > - int ret;
> > -
> > - /* Get requested port */
> > - TAP_LOG(DEBUG, "Received IPC request for %s",
> request_param->port_name);
> > - ret = rte_eth_dev_get_port_by_name(request_param->port_name,
> &port_id);
> > - if (ret) {
> > - TAP_LOG(ERR, "Failed to get port id for %s",
> > - request_param->port_name);
> > - return -1;
> > - }
> > - dev = &rte_eth_devices[port_id];
> > - process_private = dev->process_private;
> > -
> > - /* Fill file descriptors for all queues */
> > - reply.num_fds = 0;
> > - reply_param->rxq_count = 0;
> > - if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
> > - RTE_MP_MAX_FD_NUM){
> > - TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of
> fds");
> > - return -1;
> > - }
> > -
> > - for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
> > - reply.fds[reply.num_fds++] =
> process_private->rxq_fds[queue];
> > - reply_param->rxq_count++;
> > - }
> > - RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
> > -
> > - reply_param->txq_count = 0;
> > - for (queue = 0; queue < dev->data->nb_tx_queues; queue++) {
> > - reply.fds[reply.num_fds++] =
> process_private->txq_fds[queue];
> > - reply_param->txq_count++;
> > - }
> > - RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
> > -
> > - /* Send reply */
> > - strlcpy(reply.name, request->name, sizeof(reply.name));
> > - strlcpy(reply_param->port_name, request_param->port_name,
> > - sizeof(reply_param->port_name));
> > - reply.len_param = sizeof(*reply_param);
> > - if (rte_mp_reply(&reply, peer) < 0) {
> > - TAP_LOG(ERR, "Failed to reply an IPC request to sync
> queues");
> > - return -1;
> > - }
> > - return 0;
> > -}
> > -
> > /* Open a TAP interface device.
> > */
> > static int
> > @@ -2442,9 +2399,11 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> > return -1;
> > }
> >
> > - ret = tap_mp_attach_queues(name, eth_dev);
> > - if (ret != 0)
> > - return -1;
> > + ret = rte_mp_action_register(TAP_MP_REQ_START_RXTX,
> tap_mp_req_start_rxtx);
> > + if (ret < 0 && rte_errno != ENOTSUP)
> > + TAP_LOG(ERR, "tap: Failed to register IPC
> callback: %s",
> > + strerror(rte_errno));
> > +
> > rte_eth_dev_probing_finish(eth_dev);
> > return 0;
> > }
> > @@ -2492,15 +2451,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> >
> > TAP_LOG(DEBUG, "Initializing pmd_tap for %s", name);
> >
> > - /* Register IPC feed callback */
> > - if (!tap_devices_count) {
> > - ret = rte_mp_action_register(TAP_MP_KEY,
> tap_mp_sync_queues);
> > - if (ret < 0 && rte_errno != ENOTSUP) {
> > - TAP_LOG(ERR, "tap: Failed to register IPC
> callback: %s",
> > - strerror(rte_errno));
> > - goto leave;
> > - }
> > - }
> > tap_devices_count++;
> > tap_devices_count_increased = 1;
> > ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,
> > @@ -2511,8 +2461,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> > TAP_LOG(ERR, "Failed to create pmd for %s as %s",
> > name, tap_name);
> > if (tap_devices_count_increased == 1) {
> > - if (tap_devices_count == 1)
> > - rte_mp_action_unregister(TAP_MP_KEY);
> > tap_devices_count--;
> > }
> > }
> > diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> > index d95605a355..a08991bcdf 100644
> > --- a/lib/ethdev/ethdev_driver.h
> > +++ b/lib/ethdev/ethdev_driver.h
> > @@ -1629,6 +1629,23 @@ rte_eth_hairpin_queue_peer_bind(uint16_t
> cur_port, uint16_t cur_queue,
> > struct rte_hairpin_peer_info *peer_info,
> > uint32_t direction);
> >
> > +/**
>
> Please add '@internal' tag into doxygen comment.
>
> > +* Get rte_eth_dev from device name. The device name should be specified
> > +* as below:
> > +* - PCIe address (Domain:Bus:Device.Function), for example- 0000:2:00.0
> > +* - SoC device name, for example- fsl-gmac0
> > +* - vdev dpdk name, for example- net_[pcap0|null0|tap0]
> > +*
> > +* @param name
> > +* pci address or name of the device
> > +* @return
> > +* - rte_eth_dev if successful
> > +* - NULL on failure
> > +*/
> > +__rte_internal
> > +struct rte_eth_dev*
> > +rte_get_eth_dev_by_name(const char *name);
>
> As the API name, better to start with 'rte_eth_' prefix to be consistent
> with
> rest of the APIs.
> I suggest 'rte_eth_dev_get_by_name' but feel free to chose better one.
>
> > +
> > /**
> > * @internal
> > * Reset the current queue state and configuration to disconnect
> (unbind) it
> > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> > index a1d475a292..9192b0d664 100644
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -894,6 +894,17 @@ rte_eth_dev_get_port_by_name(const char *name,
> uint16_t *port_id)
> > return -ENODEV;
> > }
> >
> > +struct rte_eth_dev *
> > +rte_get_eth_dev_by_name(const char *name)
> > +{
> > + uint16_t pid;
> > +
> > + if (rte_eth_dev_get_port_by_name(name, &pid))
> > + return NULL;
> > +
> > + return &rte_eth_devices[pid];
> > +}
> > +
> > static int
> > eth_err(uint16_t port_id, int ret)
> > {
> > diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> > index c2fb0669a4..7e3797189b 100644
> > --- a/lib/ethdev/version.map
> > +++ b/lib/ethdev/version.map
> > @@ -256,6 +256,7 @@ EXPERIMENTAL {
> > rte_flow_flex_item_create;
> > rte_flow_flex_item_release;
> > rte_flow_pick_transfer_proxy;
> > +
>
> This is unintendent change.
>
> > };
> >
> > INTERNAL {
> > @@ -282,4 +283,5 @@ INTERNAL {
> > rte_eth_representor_id_get;
> > rte_eth_switch_domain_alloc;
> > rte_eth_switch_domain_free;
> > + rte_get_eth_dev_by_name;
>
> Please add in a sorted way.
>
> > };
>
>
[-- Attachment #2: Type: text/html, Size: 20593 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2] net/tap: fix to populate fds in secondary process
2022-01-24 12:54 ` kumaraparameshwaran rathinavel
@ 2022-01-24 13:05 ` Ferruh Yigit
0 siblings, 0 replies; 44+ messages in thread
From: Ferruh Yigit @ 2022-01-24 13:05 UTC (permalink / raw)
To: kumaraparameshwaran rathinavel; +Cc: dev, Kumara Parameshwaran, stable
On 1/24/2022 12:54 PM, kumaraparameshwaran rathinavel wrote:
>
>
> On Mon, Jan 24, 2022 at 6:05 PM Ferruh Yigit <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>> wrote:
>
> On 1/24/2022 12:12 PM, Kumara Parameshwaran wrote:
> > From: Kumara Parameshwaran <kparameshwar@vmware.com <mailto:kparameshwar@vmware.com>>
> >
> > When a tap device is hotplugged to primary process which in turn
> > adds the device to all secondary process, the secondary process
> > does a tap_mp_attach_queues, but the fds are not populated in
> > the primary during the probe they are populated during the queue_setup,
> > added a fix to sync the queues during rte_eth_dev_start
> >
> > Fixes: 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")
> > Cc: stable@dpdk.org <mailto:stable@dpdk.org>
> >
> > Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com <mailto:kparameshwar@vmware.com>>
> > ---
> >
> > v2:
> > * Addressed review comments to move the function declaration and version
> > map
> >
>
> Thanks for adding patch version.
>
> > drivers/net/tap/rte_eth_tap.c | 196 +++++++++++++---------------------
> > lib/ethdev/ethdev_driver.h | 17 +++
> > lib/ethdev/rte_ethdev.c | 11 ++
> > lib/ethdev/version.map | 2 +
> > 4 files changed, 102 insertions(+), 124 deletions(-)
> >
>
> Can you please separate etdev (API) changes to another patch, so this will be a patchset
> with two patches,
> first patch adds ethdev API
> second patch is tap patch, that uses the API in the first patch
>
> Sure, I will do it.
>
>
> > diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> > index f1b48cae82..f6c25d7e21 100644
> > --- a/drivers/net/tap/rte_eth_tap.c
> > +++ b/drivers/net/tap/rte_eth_tap.c
> > @@ -66,7 +66,7 @@
> > (TAP_GSO_MBUFS_PER_CORE * TAP_GSO_MBUF_CACHE_SIZE)
> >
> > /* IPC key for queue fds sync */
> > -#define TAP_MP_KEY "tap_mp_sync_queues"
> > +#define TAP_MP_REQ_START_RXTX "tap_mp_req_start_rxtx"
> >
>
> We said we can drop "tap_mp_sync_queues", but thinking twice,
> will current implementation cover following usecase:
>
> - Primary applicaiton started with tap interface, all config, setup,
> start done
> - Secondary app started without any parameter
>
> Since primary already started, I think secondary fds will be wrong,
> what do you think?
> That is true, the fds would be unitialised, we should retain for the secondary attach case.
> > #define TAP_IOV_DEFAULT_MAX 1024
> >
> > @@ -880,11 +880,48 @@ tap_link_set_up(struct rte_eth_dev *dev)
> > return tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
> > }
> >
> > +static int tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
>
> Can you please follow the coding convention:
> sure, will do it.
There are more comments/questions below, ca you please check them too?
> static int
> tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
> {
>
> > +{
> > + struct rte_mp_msg msg;
> > + struct ipc_queues *request_param = (struct ipc_queues *)msg.param;
> > + int err;
> > + int fd_iterator = 0;
> > + struct pmd_process_private *process_private = dev->process_private;
> > + int i;
> > +
> > + memset(&msg, 0, sizeof(msg));
> > + strlcpy(msg.name <http://msg.name>, TAP_MP_REQ_START_RXTX, sizeof(msg.name <http://msg.name>));
> > + strlcpy(request_param->port_name, dev->data->name, sizeof(request_param->port_name));
> > + msg.len_param = sizeof(*request_param);
> > + for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > + msg.fds[fd_iterator++] = process_private->txq_fds[i];
> > + msg.num_fds++;
> > + request_param->txq_count++;
> > + }
> > + for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > + msg.fds[fd_iterator++] = process_private->rxq_fds[i];
> > + msg.num_fds++;
> > + request_param->rxq_count++;
> > + }
> > +
> > + err = rte_mp_sendmsg(&msg);
> > + if (err < 0) {
> > + TAP_LOG(ERR, "Failed to send start req to secondary %d",
> > + rte_errno);
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int
> > tap_dev_start(struct rte_eth_dev *dev)
> > {
> > int err, i;
> >
> > + if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> > + tap_mp_req_on_rxtx(dev);
> > +
> > err = tap_intr_handle_set(dev, 1);
> > if (err)
> > return err;
> > @@ -901,6 +938,34 @@ tap_dev_start(struct rte_eth_dev *dev)
> > return err;
> > }
> >
> > +static int
> > +tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused const void *peer)
> > +{
>
> I asked last time but I don't remember the response,
> what should be in the 'peer' variable?
>
> > + struct rte_eth_dev *dev;
> > + const struct ipc_queues *request_param =
> > + (const struct ipc_queues *)request->param;
> > + int fd_iterator;
> > + int queue;
> > + struct pmd_process_private *process_private;
> > +
> > + dev = rte_get_eth_dev_by_name(request_param->port_name);
> > + if (!dev) {
> > + TAP_LOG(ERR, "Failed to get dev for %s",
> > + request_param->port_name);
> > + return -1;
> > + }
> > + process_private = dev->process_private;
> > + fd_iterator = 0;
> > + TAP_LOG(DEBUG, "tap_attach rx_q:%d tx_q:%d\n", request_param->rxq_count,
> > + request_param->txq_count);
> > + for (queue = 0; queue < request_param->txq_count; queue++)
> > + process_private->txq_fds[queue] = request->fds[fd_iterator++];
> > + for (queue = 0; queue < request_param->rxq_count; queue++)
> > + process_private->rxq_fds[queue] = request->fds[fd_iterator++];
> > +
> > + return 0;
> > +}
> > +
> > /* This function gets called when the current port gets stopped.
> > */
> > static int
> > @@ -1084,6 +1149,7 @@ tap_dev_close(struct rte_eth_dev *dev)
> >
> > if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> > rte_free(dev->process_private);
> > + rte_mp_action_unregister(TAP_MP_REQ_START_RXTX);
> > return 0;
> > }
> >
> > @@ -1140,8 +1206,6 @@ tap_dev_close(struct rte_eth_dev *dev)
> > internals->ioctl_sock = -1;
> > }
> > rte_free(dev->process_private);
> > - if (tap_devices_count == 1)
> > - rte_mp_action_unregister(TAP_MP_KEY);
> > tap_devices_count--;
> > /*
> > * Since TUN device has no more opened file descriptors
> > @@ -2292,113 +2356,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
> > return ret;
> > }
> >
> > -/* Request queue file descriptors from secondary to primary. */
> > -static int
> > -tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
> > -{
> > - int ret;
> > - struct timespec timeout = {.tv_sec = 1, .tv_nsec = 0};
> > - struct rte_mp_msg request, *reply;
> > - struct rte_mp_reply replies;
> > - struct ipc_queues *request_param = (struct ipc_queues *)request.param;
> > - struct ipc_queues *reply_param;
> > - struct pmd_process_private *process_private = dev->process_private;
> > - int queue, fd_iterator;
> > -
> > - /* Prepare the request */
> > - memset(&request, 0, sizeof(request));
> > - strlcpy(request.name <http://request.name>, TAP_MP_KEY, sizeof(request.name <http://request.name>));
> > - strlcpy(request_param->port_name, port_name,
> > - sizeof(request_param->port_name));
> > - request.len_param = sizeof(*request_param);
> > - /* Send request and receive reply */
> > - ret = rte_mp_request_sync(&request, &replies, &timeout);
> > - if (ret < 0 || replies.nb_received != 1) {
> > - TAP_LOG(ERR, "Failed to request queues from primary: %d",
> > - rte_errno);
> > - return -1;
> > - }
> > - reply = &replies.msgs[0];
> > - reply_param = (struct ipc_queues *)reply->param;
> > - TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
> > -
> > - /* Attach the queues from received file descriptors */
> > - if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds) {
> > - TAP_LOG(ERR, "Unexpected number of fds received");
> > - return -1;
> > - }
> > -
> > - dev->data->nb_rx_queues = reply_param->rxq_count;
> > - dev->data->nb_tx_queues = reply_param->txq_count;
> > - fd_iterator = 0;
> > - for (queue = 0; queue < reply_param->rxq_count; queue++)
> > - process_private->rxq_fds[queue] = reply->fds[fd_iterator++];
> > - for (queue = 0; queue < reply_param->txq_count; queue++)
> > - process_private->txq_fds[queue] = reply->fds[fd_iterator++];
> > - free(reply);
> > - return 0;
> > -}
> > -
> > -/* Send the queue file descriptors from the primary process to secondary. */
> > -static int
> > -tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
> > -{
> > - struct rte_eth_dev *dev;
> > - struct pmd_process_private *process_private;
> > - struct rte_mp_msg reply;
> > - const struct ipc_queues *request_param =
> > - (const struct ipc_queues *)request->param;
> > - struct ipc_queues *reply_param =
> > - (struct ipc_queues *)reply.param;
> > - uint16_t port_id;
> > - int queue;
> > - int ret;
> > -
> > - /* Get requested port */
> > - TAP_LOG(DEBUG, "Received IPC request for %s", request_param->port_name);
> > - ret = rte_eth_dev_get_port_by_name(request_param->port_name, &port_id);
> > - if (ret) {
> > - TAP_LOG(ERR, "Failed to get port id for %s",
> > - request_param->port_name);
> > - return -1;
> > - }
> > - dev = &rte_eth_devices[port_id];
> > - process_private = dev->process_private;
> > -
> > - /* Fill file descriptors for all queues */
> > - reply.num_fds = 0;
> > - reply_param->rxq_count = 0;
> > - if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
> > - RTE_MP_MAX_FD_NUM){
> > - TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of fds");
> > - return -1;
> > - }
> > -
> > - for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
> > - reply.fds[reply.num_fds++] = process_private->rxq_fds[queue];
> > - reply_param->rxq_count++;
> > - }
> > - RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
> > -
> > - reply_param->txq_count = 0;
> > - for (queue = 0; queue < dev->data->nb_tx_queues; queue++) {
> > - reply.fds[reply.num_fds++] = process_private->txq_fds[queue];
> > - reply_param->txq_count++;
> > - }
> > - RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
> > -
> > - /* Send reply */
> > - strlcpy(reply.name <http://reply.name>, request->name, sizeof(reply.name <http://reply.name>));
> > - strlcpy(reply_param->port_name, request_param->port_name,
> > - sizeof(reply_param->port_name));
> > - reply.len_param = sizeof(*reply_param);
> > - if (rte_mp_reply(&reply, peer) < 0) {
> > - TAP_LOG(ERR, "Failed to reply an IPC request to sync queues");
> > - return -1;
> > - }
> > - return 0;
> > -}
> > -
> > /* Open a TAP interface device.
> > */
> > static int
> > @@ -2442,9 +2399,11 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> > return -1;
> > }
> >
> > - ret = tap_mp_attach_queues(name, eth_dev);
> > - if (ret != 0)
> > - return -1;
> > + ret = rte_mp_action_register(TAP_MP_REQ_START_RXTX, tap_mp_req_start_rxtx);
> > + if (ret < 0 && rte_errno != ENOTSUP)
> > + TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
> > + strerror(rte_errno));
> > +
> > rte_eth_dev_probing_finish(eth_dev);
> > return 0;
> > }
> > @@ -2492,15 +2451,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> >
> > TAP_LOG(DEBUG, "Initializing pmd_tap for %s", name);
> >
> > - /* Register IPC feed callback */
> > - if (!tap_devices_count) {
> > - ret = rte_mp_action_register(TAP_MP_KEY, tap_mp_sync_queues);
> > - if (ret < 0 && rte_errno != ENOTSUP) {
> > - TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
> > - strerror(rte_errno));
> > - goto leave;
> > - }
> > - }
> > tap_devices_count++;
> > tap_devices_count_increased = 1;
> > ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,
> > @@ -2511,8 +2461,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> > TAP_LOG(ERR, "Failed to create pmd for %s as %s",
> > name, tap_name);
> > if (tap_devices_count_increased == 1) {
> > - if (tap_devices_count == 1)
> > - rte_mp_action_unregister(TAP_MP_KEY);
> > tap_devices_count--;
> > }
> > }
> > diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> > index d95605a355..a08991bcdf 100644
> > --- a/lib/ethdev/ethdev_driver.h
> > +++ b/lib/ethdev/ethdev_driver.h
> > @@ -1629,6 +1629,23 @@ rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue,
> > struct rte_hairpin_peer_info *peer_info,
> > uint32_t direction);
> >
> > +/**
>
> Please add '@internal' tag into doxygen comment.
>
> > +* Get rte_eth_dev from device name. The device name should be specified
> > +* as below:
> > +* - PCIe address (Domain:Bus:Device.Function), for example- 0000:2:00.0
> > +* - SoC device name, for example- fsl-gmac0
> > +* - vdev dpdk name, for example- net_[pcap0|null0|tap0]
> > +*
> > +* @param name
> > +* pci address or name of the device
> > +* @return
> > +* - rte_eth_dev if successful
> > +* - NULL on failure
> > +*/
> > +__rte_internal
> > +struct rte_eth_dev*
> > +rte_get_eth_dev_by_name(const char *name);
>
> As the API name, better to start with 'rte_eth_' prefix to be consistent with
> rest of the APIs.
> I suggest 'rte_eth_dev_get_by_name' but feel free to chose better one.
>
> > +
> > /**
> > * @internal
> > * Reset the current queue state and configuration to disconnect (unbind) it
> > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> > index a1d475a292..9192b0d664 100644
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -894,6 +894,17 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
> > return -ENODEV;
> > }
> >
> > +struct rte_eth_dev *
> > +rte_get_eth_dev_by_name(const char *name)
> > +{
> > + uint16_t pid;
> > +
> > + if (rte_eth_dev_get_port_by_name(name, &pid))
> > + return NULL;
> > +
> > + return &rte_eth_devices[pid];
> > +}
> > +
> > static int
> > eth_err(uint16_t port_id, int ret)
> > {
> > diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> > index c2fb0669a4..7e3797189b 100644
> > --- a/lib/ethdev/version.map
> > +++ b/lib/ethdev/version.map
> > @@ -256,6 +256,7 @@ EXPERIMENTAL {
> > rte_flow_flex_item_create;
> > rte_flow_flex_item_release;
> > rte_flow_pick_transfer_proxy;
> > +
>
> This is unintendent change.
>
> > };
> >
> > INTERNAL {
> > @@ -282,4 +283,5 @@ INTERNAL {
> > rte_eth_representor_id_get;
> > rte_eth_switch_domain_alloc;
> > rte_eth_switch_domain_free;
> > + rte_get_eth_dev_by_name;
>
> Please add in a sorted way.
>
> > };
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2] net/tap: fix to populate fds in secondary process
2022-01-24 12:37 ` Kumara Parameshwaran
@ 2022-01-24 13:06 ` Ferruh Yigit
0 siblings, 0 replies; 44+ messages in thread
From: Ferruh Yigit @ 2022-01-24 13:06 UTC (permalink / raw)
To: Kumara Parameshwaran, dev; +Cc: Kumara Parameshwaran, stable
On 1/24/2022 12:37 PM, Kumara Parameshwaran wrote:
> From: Kumara Parameshwaran<kparameshwar@vmware.com>
>
> When a tap device is hotplugged to primary process which in turn
> adds the device to all secondary process, the secondary process
> does a tap_mp_attach_queues, but the fds are not populated in
> the primary during the probe they are populated during the queue_setup,
> added a fix to sync the queues during rte_eth_dev_start
>
> Fixes: 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")
> Cc:stable@dpdk.org
>
> Signed-off-by: Kumara Parameshwaran<kparameshwar@vmware.com>
Should this be v3?
Can you please accumulate change longs, and increase versions as it progress?
And there were more comments in v2, like splitting the patch, can you please
check them all?
Thanks,
ferruh
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2] net/tap: fix to populate fds in secondary process
2022-01-24 12:35 ` Ferruh Yigit
2022-01-24 12:48 ` Kumara Parameshwaran
2022-01-24 12:54 ` kumaraparameshwaran rathinavel
@ 2022-01-28 9:55 ` kumaraparameshwaran rathinavel
2022-01-28 12:55 ` Ferruh Yigit
2 siblings, 1 reply; 44+ messages in thread
From: kumaraparameshwaran rathinavel @ 2022-01-28 9:55 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: dev, Kumara Parameshwaran, stable
[-- Attachment #1: Type: text/plain, Size: 15856 bytes --]
Hi Ferruh,
Please find the consolidated comments for the review. I had missed
answering a few of them in my previous email.
On Mon, Jan 24, 2022 at 6:05 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> On 1/24/2022 12:12 PM, Kumara Parameshwaran wrote:
> > From: Kumara Parameshwaran <kparameshwar@vmware.com>
> >
> > When a tap device is hotplugged to primary process which in turn
> > adds the device to all secondary process, the secondary process
> > does a tap_mp_attach_queues, but the fds are not populated in
> > the primary during the probe they are populated during the queue_setup,
> > added a fix to sync the queues during rte_eth_dev_start
> >
> > Fixes: 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
> > ---
> >
> > v2:
> > * Addressed review comments to move the function declaration and version
> > map
> >
>
> Thanks for adding patch version.
>
> > drivers/net/tap/rte_eth_tap.c | 196 +++++++++++++---------------------
> > lib/ethdev/ethdev_driver.h | 17 +++
> > lib/ethdev/rte_ethdev.c | 11 ++
> > lib/ethdev/version.map | 2 +
> > 4 files changed, 102 insertions(+), 124 deletions(-)
> >
>
> Can you please separate etdev (API) changes to another patch, so this will
> be a patchset
> with two patches,
> first patch adds ethdev API
> second patch is tap patch, that uses the API in the first patch
> Sure, will do it.
>
> > diff --git a/drivers/net/tap/rte_eth_tap.c
> b/drivers/net/tap/rte_eth_tap.c
> > index f1b48cae82..f6c25d7e21 100644
> > --- a/drivers/net/tap/rte_eth_tap.c
> > +++ b/drivers/net/tap/rte_eth_tap.c
> > @@ -66,7 +66,7 @@
> > (TAP_GSO_MBUFS_PER_CORE * TAP_GSO_MBUF_CACHE_SIZE)
> >
> > /* IPC key for queue fds sync */
> > -#define TAP_MP_KEY "tap_mp_sync_queues"
> > +#define TAP_MP_REQ_START_RXTX "tap_mp_req_start_rxtx"
> >
>
> We said we can drop "tap_mp_sync_queues", but thinking twice,
> will current implementation cover following usecase:
>
> - Primary applicaiton started with tap interface, all config, setup,
> start done
> - Secondary app started without any parameter
>
> Since primary already started, I think secondary fds will be wrong,
> what do you think?
> Yes, will retain the old behaviour.
> > #define TAP_IOV_DEFAULT_MAX 1024
> >
> > @@ -880,11 +880,48 @@ tap_link_set_up(struct rte_eth_dev *dev)
> > return tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
> > }
> >
> > +static int tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
>
> Can you please follow the coding convention:
> Sure, will follow it.
> static int
> tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
> {
>
> > +{
> > + struct rte_mp_msg msg;
> > + struct ipc_queues *request_param = (struct ipc_queues *)msg.param;
> > + int err;
> > + int fd_iterator = 0;
> > + struct pmd_process_private *process_private = dev->process_private;
> > + int i;
> > +
> > + memset(&msg, 0, sizeof(msg));
> > + strlcpy(msg.name, TAP_MP_REQ_START_RXTX, sizeof(msg.name));
> > + strlcpy(request_param->port_name, dev->data->name,
> sizeof(request_param->port_name));
> > + msg.len_param = sizeof(*request_param);
> > + for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > + msg.fds[fd_iterator++] = process_private->txq_fds[i];
> > + msg.num_fds++;
> > + request_param->txq_count++;
> > + }
> > + for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > + msg.fds[fd_iterator++] = process_private->rxq_fds[i];
> > + msg.num_fds++;
> > + request_param->rxq_count++;
> > + }
> > +
> > + err = rte_mp_sendmsg(&msg);
> > + if (err < 0) {
> > + TAP_LOG(ERR, "Failed to send start req to secondary %d",
> > + rte_errno);
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int
> > tap_dev_start(struct rte_eth_dev *dev)
> > {
> > int err, i;
> >
> > + if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> > + tap_mp_req_on_rxtx(dev);
> > +
> > err = tap_intr_handle_set(dev, 1);
> > if (err)
> > return err;
> > @@ -901,6 +938,34 @@ tap_dev_start(struct rte_eth_dev *dev)
> > return err;
> > }
> >
> > +static int
> > +tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused
> const void *peer)
> > +{
>
> I asked last time but I don't remember the response,
> what should be in the 'peer' variable?
> The peer variable is currently unused. The primary does not expect any
> response from secondary processes. Should we change the model to
> rte_mp_request_sync rather than rte_mp_send_msg?
>
> > + struct rte_eth_dev *dev;
> > + const struct ipc_queues *request_param =
> > + (const struct ipc_queues *)request->param;
> > + int fd_iterator;
> > + int queue;
> > + struct pmd_process_private *process_private;
> > +
> > + dev = rte_get_eth_dev_by_name(request_param->port_name);
> > + if (!dev) {
> > + TAP_LOG(ERR, "Failed to get dev for %s",
> > + request_param->port_name);
> > + return -1;
> > + }
> > + process_private = dev->process_private;
> > + fd_iterator = 0;
> > + TAP_LOG(DEBUG, "tap_attach rx_q:%d tx_q:%d\n",
> request_param->rxq_count,
> > + request_param->txq_count);
> > + for (queue = 0; queue < request_param->txq_count; queue++)
> > + process_private->txq_fds[queue] =
> request->fds[fd_iterator++];
> > + for (queue = 0; queue < request_param->rxq_count; queue++)
> > + process_private->rxq_fds[queue] =
> request->fds[fd_iterator++];
> > +
> > + return 0;
> > +}
> > +
> > /* This function gets called when the current port gets stopped.
> > */
> > static int
> > @@ -1084,6 +1149,7 @@ tap_dev_close(struct rte_eth_dev *dev)
> >
> > if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> > rte_free(dev->process_private);
> > + rte_mp_action_unregister(TAP_MP_REQ_START_RXTX);
> > return 0;
> > }
> >
> > @@ -1140,8 +1206,6 @@ tap_dev_close(struct rte_eth_dev *dev)
> > internals->ioctl_sock = -1;
> > }
> > rte_free(dev->process_private);
> > - if (tap_devices_count == 1)
> > - rte_mp_action_unregister(TAP_MP_KEY);
> > tap_devices_count--;
> > /*
> > * Since TUN device has no more opened file descriptors
> > @@ -2292,113 +2356,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
> > return ret;
> > }
> >
> > -/* Request queue file descriptors from secondary to primary. */
> > -static int
> > -tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
> > -{
> > - int ret;
> > - struct timespec timeout = {.tv_sec = 1, .tv_nsec = 0};
> > - struct rte_mp_msg request, *reply;
> > - struct rte_mp_reply replies;
> > - struct ipc_queues *request_param = (struct ipc_queues
> *)request.param;
> > - struct ipc_queues *reply_param;
> > - struct pmd_process_private *process_private = dev->process_private;
> > - int queue, fd_iterator;
> > -
> > - /* Prepare the request */
> > - memset(&request, 0, sizeof(request));
> > - strlcpy(request.name, TAP_MP_KEY, sizeof(request.name));
> > - strlcpy(request_param->port_name, port_name,
> > - sizeof(request_param->port_name));
> > - request.len_param = sizeof(*request_param);
> > - /* Send request and receive reply */
> > - ret = rte_mp_request_sync(&request, &replies, &timeout);
> > - if (ret < 0 || replies.nb_received != 1) {
> > - TAP_LOG(ERR, "Failed to request queues from primary: %d",
> > - rte_errno);
> > - return -1;
> > - }
> > - reply = &replies.msgs[0];
> > - reply_param = (struct ipc_queues *)reply->param;
> > - TAP_LOG(DEBUG, "Received IPC reply for %s",
> reply_param->port_name);
> > -
> > - /* Attach the queues from received file descriptors */
> > - if (reply_param->rxq_count + reply_param->txq_count !=
> reply->num_fds) {
> > - TAP_LOG(ERR, "Unexpected number of fds received");
> > - return -1;
> > - }
> > -
> > - dev->data->nb_rx_queues = reply_param->rxq_count;
> > - dev->data->nb_tx_queues = reply_param->txq_count;
> > - fd_iterator = 0;
> > - for (queue = 0; queue < reply_param->rxq_count; queue++)
> > - process_private->rxq_fds[queue] =
> reply->fds[fd_iterator++];
> > - for (queue = 0; queue < reply_param->txq_count; queue++)
> > - process_private->txq_fds[queue] =
> reply->fds[fd_iterator++];
> > - free(reply);
> > - return 0;
> > -}
> > -
> > -/* Send the queue file descriptors from the primary process to
> secondary. */
> > -static int
> > -tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
> > -{
> > - struct rte_eth_dev *dev;
> > - struct pmd_process_private *process_private;
> > - struct rte_mp_msg reply;
> > - const struct ipc_queues *request_param =
> > - (const struct ipc_queues *)request->param;
> > - struct ipc_queues *reply_param =
> > - (struct ipc_queues *)reply.param;
> > - uint16_t port_id;
> > - int queue;
> > - int ret;
> > -
> > - /* Get requested port */
> > - TAP_LOG(DEBUG, "Received IPC request for %s",
> request_param->port_name);
> > - ret = rte_eth_dev_get_port_by_name(request_param->port_name,
> &port_id);
> > - if (ret) {
> > - TAP_LOG(ERR, "Failed to get port id for %s",
> > - request_param->port_name);
> > - return -1;
> > - }
> > - dev = &rte_eth_devices[port_id];
> > - process_private = dev->process_private;
> > -
> > - /* Fill file descriptors for all queues */
> > - reply.num_fds = 0;
> > - reply_param->rxq_count = 0;
> > - if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
> > - RTE_MP_MAX_FD_NUM){
> > - TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of
> fds");
> > - return -1;
> > - }
> > -
> > - for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
> > - reply.fds[reply.num_fds++] =
> process_private->rxq_fds[queue];
> > - reply_param->rxq_count++;
> > - }
> > - RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
> > -
> > - reply_param->txq_count = 0;
> > - for (queue = 0; queue < dev->data->nb_tx_queues; queue++) {
> > - reply.fds[reply.num_fds++] =
> process_private->txq_fds[queue];
> > - reply_param->txq_count++;
> > - }
> > - RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
> > -
> > - /* Send reply */
> > - strlcpy(reply.name, request->name, sizeof(reply.name));
> > - strlcpy(reply_param->port_name, request_param->port_name,
> > - sizeof(reply_param->port_name));
> > - reply.len_param = sizeof(*reply_param);
> > - if (rte_mp_reply(&reply, peer) < 0) {
> > - TAP_LOG(ERR, "Failed to reply an IPC request to sync
> queues");
> > - return -1;
> > - }
> > - return 0;
> > -}
> > -
> > /* Open a TAP interface device.
> > */
> > static int
> > @@ -2442,9 +2399,11 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> > return -1;
> > }
> >
> > - ret = tap_mp_attach_queues(name, eth_dev);
> > - if (ret != 0)
> > - return -1;
> > + ret = rte_mp_action_register(TAP_MP_REQ_START_RXTX,
> tap_mp_req_start_rxtx);
> > + if (ret < 0 && rte_errno != ENOTSUP)
> > + TAP_LOG(ERR, "tap: Failed to register IPC
> callback: %s",
> > + strerror(rte_errno));
> > +
> > rte_eth_dev_probing_finish(eth_dev);
> > return 0;
> > }
> > @@ -2492,15 +2451,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> >
> > TAP_LOG(DEBUG, "Initializing pmd_tap for %s", name);
> >
> > - /* Register IPC feed callback */
> > - if (!tap_devices_count) {
> > - ret = rte_mp_action_register(TAP_MP_KEY,
> tap_mp_sync_queues);
> > - if (ret < 0 && rte_errno != ENOTSUP) {
> > - TAP_LOG(ERR, "tap: Failed to register IPC
> callback: %s",
> > - strerror(rte_errno));
> > - goto leave;
> > - }
> > - }
> > tap_devices_count++;
> > tap_devices_count_increased = 1;
> > ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,
> > @@ -2511,8 +2461,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> > TAP_LOG(ERR, "Failed to create pmd for %s as %s",
> > name, tap_name);
> > if (tap_devices_count_increased == 1) {
> > - if (tap_devices_count == 1)
> > - rte_mp_action_unregister(TAP_MP_KEY);
> > tap_devices_count--;
> > }
> > }
> > diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> > index d95605a355..a08991bcdf 100644
> > --- a/lib/ethdev/ethdev_driver.h
> > +++ b/lib/ethdev/ethdev_driver.h
> > @@ -1629,6 +1629,23 @@ rte_eth_hairpin_queue_peer_bind(uint16_t
> cur_port, uint16_t cur_queue,
> > struct rte_hairpin_peer_info *peer_info,
> > uint32_t direction);
> >
> > +/**
>
> Please add '@internal' tag into doxygen comment.
> Sure, will do it.
> > +* Get rte_eth_dev from device name. The device name should be specified
> > +* as below:
> > +* - PCIe address (Domain:Bus:Device.Function), for example- 0000:2:00.0
> > +* - SoC device name, for example- fsl-gmac0
> > +* - vdev dpdk name, for example- net_[pcap0|null0|tap0]
> > +*
> > +* @param name
> > +* pci address or name of the device
> > +* @return
> > +* - rte_eth_dev if successful
> > +* - NULL on failure
> > +*/
> > +__rte_internal
> > +struct rte_eth_dev*
> > +rte_get_eth_dev_by_name(const char *name);
>
> As the API name, better to start with 'rte_eth_' prefix to be consistent
> with
> rest of the APIs.
> I suggest 'rte_eth_dev_get_by_name' but feel free to chose better one.
> Sure, will change it.
> > +
> > /**
> > * @internal
> > * Reset the current queue state and configuration to disconnect
> (unbind) it
> > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> > index a1d475a292..9192b0d664 100644
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -894,6 +894,17 @@ rte_eth_dev_get_port_by_name(const char *name,
> uint16_t *port_id)
> > return -ENODEV;
> > }
> >
> > +struct rte_eth_dev *
> > +rte_get_eth_dev_by_name(const char *name)
> > +{
> > + uint16_t pid;
> > +
> > + if (rte_eth_dev_get_port_by_name(name, &pid))
> > + return NULL;
> > +
> > + return &rte_eth_devices[pid];
> > +}
> > +
> > static int
> > eth_err(uint16_t port_id, int ret)
> > {
> > diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> > index c2fb0669a4..7e3797189b 100644
> > --- a/lib/ethdev/version.map
> > +++ b/lib/ethdev/version.map
> > @@ -256,6 +256,7 @@ EXPERIMENTAL {
> > rte_flow_flex_item_create;
> > rte_flow_flex_item_release;
> > rte_flow_pick_transfer_proxy;
> > +
>
> This is unintendent change.
> Will remove it.
> > };
> >
> > INTERNAL {
> > @@ -282,4 +283,5 @@ INTERNAL {
> > rte_eth_representor_id_get;
> > rte_eth_switch_domain_alloc;
> > rte_eth_switch_domain_free;
> > + rte_get_eth_dev_by_name;
>
> Please add in a sorted way.
> Sure.
> > };
>
>
[-- Attachment #2: Type: text/html, Size: 21131 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2] net/tap: fix to populate fds in secondary process
2022-01-28 9:55 ` kumaraparameshwaran rathinavel
@ 2022-01-28 12:55 ` Ferruh Yigit
0 siblings, 0 replies; 44+ messages in thread
From: Ferruh Yigit @ 2022-01-28 12:55 UTC (permalink / raw)
To: kumaraparameshwaran rathinavel; +Cc: dev, Kumara Parameshwaran, stable
On 1/28/2022 9:55 AM, kumaraparameshwaran rathinavel wrote:
> > +static int
> > +tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused const void *peer)
> > +{
>
> I asked last time but I don't remember the response,
> what should be in the 'peer' variable?
> The peer variable is currently unused. The primary does not expect any response from secondary processes. Should we change the model to rte_mp_request_sync rather than rte_mp_send_msg?
No need to change,
I asked because I wasn't clear what 'peer' is for, as checked,
it is the address of the sender which is used to reply, so not
useful in this case.
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 1/2] ethdev: define a function to get eth dev structure
2022-01-21 4:29 [PATCH] net/tap: Bug fix to populate fds in secondary process Kumara Parameshwaran
` (2 preceding siblings ...)
2022-01-24 12:37 ` Kumara Parameshwaran
@ 2022-01-31 14:21 ` Kumara Parameshwaran
2022-01-31 14:21 ` [PATCH v3 2/2] net/tap: fix to populate fds in secondary process Kumara Parameshwaran
2022-01-31 14:32 ` [PATCH v3 1/2] ethdev: define a function to get eth dev structure Kumara Parameshwaran
4 siblings, 1 reply; 44+ messages in thread
From: Kumara Parameshwaran @ 2022-01-31 14:21 UTC (permalink / raw)
To: dev; +Cc: ferruh.yigit, Kumara Parameshwaran
From: Kumara Parameshwaran <kparameshwar@vmware.com>
The PMDs would need a function to access the rte_eth_devices
global array
Cc:stable@dpdk.org
Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
---
lib/ethdev/ethdev_driver.h | 18 ++++++++++++++++++
lib/ethdev/rte_ethdev.c | 11 +++++++++++
lib/ethdev/version.map | 1 +
3 files changed, 30 insertions(+)
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index d95605a355..7d27781f7d 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1629,6 +1629,24 @@ rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue,
struct rte_hairpin_peer_info *peer_info,
uint32_t direction);
+/**
+ * @internal
+ * Get rte_eth_dev from device name. The device name should be specified
+ * as below:
+ * - PCIe address (Domain:Bus:Device.Function), for example- 0000:2:00.0
+ * - SoC device name, for example- fsl-gmac0
+ * - vdev dpdk name, for example- net_[pcap0|null0|tap0]
+ *
+ * @param name
+ * pci address or name of the device
+ * @return
+ * - rte_eth_dev if successful
+ * - NULL on failure
+ */
+__rte_internal
+struct rte_eth_dev*
+rte_eth_dev_get_by_name(const char *name);
+
/**
* @internal
* Reset the current queue state and configuration to disconnect (unbind) it
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index a1d475a292..968475d107 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -894,6 +894,17 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
return -ENODEV;
}
+struct rte_eth_dev *
+rte_eth_dev_get_by_name(const char *name)
+{
+ uint16_t pid;
+
+ if (rte_eth_dev_get_port_by_name(name, &pid))
+ return NULL;
+
+ return &rte_eth_devices[pid];
+}
+
static int
eth_err(uint16_t port_id, int ret)
{
diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
index c2fb0669a4..1f7359c846 100644
--- a/lib/ethdev/version.map
+++ b/lib/ethdev/version.map
@@ -267,6 +267,7 @@ INTERNAL {
rte_eth_dev_callback_process;
rte_eth_dev_create;
rte_eth_dev_destroy;
+ rte_eth_dev_get_by_name;
rte_eth_dev_is_rx_hairpin_queue;
rte_eth_dev_is_tx_hairpin_queue;
rte_eth_dev_probing_finish;
--
2.17.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 2/2] net/tap: fix to populate fds in secondary process
2022-01-31 14:21 ` [PATCH v3 1/2] ethdev: define a function to get eth dev structure Kumara Parameshwaran
@ 2022-01-31 14:21 ` Kumara Parameshwaran
0 siblings, 0 replies; 44+ messages in thread
From: Kumara Parameshwaran @ 2022-01-31 14:21 UTC (permalink / raw)
To: dev; +Cc: ferruh.yigit, Kumara Parameshwaran, stable
From: Kumara Parameshwaran <kparameshwar@vmware.com>
When a tap device is hotplugged to primary process which in turn
adds the device to all secondary process, the secondary process
does a tap_mp_attach_queues, but the fds are not populated in
the primary during the probe they are populated during the queue_setup,
added a fix to sync the queues during rte_eth_dev_start
Fixes: 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")
Cc: stable@dpdk.org
Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
---
v3:
* Retain tap_sync_queues to retain the attach of secondary process
* Fix coding convention for a function definition
* Renamed rte_get_eth_dev_by_name to rte_eth_dev_get_by_name in sorted
in version.map
* Remove uninteded blank line
drivers/net/tap/rte_eth_tap.c | 80 +++++++++++++++++++++++++++++++++++
1 file changed, 80 insertions(+)
diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index f1b48cae82..d13baadbe7 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -67,6 +67,7 @@
/* IPC key for queue fds sync */
#define TAP_MP_KEY "tap_mp_sync_queues"
+#define TAP_MP_REQ_START_RXTX "tap_mp_req_start_rxtx"
#define TAP_IOV_DEFAULT_MAX 1024
@@ -880,11 +881,49 @@ tap_link_set_up(struct rte_eth_dev *dev)
return tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
}
+static int
+tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
+{
+ struct rte_mp_msg msg;
+ struct ipc_queues *request_param = (struct ipc_queues *)msg.param;
+ int err;
+ int fd_iterator = 0;
+ struct pmd_process_private *process_private = dev->process_private;
+ int i;
+
+ memset(&msg, 0, sizeof(msg));
+ strlcpy(msg.name, TAP_MP_REQ_START_RXTX, sizeof(msg.name));
+ strlcpy(request_param->port_name, dev->data->name, sizeof(request_param->port_name));
+ msg.len_param = sizeof(*request_param);
+ for (i = 0; i < dev->data->nb_tx_queues; i++) {
+ msg.fds[fd_iterator++] = process_private->txq_fds[i];
+ msg.num_fds++;
+ request_param->txq_count++;
+ }
+ for (i = 0; i < dev->data->nb_rx_queues; i++) {
+ msg.fds[fd_iterator++] = process_private->rxq_fds[i];
+ msg.num_fds++;
+ request_param->rxq_count++;
+ }
+
+ err = rte_mp_sendmsg(&msg);
+ if (err < 0) {
+ TAP_LOG(ERR, "Failed to send start req to secondary %d",
+ rte_errno);
+ return -1;
+ }
+
+ return 0;
+}
+
static int
tap_dev_start(struct rte_eth_dev *dev)
{
int err, i;
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+ tap_mp_req_on_rxtx(dev);
+
err = tap_intr_handle_set(dev, 1);
if (err)
return err;
@@ -901,6 +940,34 @@ tap_dev_start(struct rte_eth_dev *dev)
return err;
}
+static int
+tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused const void *peer)
+{
+ struct rte_eth_dev *dev;
+ const struct ipc_queues *request_param =
+ (const struct ipc_queues *)request->param;
+ int fd_iterator;
+ int queue;
+ struct pmd_process_private *process_private;
+
+ dev = rte_eth_dev_get_by_name(request_param->port_name);
+ if (!dev) {
+ TAP_LOG(ERR, "Failed to get dev for %s",
+ request_param->port_name);
+ return -1;
+ }
+ process_private = dev->process_private;
+ fd_iterator = 0;
+ TAP_LOG(DEBUG, "tap_attach rx_q:%d tx_q:%d\n", request_param->rxq_count,
+ request_param->txq_count);
+ for (queue = 0; queue < request_param->txq_count; queue++)
+ process_private->txq_fds[queue] = request->fds[fd_iterator++];
+ for (queue = 0; queue < request_param->rxq_count; queue++)
+ process_private->rxq_fds[queue] = request->fds[fd_iterator++];
+
+ return 0;
+}
+
/* This function gets called when the current port gets stopped.
*/
static int
@@ -1084,6 +1151,9 @@ tap_dev_close(struct rte_eth_dev *dev)
if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
rte_free(dev->process_private);
+ if (tap_devices_count == 1)
+ rte_mp_action_unregister(TAP_MP_REQ_START_RXTX);
+ tap_devices_count--;
return 0;
}
@@ -2445,6 +2515,16 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
ret = tap_mp_attach_queues(name, eth_dev);
if (ret != 0)
return -1;
+
+ if (!tap_devices_count) {
+ ret = rte_mp_action_register(TAP_MP_REQ_START_RXTX, tap_mp_req_start_rxtx);
+ if (ret < 0 && rte_errno != ENOTSUP) {
+ TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
+ strerror(rte_errno));
+ return -1;
+ }
+ }
+ tap_devices_count++;
rte_eth_dev_probing_finish(eth_dev);
return 0;
}
--
2.17.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 1/2] ethdev: define a function to get eth dev structure
2022-01-21 4:29 [PATCH] net/tap: Bug fix to populate fds in secondary process Kumara Parameshwaran
` (3 preceding siblings ...)
2022-01-31 14:21 ` [PATCH v3 1/2] ethdev: define a function to get eth dev structure Kumara Parameshwaran
@ 2022-01-31 14:32 ` Kumara Parameshwaran
2022-01-31 14:32 ` [PATCH v3 2/2] net/tap: fix to populate fds in secondary process Kumara Parameshwaran
2022-02-01 16:56 ` [PATCH v3 1/2] ethdev: define a function to get eth dev structure Ferruh Yigit
4 siblings, 2 replies; 44+ messages in thread
From: Kumara Parameshwaran @ 2022-01-31 14:32 UTC (permalink / raw)
To: dev; +Cc: ferruh.yigit, Kumara Parameshwaran, stable
From: Kumara Parameshwaran <kparameshwar@vmware.com>
The PMDs would need a function to access the rte_eth_devices
global array
Cc: stable@dpdk.org
Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
---
lib/ethdev/ethdev_driver.h | 18 ++++++++++++++++++
lib/ethdev/rte_ethdev.c | 11 +++++++++++
lib/ethdev/version.map | 1 +
3 files changed, 30 insertions(+)
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index d95605a355..7d27781f7d 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1629,6 +1629,24 @@ rte_eth_hairpin_queue_peer_bind(uint16_t cur_port, uint16_t cur_queue,
struct rte_hairpin_peer_info *peer_info,
uint32_t direction);
+/**
+ * @internal
+ * Get rte_eth_dev from device name. The device name should be specified
+ * as below:
+ * - PCIe address (Domain:Bus:Device.Function), for example- 0000:2:00.0
+ * - SoC device name, for example- fsl-gmac0
+ * - vdev dpdk name, for example- net_[pcap0|null0|tap0]
+ *
+ * @param name
+ * pci address or name of the device
+ * @return
+ * - rte_eth_dev if successful
+ * - NULL on failure
+ */
+__rte_internal
+struct rte_eth_dev*
+rte_eth_dev_get_by_name(const char *name);
+
/**
* @internal
* Reset the current queue state and configuration to disconnect (unbind) it
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index a1d475a292..968475d107 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -894,6 +894,17 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
return -ENODEV;
}
+struct rte_eth_dev *
+rte_eth_dev_get_by_name(const char *name)
+{
+ uint16_t pid;
+
+ if (rte_eth_dev_get_port_by_name(name, &pid))
+ return NULL;
+
+ return &rte_eth_devices[pid];
+}
+
static int
eth_err(uint16_t port_id, int ret)
{
diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
index c2fb0669a4..1f7359c846 100644
--- a/lib/ethdev/version.map
+++ b/lib/ethdev/version.map
@@ -267,6 +267,7 @@ INTERNAL {
rte_eth_dev_callback_process;
rte_eth_dev_create;
rte_eth_dev_destroy;
+ rte_eth_dev_get_by_name;
rte_eth_dev_is_rx_hairpin_queue;
rte_eth_dev_is_tx_hairpin_queue;
rte_eth_dev_probing_finish;
--
2.17.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 2/2] net/tap: fix to populate fds in secondary process
2022-01-31 14:32 ` [PATCH v3 1/2] ethdev: define a function to get eth dev structure Kumara Parameshwaran
@ 2022-01-31 14:32 ` Kumara Parameshwaran
2022-02-01 16:57 ` Ferruh Yigit
2022-02-01 16:56 ` [PATCH v3 1/2] ethdev: define a function to get eth dev structure Ferruh Yigit
1 sibling, 1 reply; 44+ messages in thread
From: Kumara Parameshwaran @ 2022-01-31 14:32 UTC (permalink / raw)
To: dev; +Cc: ferruh.yigit, Kumara Parameshwaran, stable
From: Kumara Parameshwaran <kparameshwar@vmware.com>
When a tap device is hotplugged to primary process which in turn
adds the device to all secondary process, the secondary process
does a tap_mp_attach_queues, but the fds are not populated in
the primary during the probe they are populated during the queue_setup,
added a fix to sync the queues during rte_eth_dev_start
Fixes: 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")
Cc: stable@dpdk.org
Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
---
v3:
* Retain tap_sync_queues to retain the attach of secondary process
* Fix coding convention for a function definition
* Renamed rte_get_eth_dev_by_name to rte_eth_dev_get_by_name in sorted
in version.map
* Remove uninteded blank line
drivers/net/tap/rte_eth_tap.c | 80 +++++++++++++++++++++++++++++++++++
1 file changed, 80 insertions(+)
diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index f1b48cae82..d13baadbe7 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -67,6 +67,7 @@
/* IPC key for queue fds sync */
#define TAP_MP_KEY "tap_mp_sync_queues"
+#define TAP_MP_REQ_START_RXTX "tap_mp_req_start_rxtx"
#define TAP_IOV_DEFAULT_MAX 1024
@@ -880,11 +881,49 @@ tap_link_set_up(struct rte_eth_dev *dev)
return tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
}
+static int
+tap_mp_req_on_rxtx(struct rte_eth_dev *dev)
+{
+ struct rte_mp_msg msg;
+ struct ipc_queues *request_param = (struct ipc_queues *)msg.param;
+ int err;
+ int fd_iterator = 0;
+ struct pmd_process_private *process_private = dev->process_private;
+ int i;
+
+ memset(&msg, 0, sizeof(msg));
+ strlcpy(msg.name, TAP_MP_REQ_START_RXTX, sizeof(msg.name));
+ strlcpy(request_param->port_name, dev->data->name, sizeof(request_param->port_name));
+ msg.len_param = sizeof(*request_param);
+ for (i = 0; i < dev->data->nb_tx_queues; i++) {
+ msg.fds[fd_iterator++] = process_private->txq_fds[i];
+ msg.num_fds++;
+ request_param->txq_count++;
+ }
+ for (i = 0; i < dev->data->nb_rx_queues; i++) {
+ msg.fds[fd_iterator++] = process_private->rxq_fds[i];
+ msg.num_fds++;
+ request_param->rxq_count++;
+ }
+
+ err = rte_mp_sendmsg(&msg);
+ if (err < 0) {
+ TAP_LOG(ERR, "Failed to send start req to secondary %d",
+ rte_errno);
+ return -1;
+ }
+
+ return 0;
+}
+
static int
tap_dev_start(struct rte_eth_dev *dev)
{
int err, i;
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+ tap_mp_req_on_rxtx(dev);
+
err = tap_intr_handle_set(dev, 1);
if (err)
return err;
@@ -901,6 +940,34 @@ tap_dev_start(struct rte_eth_dev *dev)
return err;
}
+static int
+tap_mp_req_start_rxtx(const struct rte_mp_msg *request, __rte_unused const void *peer)
+{
+ struct rte_eth_dev *dev;
+ const struct ipc_queues *request_param =
+ (const struct ipc_queues *)request->param;
+ int fd_iterator;
+ int queue;
+ struct pmd_process_private *process_private;
+
+ dev = rte_eth_dev_get_by_name(request_param->port_name);
+ if (!dev) {
+ TAP_LOG(ERR, "Failed to get dev for %s",
+ request_param->port_name);
+ return -1;
+ }
+ process_private = dev->process_private;
+ fd_iterator = 0;
+ TAP_LOG(DEBUG, "tap_attach rx_q:%d tx_q:%d\n", request_param->rxq_count,
+ request_param->txq_count);
+ for (queue = 0; queue < request_param->txq_count; queue++)
+ process_private->txq_fds[queue] = request->fds[fd_iterator++];
+ for (queue = 0; queue < request_param->rxq_count; queue++)
+ process_private->rxq_fds[queue] = request->fds[fd_iterator++];
+
+ return 0;
+}
+
/* This function gets called when the current port gets stopped.
*/
static int
@@ -1084,6 +1151,9 @@ tap_dev_close(struct rte_eth_dev *dev)
if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
rte_free(dev->process_private);
+ if (tap_devices_count == 1)
+ rte_mp_action_unregister(TAP_MP_REQ_START_RXTX);
+ tap_devices_count--;
return 0;
}
@@ -2445,6 +2515,16 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
ret = tap_mp_attach_queues(name, eth_dev);
if (ret != 0)
return -1;
+
+ if (!tap_devices_count) {
+ ret = rte_mp_action_register(TAP_MP_REQ_START_RXTX, tap_mp_req_start_rxtx);
+ if (ret < 0 && rte_errno != ENOTSUP) {
+ TAP_LOG(ERR, "tap: Failed to register IPC callback: %s",
+ strerror(rte_errno));
+ return -1;
+ }
+ }
+ tap_devices_count++;
rte_eth_dev_probing_finish(eth_dev);
return 0;
}
--
2.17.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 1/2] ethdev: define a function to get eth dev structure
2022-01-31 14:32 ` [PATCH v3 1/2] ethdev: define a function to get eth dev structure Kumara Parameshwaran
2022-01-31 14:32 ` [PATCH v3 2/2] net/tap: fix to populate fds in secondary process Kumara Parameshwaran
@ 2022-02-01 16:56 ` Ferruh Yigit
2022-02-01 17:12 ` Ferruh Yigit
2022-02-01 17:15 ` Ferruh Yigit
1 sibling, 2 replies; 44+ messages in thread
From: Ferruh Yigit @ 2022-02-01 16:56 UTC (permalink / raw)
To: Kumara Parameshwaran, dev
Cc: Kumara Parameshwaran, stable, Luca Boccassi, Kevin Traynor,
Christian Ehrhardt
On 1/31/2022 2:32 PM, Kumara Parameshwaran wrote:
> From: Kumara Parameshwaran <kparameshwar@vmware.com>
>
> The PMDs would need a function to access the rte_eth_devices
> global array
>
> Cc: stable@dpdk.org
>
Not sure if this patch is suitable for backport, since it introduces
new internal API.
But since API is internal, perhaps it is OK to get it, specially because
it is required for next patch that is a fix. Keeping the tag and leaving
the decision to LTS maintainers.
> Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 2/2] net/tap: fix to populate fds in secondary process
2022-01-31 14:32 ` [PATCH v3 2/2] net/tap: fix to populate fds in secondary process Kumara Parameshwaran
@ 2022-02-01 16:57 ` Ferruh Yigit
0 siblings, 0 replies; 44+ messages in thread
From: Ferruh Yigit @ 2022-02-01 16:57 UTC (permalink / raw)
To: Kumara Parameshwaran, dev; +Cc: Kumara Parameshwaran, stable
On 1/31/2022 2:32 PM, Kumara Parameshwaran wrote:
> From: Kumara Parameshwaran<kparameshwar@vmware.com>
>
> When a tap device is hotplugged to primary process which in turn
> adds the device to all secondary process, the secondary process
> does a tap_mp_attach_queues, but the fds are not populated in
> the primary during the probe they are populated during the queue_setup,
> added a fix to sync the queues during rte_eth_dev_start
>
> Fixes: 4852aa8f6e21 ("drivers/net: enable hotplug on secondary process")
> Cc:stable@dpdk.org
>
> Signed-off-by: Kumara Parameshwaran<kparameshwar@vmware.com>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 1/2] ethdev: define a function to get eth dev structure
2022-02-01 16:56 ` [PATCH v3 1/2] ethdev: define a function to get eth dev structure Ferruh Yigit
@ 2022-02-01 17:12 ` Ferruh Yigit
2022-02-01 17:15 ` Ferruh Yigit
1 sibling, 0 replies; 44+ messages in thread
From: Ferruh Yigit @ 2022-02-01 17:12 UTC (permalink / raw)
To: Kumara Parameshwaran, dev
Cc: Kumara Parameshwaran, stable, Thomas Monjalon, Andrew Rybchenko,
Singh, Aman Deep
On 2/1/2022 4:56 PM, Ferruh Yigit wrote:
> On 1/31/2022 2:32 PM, Kumara Parameshwaran wrote:
>> From: Kumara Parameshwaran <kparameshwar@vmware.com>
>>
>> The PMDs would need a function to access the rte_eth_devices
>> global array
>>
>> Cc: stable@dpdk.org
>>
>
> Not sure if this patch is suitable for backport, since it introduces
> new internal API.
> But since API is internal, perhaps it is OK to get it, specially because
> it is required for next patch that is a fix. Keeping the tag and leaving
> the decision to LTS maintainers.
>
>> Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
Hi Kumara,
There are some instances in the existing code that can benefit from new API [1],
do you have any bandwidth to make a patch to replace 'rte_eth_dev_get_port_by_name()'
with new 'rte_eth_dev_get_by_name()'?
(For the cases 'port_id' is used to get eth_dev)
Thanks,
ferruh
[1]
$ git grep rte_eth_dev_get_port_by_name drivers/net/
drivers/net/bonding/rte_eth_bond_api.c: ret = rte_eth_dev_get_port_by_name(name, &port_id);
drivers/net/ipn3ke/ipn3ke_ethdev.c: retval = rte_eth_dev_get_port_by_name(fvl_bdf,
drivers/net/memif/rte_eth_memif.c: ret = rte_eth_dev_get_port_by_name(msg_param->port_name, &port_id);
drivers/net/mlx5/linux/mlx5_os.c: if (rte_eth_dev_get_port_by_name(name, &port_id) == 0) {
drivers/net/mlx5/windows/mlx5_os.c: if (rte_eth_dev_get_port_by_name(name, &port_id) == 0) {
drivers/net/ring/rte_eth_ring.c: ret = rte_eth_dev_get_port_by_name(ring_name, &port_id);
drivers/net/softnic/rte_eth_softnic_cli.c: status = rte_eth_dev_get_port_by_name(softnic->params.name, &port_id);
drivers/net/softnic/rte_eth_softnic_cli.c: status = rte_eth_dev_get_port_by_name(softnic->params.name, &port_id);
drivers/net/softnic/rte_eth_softnic_cli.c: status = rte_eth_dev_get_port_by_name(softnic->params.name, &port_id);
drivers/net/softnic/rte_eth_softnic_cli.c: status = rte_eth_dev_get_port_by_name(softnic->params.name, &port_id);
drivers/net/softnic/rte_eth_softnic_cli.c: status = rte_eth_dev_get_port_by_name(softnic->params.name, &port_id);
drivers/net/softnic/rte_eth_softnic_internals.h: status = rte_eth_dev_get_port_by_name(softnic->params.name, &port_id);
drivers/net/softnic/rte_eth_softnic_link.c: status = rte_eth_dev_get_port_by_name(params->dev_name,
drivers/net/softnic/rte_eth_softnic_thread.c: status = rte_eth_dev_get_port_by_name(softnic->params.name, &port_id);
drivers/net/tap/rte_eth_tap.c: ret = rte_eth_dev_get_port_by_name(request_param->port_name, &port_id);
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 1/2] ethdev: define a function to get eth dev structure
2022-02-01 16:56 ` [PATCH v3 1/2] ethdev: define a function to get eth dev structure Ferruh Yigit
2022-02-01 17:12 ` Ferruh Yigit
@ 2022-02-01 17:15 ` Ferruh Yigit
1 sibling, 0 replies; 44+ messages in thread
From: Ferruh Yigit @ 2022-02-01 17:15 UTC (permalink / raw)
To: Kumara Parameshwaran, dev
Cc: Kumara Parameshwaran, stable, Luca Boccassi, Kevin Traynor,
Christian Ehrhardt
On 2/1/2022 4:56 PM, Ferruh Yigit wrote:
> On 1/31/2022 2:32 PM, Kumara Parameshwaran wrote:
>> From: Kumara Parameshwaran <kparameshwar@vmware.com>
>>
>> The PMDs would need a function to access the rte_eth_devices
>> global array
>>
>> Cc: stable@dpdk.org
>>
>
> Not sure if this patch is suitable for backport, since it introduces
> new internal API.
> But since API is internal, perhaps it is OK to get it, specially because
> it is required for next patch that is a fix. Keeping the tag and leaving
> the decision to LTS maintainers.
>
>> Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
Series applied to dpdk-next-net/main, thanks.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
2022-01-20 11:12 Kumara Parameshwaran
2022-01-20 15:49 ` Stephen Hemminger
@ 2022-01-24 9:32 ` Ferruh Yigit
1 sibling, 0 replies; 44+ messages in thread
From: Ferruh Yigit @ 2022-01-24 9:32 UTC (permalink / raw)
To: Kumara Parameshwaran; +Cc: dev, Kumara Parameshwaran, Stephen Hemminger
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: *;
> };
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
2022-01-20 11:12 Kumara Parameshwaran
@ 2022-01-20 15:49 ` Stephen Hemminger
2022-01-24 9:32 ` Ferruh Yigit
1 sibling, 0 replies; 44+ messages in thread
From: Stephen Hemminger @ 2022-01-20 15:49 UTC (permalink / raw)
To: Kumara Parameshwaran; +Cc: ferruh.yigit, dev, Kumara Parameshwaran
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.
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH] net/tap: Bug fix to populate fds in secondary process
@ 2022-01-20 13:38 Kumara Parameshwaran
0 siblings, 0 replies; 44+ messages in thread
From: Kumara Parameshwaran @ 2022-01-20 13:38 UTC (permalink / raw)
To: ferruh.yigit; +Cc: dev, 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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH] net/tap: Bug fix to populate fds in secondary process
@ 2022-01-20 13:26 Kumara Parameshwaran
0 siblings, 0 replies; 44+ messages in thread
From: Kumara Parameshwaran @ 2022-01-20 13:26 UTC (permalink / raw)
To: ferruh.yigit; +Cc: dev, 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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH] net/tap: Bug fix to populate fds in secondary process
@ 2022-01-20 11:12 Kumara Parameshwaran
2022-01-20 15:49 ` Stephen Hemminger
2022-01-24 9:32 ` Ferruh Yigit
0 siblings, 2 replies; 44+ messages in thread
From: Kumara Parameshwaran @ 2022-01-20 11:12 UTC (permalink / raw)
To: ferruh.yigit; +Cc: dev, 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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
2022-01-19 4:33 ` kumaraparameshwaran rathinavel
@ 2022-01-19 4:51 ` Stephen Hemminger
0 siblings, 0 replies; 44+ messages in thread
From: Stephen Hemminger @ 2022-01-19 4:51 UTC (permalink / raw)
To: kumaraparameshwaran rathinavel; +Cc: Kumara Parameshwaran, keith.wiles, dev
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.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
2022-01-18 16:21 ` Stephen Hemminger
@ 2022-01-19 4:33 ` kumaraparameshwaran rathinavel
2022-01-19 4:51 ` Stephen Hemminger
0 siblings, 1 reply; 44+ messages in thread
From: kumaraparameshwaran rathinavel @ 2022-01-19 4:33 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Kumara Parameshwaran, keith.wiles, dev
[-- 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 --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
2022-01-18 5:22 ` Kumara Parameshwaran
@ 2022-01-18 16:21 ` Stephen Hemminger
2022-01-19 4:33 ` kumaraparameshwaran rathinavel
0 siblings, 1 reply; 44+ messages in thread
From: Stephen Hemminger @ 2022-01-18 16:21 UTC (permalink / raw)
To: Kumara Parameshwaran; +Cc: Kumara Parameshwaran, keith.wiles, dev
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.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
2022-01-18 12:12 ` Ferruh Yigit
@ 2022-01-18 12:31 ` Thomas Monjalon
0 siblings, 0 replies; 44+ messages in thread
From: Thomas Monjalon @ 2022-01-18 12:31 UTC (permalink / raw)
To: kumaraparameshwaran rathinavel, Ferruh Yigit
Cc: Wiles, Keith, dev, Kumara Parameshwaran, Andrew Rybchenko,
David Marchand
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.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
2022-01-18 10:52 ` kumaraparameshwaran rathinavel
@ 2022-01-18 12:14 ` Ferruh Yigit
0 siblings, 0 replies; 44+ messages in thread
From: Ferruh Yigit @ 2022-01-18 12:14 UTC (permalink / raw)
To: kumaraparameshwaran rathinavel
Cc: Kumara Parameshwaran, keith.wiles, dev, Raslan Darawsheh
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.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
2022-01-18 11:21 ` kumaraparameshwaran rathinavel
@ 2022-01-18 12:12 ` Ferruh Yigit
2022-01-18 12:31 ` Thomas Monjalon
0 siblings, 1 reply; 44+ messages in thread
From: Ferruh Yigit @ 2022-01-18 12:12 UTC (permalink / raw)
To: kumaraparameshwaran rathinavel
Cc: Thomas Monjalon, Wiles, Keith, dev, Kumara Parameshwaran,
Andrew Rybchenko, David Marchand
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.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
2022-01-18 9:47 ` Ferruh Yigit
@ 2022-01-18 11:21 ` kumaraparameshwaran rathinavel
2022-01-18 12:12 ` Ferruh Yigit
0 siblings, 1 reply; 44+ messages in thread
From: kumaraparameshwaran rathinavel @ 2022-01-18 11:21 UTC (permalink / raw)
To: Ferruh Yigit
Cc: Thomas Monjalon, Wiles, Keith, dev, Kumara Parameshwaran,
Andrew Rybchenko, David Marchand
[-- 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 --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
2022-01-18 9:10 ` Ferruh Yigit
@ 2022-01-18 10:52 ` kumaraparameshwaran rathinavel
2022-01-18 12:14 ` Ferruh Yigit
0 siblings, 1 reply; 44+ messages in thread
From: kumaraparameshwaran rathinavel @ 2022-01-18 10:52 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: Kumara Parameshwaran, keith.wiles, dev, Raslan Darawsheh
[-- 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 --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
2022-01-17 18:33 ` Thomas Monjalon
@ 2022-01-18 9:47 ` Ferruh Yigit
2022-01-18 11:21 ` kumaraparameshwaran rathinavel
0 siblings, 1 reply; 44+ messages in thread
From: Ferruh Yigit @ 2022-01-18 9:47 UTC (permalink / raw)
To: Thomas Monjalon, Kumara Parameshwaran, keith.wiles
Cc: dev, Kumara Parameshwaran, Andrew Rybchenko, David Marchand
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.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
2022-01-18 4:39 ` Kumara Parameshwaran
@ 2022-01-18 9:10 ` Ferruh Yigit
2022-01-18 10:52 ` kumaraparameshwaran rathinavel
0 siblings, 1 reply; 44+ messages in thread
From: Ferruh Yigit @ 2022-01-18 9:10 UTC (permalink / raw)
To: Kumara Parameshwaran, Kumara Parameshwaran, keith.wiles
Cc: dev, Raslan Darawsheh
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.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
2022-01-17 22:16 ` Stephen Hemminger
@ 2022-01-18 5:22 ` Kumara Parameshwaran
2022-01-18 16:21 ` Stephen Hemminger
0 siblings, 1 reply; 44+ messages in thread
From: Kumara Parameshwaran @ 2022-01-18 5:22 UTC (permalink / raw)
To: Stephen Hemminger, Kumara Parameshwaran; +Cc: keith.wiles, dev
[-- 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 --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
2022-01-17 18:22 ` Ferruh Yigit
@ 2022-01-18 4:39 ` Kumara Parameshwaran
2022-01-18 9:10 ` Ferruh Yigit
0 siblings, 1 reply; 44+ messages in thread
From: Kumara Parameshwaran @ 2022-01-18 4:39 UTC (permalink / raw)
To: Ferruh Yigit, Kumara Parameshwaran, keith.wiles; +Cc: dev, Raslan Darawsheh
[-- 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 --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
2021-11-26 4:15 Kumara Parameshwaran
2022-01-17 18:22 ` Ferruh Yigit
2022-01-17 18:28 ` Ferruh Yigit
@ 2022-01-17 22:16 ` Stephen Hemminger
2022-01-18 5:22 ` Kumara Parameshwaran
2 siblings, 1 reply; 44+ messages in thread
From: Stephen Hemminger @ 2022-01-17 22:16 UTC (permalink / raw)
To: Kumara Parameshwaran; +Cc: keith.wiles, dev, Kumara Parameshwaran
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.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
2022-01-17 18:28 ` Ferruh Yigit
@ 2022-01-17 18:33 ` Thomas Monjalon
2022-01-18 9:47 ` Ferruh Yigit
0 siblings, 1 reply; 44+ messages in thread
From: Thomas Monjalon @ 2022-01-17 18:33 UTC (permalink / raw)
To: Kumara Parameshwaran, keith.wiles, Ferruh Yigit
Cc: dev, Kumara Parameshwaran, Andrew Rybchenko, David Marchand
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?
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
2021-11-26 4:15 Kumara Parameshwaran
2022-01-17 18:22 ` Ferruh Yigit
@ 2022-01-17 18:28 ` Ferruh Yigit
2022-01-17 18:33 ` Thomas Monjalon
2022-01-17 22:16 ` Stephen Hemminger
2 siblings, 1 reply; 44+ messages in thread
From: Ferruh Yigit @ 2022-01-17 18:28 UTC (permalink / raw)
To: Kumara Parameshwaran, keith.wiles
Cc: dev, Kumara Parameshwaran, Thomas Monjalon, Andrew Rybchenko,
David Marchand
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?
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] net/tap: Bug fix to populate fds in secondary process
2021-11-26 4:15 Kumara Parameshwaran
@ 2022-01-17 18:22 ` Ferruh Yigit
2022-01-18 4:39 ` Kumara Parameshwaran
2022-01-17 18:28 ` Ferruh Yigit
2022-01-17 22:16 ` Stephen Hemminger
2 siblings, 1 reply; 44+ messages in thread
From: Ferruh Yigit @ 2022-01-17 18:22 UTC (permalink / raw)
To: Kumara Parameshwaran, keith.wiles
Cc: dev, Kumara Parameshwaran, Raslan Darawsheh
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()'?
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH] net/tap: Bug fix to populate fds in secondary process
@ 2021-11-26 4:15 Kumara Parameshwaran
2022-01-17 18:22 ` Ferruh Yigit
` (2 more replies)
0 siblings, 3 replies; 44+ messages in thread
From: Kumara Parameshwaran @ 2021-11-26 4:15 UTC (permalink / raw)
To: keith.wiles; +Cc: dev, 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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH] net/tap: Bug fix to populate fds in secondary process
@ 2021-11-25 12:25 Kumara Parameshwaran
0 siblings, 0 replies; 44+ messages in thread
From: Kumara Parameshwaran @ 2021-11-25 12:25 UTC (permalink / raw)
To: keith.wiles; +Cc: dev, 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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH] net/tap: Bug fix to populate fds in secondary process
@ 2021-11-25 12:23 Kumara Parameshwaran
0 siblings, 0 replies; 44+ messages in thread
From: Kumara Parameshwaran @ 2021-11-25 12:23 UTC (permalink / raw)
To: keith.wiles; +Cc: dev, 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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH] net/tap: Bug fix to populate fds in secondary process
@ 2021-11-25 12:04 Kumara Parameshwaran
0 siblings, 0 replies; 44+ messages in thread
From: Kumara Parameshwaran @ 2021-11-25 12:04 UTC (permalink / raw)
To: keith.wiles; +Cc: dev, 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
^ permalink raw reply [flat|nested] 44+ messages in thread
end of thread, other threads:[~2022-02-01 17:16 UTC | newest]
Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-21 4:29 [PATCH] net/tap: Bug fix to populate fds in secondary process Kumara Parameshwaran
2022-01-24 9:47 ` Ferruh Yigit
2022-01-24 12:12 ` [PATCH v2] net/tap: " Kumara Parameshwaran
2022-01-24 12:35 ` Ferruh Yigit
2022-01-24 12:48 ` Kumara Parameshwaran
2022-01-24 12:54 ` kumaraparameshwaran rathinavel
2022-01-24 13:05 ` Ferruh Yigit
2022-01-28 9:55 ` kumaraparameshwaran rathinavel
2022-01-28 12:55 ` Ferruh Yigit
2022-01-24 12:37 ` Kumara Parameshwaran
2022-01-24 13:06 ` Ferruh Yigit
2022-01-31 14:21 ` [PATCH v3 1/2] ethdev: define a function to get eth dev structure Kumara Parameshwaran
2022-01-31 14:21 ` [PATCH v3 2/2] net/tap: fix to populate fds in secondary process Kumara Parameshwaran
2022-01-31 14:32 ` [PATCH v3 1/2] ethdev: define a function to get eth dev structure Kumara Parameshwaran
2022-01-31 14:32 ` [PATCH v3 2/2] net/tap: fix to populate fds in secondary process Kumara Parameshwaran
2022-02-01 16:57 ` Ferruh Yigit
2022-02-01 16:56 ` [PATCH v3 1/2] ethdev: define a function to get eth dev structure Ferruh Yigit
2022-02-01 17:12 ` Ferruh Yigit
2022-02-01 17:15 ` Ferruh Yigit
-- strict thread matches above, loose matches on Subject: below --
2022-01-20 13:38 [PATCH] net/tap: Bug fix to populate fds in secondary process Kumara Parameshwaran
2022-01-20 13:26 Kumara Parameshwaran
2022-01-20 11:12 Kumara Parameshwaran
2022-01-20 15:49 ` Stephen Hemminger
2022-01-24 9:32 ` Ferruh Yigit
2021-11-26 4:15 Kumara Parameshwaran
2022-01-17 18:22 ` Ferruh Yigit
2022-01-18 4:39 ` Kumara Parameshwaran
2022-01-18 9:10 ` Ferruh Yigit
2022-01-18 10:52 ` kumaraparameshwaran rathinavel
2022-01-18 12:14 ` Ferruh Yigit
2022-01-17 18:28 ` Ferruh Yigit
2022-01-17 18:33 ` Thomas Monjalon
2022-01-18 9:47 ` Ferruh Yigit
2022-01-18 11:21 ` kumaraparameshwaran rathinavel
2022-01-18 12:12 ` Ferruh Yigit
2022-01-18 12:31 ` Thomas Monjalon
2022-01-17 22:16 ` Stephen Hemminger
2022-01-18 5:22 ` Kumara Parameshwaran
2022-01-18 16:21 ` Stephen Hemminger
2022-01-19 4:33 ` kumaraparameshwaran rathinavel
2022-01-19 4:51 ` Stephen Hemminger
2021-11-25 12:25 Kumara Parameshwaran
2021-11-25 12:23 Kumara Parameshwaran
2021-11-25 12:04 Kumara Parameshwaran
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).