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

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(+)
  

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 {
  

On Wed, Oct 09, 2024 at 08:57:41AM +0800, fengchengwen wrote:
> 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>
> > ---

Hi,

thanks for the detailed review. Most comments are fine and will fix. One
reply below for just one of them though.

/Bruce

<snip>
> > + */
> > +__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 consistency

Why do you think it should go after node resume? I deliberately placed it
after node_add function since the parameters are matching each other,
whatever parameters you provided on add, you get returned to you on query.
  

On 2024/10/9 16:07, Bruce Richardson wrote:
> On Wed, Oct 09, 2024 at 08:57:41AM +0800, fengchengwen wrote:
>> 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>
>>> ---
> 
> Hi,
> 
> thanks for the detailed review. Most comments are fine and will fix. One
> reply below for just one of them though.
> 
> /Bruce
> 
> <snip>
>>> + */
>>> +__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 consistency
> 
> Why do you think it should go after node resume? I deliberately placed it
> after node_add function since the parameters are matching each other,
> whatever parameters you provided on add, you get returned to you on query.

To one object, we may have four basic operation: add/del/modify/query, I always keep this order in .c/.h file.
As for the tm node case, we could treat node_suspend/node_resume as the modify operation.

It is just a personal coding style. From my point of view, it's fine in that order.

> 
>