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]

Reply via email to