Re: [PATCH v11 09/14] ethdev: add port mirroring feature

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

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 <stephen@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 <stephen@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]