On 8/8/25 19:55, Stephen Hemminger wrote:
This adds new feature port mirroring to the ethdev layer.
And standalone tests for those features.
Signed-off-by: Stephen Hemminger <step...@networkplumber.org>
[snip]
@@ -513,23 +551,25 @@ static_assert(sizeof(struct ethdev_mp_response) <=
RTE_MP_MAX_PARAM_LEN,
int
ethdev_server(const struct rte_mp_msg *mp_msg, const void *peer)
{
- const struct ethdev_mp_request *req
- = (const struct ethdev_mp_request *)mp_msg->param;
-
struct rte_mp_msg mp_resp = {
.name = ETHDEV_MP,
};
struct ethdev_mp_response *resp;
+ const struct ethdev_mp_request *req;
resp = (struct ethdev_mp_response *)mp_resp.param;
mp_resp.len_param = sizeof(*resp);
- resp->res_op = req->operation;
/* recv client requests */
- if (mp_msg->len_param != sizeof(*req))
+ if (mp_msg->len_param < (int)sizeof(*req)) {
+ RTE_ETHDEV_LOG_LINE(ERR, "invalid request from secondary");
resp->err_value = -EINVAL;
- else
- resp->err_value = ethdev_handle_request(req);
+ } else {
+ req = (const struct ethdev_mp_request *)mp_msg->param;
+ resp->res_op = req->operation;
+ resp->err_value = ethdev_handle_request(req, mp_msg->len_param);
+
+ }
Above changes in ethdev_server() looks unrelated to the patch.
return rte_mp_reply(&mp_resp, peer);
}
diff --git a/lib/ethdev/ethdev_private.h b/lib/ethdev/ethdev_private.h
index f58f161871..d2fdc20057 100644
--- a/lib/ethdev/ethdev_private.h
+++ b/lib/ethdev/ethdev_private.h
@@ -85,6 +85,9 @@ int eth_dev_tx_queue_config(struct rte_eth_dev *dev, uint16_t
nb_queues);
enum ethdev_mp_operation {
ETH_REQ_START,
ETH_REQ_STOP,
+ ETH_REQ_RESET,
unrelated to the patch
+ ETH_REQ_ADD_MIRROR,
+ ETH_REQ_REMOVE_MIRROR,
};
struct ethdev_mp_request {
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index adeec575be..ac889c220a 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -14,6 +14,8 @@
#include <bus_driver.h>
#include <eal_export.h>
#include <rte_log.h>
+#include <rte_cycles.h>
+#include <rte_alarm.h>
Looks unrelated
#include <rte_interrupts.h>
#include <rte_kvargs.h>
#include <rte_memcpy.h>
@@ -2041,13 +2043,16 @@ rte_eth_dev_reset(uint16_t port_id)
if (dev->dev_ops->dev_reset == NULL)
return -ENOTSUP;
- ret = rte_eth_dev_stop(port_id);
- if (ret != 0) {
- RTE_ETHDEV_LOG_LINE(ERR,
- "Failed to stop device (port %u) before reset: %s -
ignore",
- port_id, rte_strerror(-ret));
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+ ret = rte_eth_dev_stop(port_id);
+ if (ret != 0)
+ RTE_ETHDEV_LOG_LINE(ERR,
+ "Failed to stop device (port %u) before reset:
%s - ignore",
+ port_id, rte_strerror(-ret));
+ ret = eth_err(port_id, dev->dev_ops->dev_reset(dev));
+ } else {
+ ret = ethdev_request(port_id, ETH_REQ_STOP, NULL, 0);
}
- ret = eth_err(port_id, dev->dev_ops->dev_reset(dev));
rte_ethdev_trace_reset(port_id, ret);
Abov looks unrelated to the patch. Also it is unclear why just sto is
done in the case of secondary (without reset).
diff --git a/lib/ethdev/rte_mirror.c b/lib/ethdev/rte_mirror.c
new file mode 100644
index 0000000000..27b613b8ff
--- /dev/null
+++ b/lib/ethdev/rte_mirror.c
[snip]
+RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_eth_add_mirror, 25.11)
+int
+rte_eth_add_mirror(uint16_t port_id, const struct rte_eth_mirror_conf *conf)
+{
+#ifndef RTE_ETHDEV_MIRROR
+ return -ENOTSUP;
+#endif
+
+ RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+ struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+ struct rte_eth_dev_info dev_info;
+
+ if (conf == NULL) {
+ RTE_ETHDEV_LOG_LINE(ERR, "Missing configuration information");
Please, mention "mirror" above to make error less generic. Otherwise, it
could easierly overlap with other messages.
+ return -EINVAL;
+ }
+
+ if (conf->mp == NULL) {
+ RTE_ETHDEV_LOG_LINE(ERR, "not a valid mempool");
I'm confused since:
+ struct rte_mempool *mp; /**< Memory pool for copies, If NULL then
cloned. */
+ return -EINVAL;
+ }
+
+ if (conf->flags & ~(RTE_ETH_MIRROR_DIRECTION_MASK |
RTE_ETH_MIRROR_FLAG_MASK)) {
+ RTE_ETHDEV_LOG_LINE(ERR, "unsupported flags");
Same as above, may I suggest to mention mirror here as well.
+ return -EINVAL;
+ }
+
[snip]
+
+static inline void
inline looks suspicious here taking into account that the function has
loop. If you really need it, __rte_always_inline shoud be used.
Otherwise, just drop.
+eth_dev_mirror(uint16_t port_id, uint16_t queue_id, uint8_t direction,
+ struct rte_mbuf **pkts, uint16_t nb_pkts,
+ struct rte_eth_mirror *mirror)
[snip]
+static int
+ethdev_mirror_stats_reset(RTE_ATOMIC(struct rte_eth_mirror *) *head, uint16_t
target_id)
+{
+ struct rte_eth_mirror *mirror;
+
+ mirror = ethdev_find_mirror(head, target_id);
+ if (mirror == NULL)
+ return -1;
+
+ memset(&mirror->stats, 0, sizeof(struct rte_eth_mirror_stats));
You use sizeof(*stats) above, maybe it is better to be consistent here
as well and use sizeof(mirror->stats)?
[snip]
diff --git a/lib/ethdev/rte_mirror.h b/lib/ethdev/rte_mirror.h
new file mode 100644
index 0000000000..593ef477b6
--- /dev/null
+++ b/lib/ethdev/rte_mirror.h
IMHO rte_mirror sounds too generic. May be it would be better to name it
ethdev-specific: rte_ethdev_mirror.h
@@ -0,0 +1,147 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2025 Stephen Hemminger <step...@networkplumber.org>
+ */
+
+#ifndef RTE_MIRROR_H_
+#define RTE_MIRROR_H_
If name is changed above, don't forget to fix defines as well.
[snip]
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Create a port mirror instance.
+ *
+ * @param port_id
+ * The port identifier of the source Ethernet device.
+ * @param conf
+ * Settings for this MIRROR instance.
+ * @return
+ * Negative errno value on error, 0 on success.
+ */
+__rte_experimental
+int
+rte_eth_add_mirror(uint16_t port_id, const struct rte_eth_mirror_conf *conf);
Please, join above two lines for consistency with style below.
[snip]