Re: [PATCH v5 3/5] net/ice: enhance Tx scheduler hierarchy support

["Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>]

Hi Bruce,

On 23/10/2024 17:55, Bruce Richardson wrote:
> Increase the flexibility of the Tx scheduler hierarchy support in the
> driver. If the HW/firmware allows it, allow creating up to 2k child
> nodes per scheduler node. Also expand the number of supported layers to
> the max available, rather than always just having 3 layers.  One
> restriction on this change is that the topology needs to be configured
> and enabled before port queue setup, in many cases, and before port
> start in all cases.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>   doc/guides/nics/ice.rst      |  31 +--
>   drivers/net/ice/ice_ethdev.c |   9 -
>   drivers/net/ice/ice_ethdev.h |  15 +-
>   drivers/net/ice/ice_rxtx.c   |  10 +
>   drivers/net/ice/ice_tm.c     | 496 ++++++++++++++---------------------
>   5 files changed, 224 insertions(+), 337 deletions(-)
>
<snip>
> @@ -393,16 +406,35 @@ ice_tm_node_add(struct rte_eth_dev *dev, uint32_t node_id,
>   	      struct rte_tm_error *error)
>   {
>   	struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> +	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>   	struct ice_tm_shaper_profile *shaper_profile = NULL;
>   	struct ice_tm_node *tm_node;
> -	struct ice_tm_node *parent_node;
> +	struct ice_tm_node *parent_node = NULL;
>   	int ret;
>   
>   	if (!params || !error)
>   		return -EINVAL;
>   
> -	ret = ice_node_param_check(pf, node_id, priority, weight,
> -				    params, error);
> +	if (parent_node_id != RTE_TM_NODE_ID_NULL) {
> +		parent_node = find_node(pf->tm_conf.root, parent_node_id);
> +		if (!parent_node) {
> +			error->type = RTE_TM_ERROR_TYPE_NODE_PARENT_NODE_ID;
> +			error->message = "parent not exist";
> +			return -EINVAL;
> +		}
> +	}
> +	if (level_id == RTE_TM_NODE_LEVEL_ID_ANY && parent_node != NULL)
> +		level_id = parent_node->level + 1;
> +
> +	/* check level */
> +	if (parent_node != NULL && level_id != parent_node->level + 1) {
> +		error->type = RTE_TM_ERROR_TYPE_NODE_PARAMS;
> +		error->message = "Wrong level";
> +		return -EINVAL;
> +	}
> +
> +	ret = ice_node_param_check(node_id, priority, weight,
> +			params, level_id == ice_get_leaf_level(hw), error);

As a suggestion, move the following section:

/* root node if not have a parent */
     if (parent_node_id == RTE_TM_NODE_ID_NULL) {

     ...

}

before those checks and simplify if statements

>   	if (ret)
>   		return ret;
>   
>
>
> @@ -573,7 +574,7 @@ ice_tm_node_delete(struct rte_eth_dev *dev, uint32_t node_id,
>   	}
>   
>   	/* root node */
> -	if (tm_node->level == ICE_TM_NODE_TYPE_PORT) {
> +	if (tm_node->level == 0) {
>   		rte_free(tm_node);
>   		pf->tm_conf.root = NULL;
>   		return 0;
> @@ -593,53 +594,6 @@ ice_tm_node_delete(struct rte_eth_dev *dev, uint32_t node_id,
>   	return 0;
>   }
>   
<snip>
> +int
> +ice_tm_setup_txq_node(struct ice_pf *pf, struct ice_hw *hw, uint16_t qid, uint32_t teid)
>   {
> -	struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> -	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> -	struct ice_sched_node *vsi_node = ice_get_vsi_node(hw);
> -	struct ice_tm_node *root = pf->tm_conf.root;
> -	uint32_t i;
> -	int ret;
> -
> -	/* reset vsi_node */
> -	ret = ice_set_node_rate(hw, NULL, vsi_node);
> -	if (ret) {
> -		PMD_DRV_LOG(ERR, "reset vsi node failed");
> -		return ret;
> -	}
> +	struct ice_sched_node *hw_node = ice_sched_find_node_by_teid(hw->port_info->root, teid);
> +	struct ice_tm_node *sw_node = find_node(pf->tm_conf.root, qid);
>   
> -	if (root == NULL)
> +	/* not configured in hierarchy */
> +	if (sw_node == NULL)
>   		return 0;
>   
> -	for (i = 0; i < root->reference_count; i++) {
> -		struct ice_tm_node *tm_node = root->children[i];
> +	sw_node->sched_node = hw_node;
>   
> -		if (tm_node->sched_node == NULL)
> -			continue;
> +	/* if the queue node has been put in the wrong place in hierarchy */
> +	if (hw_node->parent != sw_node->parent->sched_node) {
need to check hw_node for NULL
> +		struct ice_aqc_move_txqs_data *buf;
> +		uint8_t txqs_moved = 0;
> +		uint16_t buf_size = ice_struct_size(buf, txqs, 1);
> +
<snip>
>   
> -static int ice_hierarchy_commit(struct rte_eth_dev *dev,
> +static int
> +commit_new_hierarchy(struct rte_eth_dev *dev)
> +{
> +	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> +	struct ice_port_info *pi = hw->port_info;
> +	struct ice_tm_node *sw_root = pf->tm_conf.root;
> +	struct ice_sched_node *new_vsi_root = (pi->has_tc) ? pi->root->children[0] : pi->root;
> +	uint16_t nodes_created_per_level[10] = {0}; /* counted per hw level, not per logical */

I think it is worth to to change "10" with something meaningful. Also it 
is probably would be good to add an extra check against this value into:

ice_sched:ice_sched_query_res_alloc():

     ...

     hw->num_tx_sched_layers =
         (u8)LE16_TO_CPU(buf->sched_props.logical_levels);


> +	uint8_t q_lvl = ice_get_leaf_level(hw);
> +	uint8_t qg_lvl = q_lvl - 1;
<snip>

-- 
Regards,
Vladimir