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
@@ -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--;
}
}
@@ -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)
{
@@ -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
@@ -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 {