Re: [PATCH v2 1/3] ethdev: add traffic manager query function

[fengchengwen <fengchengwen@huawei.com>]

On 2024/10/8 22:43, Bruce Richardson wrote:
> Add function to allow querying a node in the scheduler tree.  Returns
> the parameters as were given to the add function. Adding this function
> allows apps to just query the hierarchy rather than having to maintain
> their own copies of it internally.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/ethdev/ethdev_trace.h        | 16 ++++++++++
>  lib/ethdev/ethdev_trace_points.c |  3 ++
>  lib/ethdev/rte_tm.c              | 25 ++++++++++++++++
>  lib/ethdev/rte_tm.h              | 50 ++++++++++++++++++++++++++++++++
>  lib/ethdev/rte_tm_driver.h       | 12 ++++++++
>  lib/ethdev/version.map           |  1 +
>  6 files changed, 107 insertions(+)
> 
> diff --git a/lib/ethdev/ethdev_trace.h b/lib/ethdev/ethdev_trace.h
> index 738d9829af..70fd6214fc 100644
> --- a/lib/ethdev/ethdev_trace.h
> +++ b/lib/ethdev/ethdev_trace.h
> @@ -1905,6 +1905,22 @@ RTE_TRACE_POINT(
>  	rte_trace_point_emit_int(ret);
>  )
>  
> +RTE_TRACE_POINT(
> +	rte_tm_trace_node_query,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint32_t node_id,
> +		uint32_t *parent_node_id, uint32_t *priority,
> +		uint32_t *weight, uint32_t *level_id,
> +		struct rte_tm_node_params *params, int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_u32(node_id);
> +	rte_trace_point_emit_ptr(parent_node_id);
> +	rte_trace_point_emit_ptr(priority);
> +	rte_trace_point_emit_ptr(weight);
> +	rte_trace_point_emit_ptr(level_id);
> +	rte_trace_point_emit_ptr(params);
> +	rte_trace_point_emit_int(ret);
> +)
> +
>  RTE_TRACE_POINT(
>  	rte_tm_trace_node_delete,
>  	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint32_t node_id, int ret),
> diff --git a/lib/ethdev/ethdev_trace_points.c b/lib/ethdev/ethdev_trace_points.c
> index 902e4f7533..eee23613b8 100644
> --- a/lib/ethdev/ethdev_trace_points.c
> +++ b/lib/ethdev/ethdev_trace_points.c
> @@ -706,6 +706,9 @@ RTE_TRACE_POINT_REGISTER(rte_tm_trace_mark_vlan_dei,
>  RTE_TRACE_POINT_REGISTER(rte_tm_trace_node_add,
>  	lib.ethdev.tm.node_add)
>  
> +RTE_TRACE_POINT_REGISTER(rte_tm_trace_node_query,
> +	lib.ethdev.tm.node_query)
> +
>  RTE_TRACE_POINT_REGISTER(rte_tm_trace_node_capabilities_get,
>  	lib.ethdev.tm.node_capabilities_get)
>  
> diff --git a/lib/ethdev/rte_tm.c b/lib/ethdev/rte_tm.c
> index d594fe0049..9ea140df09 100644
> --- a/lib/ethdev/rte_tm.c
> +++ b/lib/ethdev/rte_tm.c
> @@ -301,6 +301,31 @@ int rte_tm_node_add(uint16_t port_id,
>  	return ret;
>  }
>  
> +int rte_tm_node_query(uint16_t port_id,
> +	uint32_t node_id,
> +	uint32_t *parent_node_id,
> +	uint32_t *priority,
> +	uint32_t *weight,
> +	uint32_t *level_id,
> +	struct rte_tm_node_params *params,
> +	struct rte_tm_error *error)
> +{
> +	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> +	int ret;
> +
> +	if (dev == NULL)
> +		return -EINVAL;

The RTE_TM_FUNC will check it, so no need do above judgement.

> +
> +	ret = RTE_TM_FUNC(port_id, node_query)(dev,
> +		node_id, parent_node_id, priority, weight, level_id,
> +		params, error);
> +
> +	rte_tm_trace_node_query(port_id, node_id, parent_node_id, priority,
> +			      weight, level_id, params, ret);
> +
> +	return ret;
> +}
> +
>  /* Delete node from traffic manager hierarchy */
>  int rte_tm_node_delete(uint16_t port_id,
>  	uint32_t node_id,
> diff --git a/lib/ethdev/rte_tm.h b/lib/ethdev/rte_tm.h
> index 07028c9b36..395e9d6d7f 100644
> --- a/lib/ethdev/rte_tm.h
> +++ b/lib/ethdev/rte_tm.h
> @@ -20,6 +20,7 @@
>  
>  #include <rte_common.h>
>  #include <rte_meter.h>
> +#include <rte_compat.h>
>  
>  #ifdef __cplusplus
>  extern "C" {
> @@ -1599,6 +1600,55 @@ rte_tm_node_add(uint16_t port_id,
>  	struct rte_tm_node_params *params,
>  	struct rte_tm_error *error);
>  
> +/**
> + * Return information about a traffic management node
> + *
> + * Return information about a hierarchy node, using the same format of parameters
> + * as was passed to the rte_rm_node_add() function.
> + * Each of the "out" parameters pointers (except error) may be passed as NULL if the
> + * information is not needed by the caller. For example, to one may check if a node id
> + * is in use by:
> + *
> + *  struct rte_tm_error error;
> + *  int ret = rte_tm_node_query(port, node_id, NULL, NULL, NULL, NULL, NULL, &error);
> + *  if (ret == ENOENT) ...
> + *
> + * @param[in] port_id
> + *   The port identifier of the Ethernet device.
> + * @param[in] node_id
> + *   Node ID. Should be a valid node id.
> + * @param[out] parent_node_id
> + *   Parent node ID.
> + * @param[out] priority
> + *   Node priority. The highest node priority is zero. Used by the SP algorithm
> + *   running on the parent of the current node for scheduling this child node.
> + * @param[out] weight
> + *   Node weight. The node weight is relative to the weight sum of all siblings
> + *   that have the same priority. The lowest weight is one. Used by the WFQ
> + *   algorithm running on the parent of the current node for scheduling this
> + *   child node.
> + * @param[out] level_id
> + *   The node level in the scheduler hierarchy.
> + * @param[out] params
> + *   Node parameters, as would be used when creating the node.
> + * @param[out] error
> + *   Error details. Filled in only on error, when not NULL.

This parameter should not be NULL, as said before:
 Each of the "out" parameters pointers (except error) may be passed as NULL if the

> + * @return
> + *   0 on success, non-zero error code otherwise.
> + *   -EINVAL - port or node id value is invalid
> + *   -ENOENT - no node exists with the provided id

id -> port
no node exists with the provided port

> + */
> +__rte_experimental
> +int
> +rte_tm_node_query(uint16_t port_id,
> +	uint32_t node_id,
> +	uint32_t *parent_node_id,
> +	uint32_t *priority,
> +	uint32_t *weight,
> +	uint32_t *level_id,
> +	struct rte_tm_node_params *params,
> +	struct rte_tm_error *error);
> +

Suggest this new function place after node_resume in header/impl.c(e.g. source or trace), keep them consisteny

>  /**
>   * Traffic manager node delete
>   *
> diff --git a/lib/ethdev/rte_tm_driver.h b/lib/ethdev/rte_tm_driver.h
> index 45290fb3fd..1efb2caaec 100644
> --- a/lib/ethdev/rte_tm_driver.h
> +++ b/lib/ethdev/rte_tm_driver.h
> @@ -119,6 +119,16 @@ typedef int (*rte_tm_node_resume_t)(struct rte_eth_dev *dev,
>  	uint32_t node_id,
>  	struct rte_tm_error *error);
>  
> +/** @internal Traffic manager node query */
> +typedef int (*rte_tm_node_query_t)(const struct rte_eth_dev *dev,
> +	uint32_t node_id,
> +	uint32_t *parent_node_id,
> +	uint32_t *priority,
> +	uint32_t *weight,
> +	uint32_t *level_id,
> +	struct rte_tm_node_params *params,
> +	struct rte_tm_error *error);
> +
>  /** @internal Traffic manager hierarchy commit */
>  typedef int (*rte_tm_hierarchy_commit_t)(struct rte_eth_dev *dev,
>  	int clear_on_fail,
> @@ -248,6 +258,8 @@ struct rte_tm_ops {
>  	rte_tm_node_suspend_t node_suspend;
>  	/** Traffic manager node resume */
>  	rte_tm_node_resume_t node_resume;
> +	/** Traffic manager node resume */

Should be node query

> +	rte_tm_node_query_t node_query;
>  	/** Traffic manager hierarchy commit */
>  	rte_tm_hierarchy_commit_t hierarchy_commit;
>  
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index f63dc32aa2..c1a386eddc 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -335,6 +335,7 @@ EXPERIMENTAL {
>  	rte_eth_speed_lanes_get_capability;
>  	rte_eth_speed_lanes_set;
>  	rte_flow_async_create_by_index_with_pattern;
> +	rte_tm_node_query;
>  };
>  
>  INTERNAL {