[PATCH iwl-next v2 0/6] ice: refactor aggregator node logic

[PATCH iwl-next v2 0/6] ice: refactor aggregator node logic

[Jacob Keller]

The ice hardware uses aggregator nodes to track VSIs in the device
scheduler tree. The existing logic used by the ice driver results in a
two-layered tracking approach where the VSI has an ice_agg_node structure
that wraps the ice_sched_agg_info structure.

This logic is done outside of the scheduler code, and results in the driver
having two sources of "truth" for the aggregator nodes. If the two places
become out of sync it could lead to imbalances in the scheduler tree, or
failure to use the device resources as effectively.

In addition, the tracking logic requires each struct ice_pf to contain two
arrays, pf_agg_node and vf_agg_node, which track the ice_agg_node
structures. This wastes about a kilobyte of memory (~5% of the total PF
structure size).

Almost all of the information required is already stored in the
ice_sched_agg_info structure, so duplicating this information is redundant
and unnecessary.

This series refactors the logic in ice_sched.c, converting from a linked
list to an xarray, implements a new ice_cfg_vsi_agg() function to replace
the ice_cfg_agg() function. The new logic better integrates with what the
driver actually wants to do.

The result is that aggregator node information is tracked in only one
place, preventing the possibility that it gets out of sync.

-- Outstanding and preexisting issues --

It seems that since commit 47e3e53ceadc ("ice: Destroy scheduler tree in
reset path"), the reset paths now calls ice_rm_vsi_lan_cfg() which calls
ice_sched_rm_vsi_cfg() which will eventually call
ice_sched_rm_agg_vsi_info(), clearing the aggregator node info after a
reset. Since this removes the data from the agg_info structures, the replay
logic does not appear to replay aggregator node info properly after a
reset. The driver only calls ice_set_agg_vsi() once during ice_vsi_setup()
and does not call it during ice_vsi_cfg() or any rebuild path. I suspect
that post-reset the driver will leave all VSIs effectively in the default
aggregator node. This appears to be a preexisting issue since the
above-mentioned commit, which I have not attempted to fix in this series.

The logic for aggregator nodes and traffic classes is quite confusing. We
pass the ena_tc bits into the functions to create aggregator nodes, with
each node having a tc_bitmap of the enabled TCs. However, since we only
perform this setup during initialization, any changes to the traffic
classes do not get reflected in aggregator node configuration. The logic in
ice_sched.c is very confusing because it seems to imply that each VSI can
have its own set of traffic classes, but there is no attempt to handle the
case where a VSI with one set of TCs is placed onto an aggregator node
created with a different set of TCs. This appears to be a preexisting logic
since the beginning of driver development, which I have not attempted to
fix in this series.

Several places in the driver use a u8 for storing enabled TCs, while others
use proper bitmap APIs. This probably should be cleaned up so that
everything uses the same style instead of mixing and matching, copying or
casting from u8 to unsigned long, or using tc_bitmap[0] in places that
should have just passed the bitmap by pointer instead of by value. I
thought about cleaning this up but left it out of this series.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Changes in v2:
- Rename agg_list to agg_nodes to avoid implying the old data structure
  type
- Squash the cleanup of passing agg_info around into the patch for
  extracting aggregator allocation out of ice_sched_cfg_agg. This
  sequencing is cleaner and more logical.
- Add a patch to also pass agg_info instead of performing a lookup at a few
  other functions that now already have a valid agg_info pointer in the
  callers.
- Move configuring an allocator node into ice_find_available_agg, so that
  we only call ice_sched_cfg_agg() once per node. This fixes a behavioral
  difference with the previous implementation that was pointed out by
  Sashiko.
- Link to v1: https://patch.msgid.link/20260601-jk-cleanup-vsi-aggregator-nodes-v1-0-36d6ecbe5ede@intel.com

---
Jacob Keller (6):
      ice: convert hw->agg_list from linked list to xarray
      ice: count number of VSIS in agg_vsi_list
      ice: extract aggregator allocation out of ice_sched_cfg_agg
      ice: pass agg_info instead of performing lookup
      ice: remove ice_agg_node wrapper structure
      ice: remove unused aggregator node functions

 drivers/net/ethernet/intel/ice/ice.h        |  23 +-
 drivers/net/ethernet/intel/ice/ice_sched.h  |  13 +-
 drivers/net/ethernet/intel/ice/ice_type.h   |   2 +-
 drivers/net/ethernet/intel/ice/ice_common.c |   4 +-
 drivers/net/ethernet/intel/ice/ice_lib.c    | 114 ++--------
 drivers/net/ethernet/intel/ice/ice_main.c   |   7 -
 drivers/net/ethernet/intel/ice/ice_sched.c  | 336 ++++++++++++++++------------
 drivers/net/ethernet/intel/ice/ice_vf_lib.c |  22 +-
 8 files changed, 237 insertions(+), 284 deletions(-)
---
base-commit: 4fdf2a2915a3c1baa162671f8c7155cebf02354c
change-id: 20260505-jk-cleanup-vsi-aggregator-nodes-9bb5285cceb4

Best regards,
--  
Jacob Keller <jacob.e.keller@intel.com>

[PATCH iwl-next v2 1/6] ice: convert hw->agg_list from linked list to xarray

[Jacob Keller]

The scheduler code saves information about the aggregator nodes in a linked
list in the hw->agg_list structure. This choice of data structure is not
ideal. Indeed, several places in the code iterate the list searching to see
if a given ID is present.

Convert the linked list into an xarray. Where possible, simplify aggregator
info lookup to use xa_load instead of iterating over all the entries.

Switching to xarray data structure better matches the existing usage
pattern. In addition, it prepares the code to allow removal of the
ice_agg_node wrapping structure used by ice_vsi, which will be completed in
following changes.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_sched.h  |  1 -
 drivers/net/ethernet/intel/ice/ice_type.h   |  2 +-
 drivers/net/ethernet/intel/ice/ice_common.c |  4 +-
 drivers/net/ethernet/intel/ice/ice_sched.c  | 58 ++++++++++++-----------------
 4 files changed, 27 insertions(+), 38 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_sched.h b/drivers/net/ethernet/intel/ice/ice_sched.h
index 7b668083be07..f2a619fcab8b 100644
--- a/drivers/net/ethernet/intel/ice/ice_sched.h
+++ b/drivers/net/ethernet/intel/ice/ice_sched.h
@@ -67,7 +67,6 @@ struct ice_sched_agg_vsi_info {
 
 struct ice_sched_agg_info {
 	struct list_head agg_vsi_list;
-	struct list_head list_entry;
 	DECLARE_BITMAP(tc_bitmap, ICE_MAX_TRAFFIC_CLASS);
 	u32 agg_id;
 	enum ice_agg_type agg_type;
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index b8bdd7e8c768..83f7e809cfc4 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -928,7 +928,7 @@ struct ice_hw {
 	u8 max_cgds;
 	u8 sw_entry_point_layer;
 	u16 max_children[ICE_AQC_TOPO_MAX_LEVEL_NUM];
-	struct list_head agg_list;	/* lists all aggregator */
+	struct xarray agg_nodes; /* array of aggregator nodes */
 	struct xarray sched_node_ids;
 
 	struct ice_vsi_ctx *vsi_ctx[ICE_MAX_VSI];
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index feccd2b0f889..16f20b0feb02 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1052,6 +1052,7 @@ int ice_init_hw(struct ice_hw *hw)
 	hw->evb_veb = true;
 
 	xa_init_flags(&hw->sched_node_ids, XA_FLAGS_ALLOC);
+	xa_init(&hw->agg_nodes);
 
 	/* Query the allocated resources for Tx scheduler */
 	status = ice_sched_query_res_alloc(hw);
@@ -1091,7 +1092,6 @@ int ice_init_hw(struct ice_hw *hw)
 		status = -EIO;
 		goto err_unroll_sched;
 	}
-	INIT_LIST_HEAD(&hw->agg_list);
 	/* Initialize max burst size */
 	if (!hw->max_burst_size)
 		ice_cfg_rl_burst_size(hw, ICE_SCHED_DFLT_BURST_SIZE);
@@ -1147,6 +1147,7 @@ int ice_init_hw(struct ice_hw *hw)
 	ice_sched_cleanup_all(hw);
 err_unroll_xarray:
 	xa_destroy(&hw->sched_node_ids);
+	xa_destroy(&hw->agg_nodes);
 err_unroll_alloc:
 	devm_kfree(ice_hw_to_dev(hw), hw->port_info);
 err_unroll_cqinit:
@@ -1189,6 +1190,7 @@ void ice_deinit_hw(struct ice_hw *hw)
 	ice_clear_all_vsi_ctx(hw);
 
 	xa_destroy(&hw->sched_node_ids);
+	xa_destroy(&hw->agg_nodes);
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c
index ffa18d86729a..bcfc9db69e17 100644
--- a/drivers/net/ethernet/intel/ice/ice_sched.c
+++ b/drivers/net/ethernet/intel/ice/ice_sched.c
@@ -813,9 +813,9 @@ static void ice_sched_clear_rl_prof(struct ice_port_info *pi)
 void ice_sched_clear_agg(struct ice_hw *hw)
 {
 	struct ice_sched_agg_info *agg_info;
-	struct ice_sched_agg_info *atmp;
+	unsigned long index;
 
-	list_for_each_entry_safe(agg_info, atmp, &hw->agg_list, list_entry) {
+	xa_for_each(&hw->agg_nodes, index, agg_info) {
 		struct ice_sched_agg_vsi_info *agg_vsi_info;
 		struct ice_sched_agg_vsi_info *vtmp;
 
@@ -824,7 +824,7 @@ void ice_sched_clear_agg(struct ice_hw *hw)
 			list_del(&agg_vsi_info->list_entry);
 			devm_kfree(ice_hw_to_dev(hw), agg_vsi_info);
 		}
-		list_del(&agg_info->list_entry);
+		xa_erase(&hw->agg_nodes, index);
 		devm_kfree(ice_hw_to_dev(hw), agg_info);
 	}
 }
@@ -2060,10 +2060,9 @@ ice_sched_cfg_vsi(struct ice_port_info *pi, u16 vsi_handle, u8 tc, u16 maxqs,
 static void ice_sched_rm_agg_vsi_info(struct ice_port_info *pi, u16 vsi_handle)
 {
 	struct ice_sched_agg_info *agg_info;
-	struct ice_sched_agg_info *atmp;
+	unsigned long index;
 
-	list_for_each_entry_safe(agg_info, atmp, &pi->hw->agg_list,
-				 list_entry) {
+	xa_for_each(&pi->hw->agg_nodes, index, agg_info) {
 		struct ice_sched_agg_vsi_info *agg_vsi_info;
 		struct ice_sched_agg_vsi_info *vtmp;
 
@@ -2226,26 +2225,6 @@ int ice_rm_vsi_rdma_cfg(struct ice_port_info *pi, u16 vsi_handle)
 	return ice_sched_rm_vsi_cfg(pi, vsi_handle, ICE_SCHED_NODE_OWNER_RDMA);
 }
 
-/**
- * ice_get_agg_info - get the aggregator ID
- * @hw: pointer to the hardware structure
- * @agg_id: aggregator ID
- *
- * This function validates aggregator ID. The function returns info if
- * aggregator ID is present in list otherwise it returns null.
- */
-static struct ice_sched_agg_info *
-ice_get_agg_info(struct ice_hw *hw, u32 agg_id)
-{
-	struct ice_sched_agg_info *agg_info;
-
-	list_for_each_entry(agg_info, &hw->agg_list, list_entry)
-		if (agg_info->agg_id == agg_id)
-			return agg_info;
-
-	return NULL;
-}
-
 /**
  * ice_sched_get_free_vsi_parent - Find a free parent node in aggregator subtree
  * @hw: pointer to the HW struct
@@ -2619,7 +2598,7 @@ ice_save_agg_tc_bitmap(struct ice_port_info *pi, u32 agg_id,
 {
 	struct ice_sched_agg_info *agg_info;
 
-	agg_info = ice_get_agg_info(pi->hw, agg_id);
+	agg_info = xa_load(&pi->hw->agg_nodes, agg_id);
 	if (!agg_info)
 		return -EINVAL;
 	bitmap_copy(agg_info->replay_tc_bitmap, tc_bitmap,
@@ -2735,7 +2714,7 @@ ice_sched_cfg_agg(struct ice_port_info *pi, u32 agg_id,
 	int status = 0;
 	u8 tc;
 
-	agg_info = ice_get_agg_info(hw, agg_id);
+	agg_info = xa_load(&hw->agg_nodes, agg_id);
 	if (!agg_info) {
 		/* Create new entry for new aggregator ID */
 		agg_info = devm_kzalloc(ice_hw_to_dev(hw), sizeof(*agg_info),
@@ -2750,8 +2729,13 @@ ice_sched_cfg_agg(struct ice_port_info *pi, u32 agg_id,
 		/* Initialize the aggregator VSI list head */
 		INIT_LIST_HEAD(&agg_info->agg_vsi_list);
 
-		/* Add new entry in aggregator list */
-		list_add(&agg_info->list_entry, &hw->agg_list);
+		/* Add new entry in aggregator array */
+		status = xa_insert(&hw->agg_nodes, agg_id, agg_info,
+				   GFP_KERNEL);
+		if (status) {
+			devm_kfree(ice_hw_to_dev(hw), agg_info);
+			return status;
+		}
 	}
 	/* Create aggregator node(s) for requested TC(s) */
 	ice_for_each_traffic_class(tc) {
@@ -2836,8 +2820,9 @@ static struct ice_sched_agg_info *
 ice_get_vsi_agg_info(struct ice_hw *hw, u16 vsi_handle)
 {
 	struct ice_sched_agg_info *agg_info;
+	unsigned long index;
 
-	list_for_each_entry(agg_info, &hw->agg_list, list_entry) {
+	xa_for_each(&hw->agg_nodes, index, agg_info) {
 		struct ice_sched_agg_vsi_info *agg_vsi_info;
 
 		agg_vsi_info = ice_get_agg_vsi_info(agg_info, vsi_handle);
@@ -2864,7 +2849,7 @@ ice_save_agg_vsi_tc_bitmap(struct ice_port_info *pi, u32 agg_id, u16 vsi_handle,
 	struct ice_sched_agg_vsi_info *agg_vsi_info;
 	struct ice_sched_agg_info *agg_info;
 
-	agg_info = ice_get_agg_info(pi->hw, agg_id);
+	agg_info = xa_load(&pi->hw->agg_nodes, agg_id);
 	if (!agg_info)
 		return -EINVAL;
 	/* check if entry already exist */
@@ -2899,7 +2884,7 @@ ice_sched_assoc_vsi_to_agg(struct ice_port_info *pi, u32 agg_id,
 
 	if (!ice_is_vsi_valid(pi->hw, vsi_handle))
 		return -EINVAL;
-	agg_info = ice_get_agg_info(hw, agg_id);
+	agg_info = xa_load(&hw->agg_nodes, agg_id);
 	if (!agg_info)
 		return -EINVAL;
 	/* If the VSI is already part of another aggregator then update
@@ -4314,9 +4299,10 @@ void ice_sched_replay_agg(struct ice_hw *hw)
 {
 	struct ice_port_info *pi = hw->port_info;
 	struct ice_sched_agg_info *agg_info;
+	unsigned long index;
 
 	mutex_lock(&pi->sched_lock);
-	list_for_each_entry(agg_info, &hw->agg_list, list_entry)
+	xa_for_each(&hw->agg_nodes, index, agg_info) {
 		/* replay aggregator (re-create aggregator node) */
 		if (!bitmap_equal(agg_info->tc_bitmap, agg_info->replay_tc_bitmap,
 				  ICE_MAX_TRAFFIC_CLASS)) {
@@ -4339,6 +4325,7 @@ void ice_sched_replay_agg(struct ice_hw *hw)
 				continue;
 			}
 		}
+	}
 	mutex_unlock(&pi->sched_lock);
 }
 
@@ -4353,9 +4340,10 @@ void ice_sched_replay_agg_vsi_preinit(struct ice_hw *hw)
 {
 	struct ice_port_info *pi = hw->port_info;
 	struct ice_sched_agg_info *agg_info;
+	unsigned long index;
 
 	mutex_lock(&pi->sched_lock);
-	list_for_each_entry(agg_info, &hw->agg_list, list_entry) {
+	xa_for_each(&hw->agg_nodes, index, agg_info) {
 		struct ice_sched_agg_vsi_info *agg_vsi_info;
 
 		agg_info->tc_bitmap[0] = 0;

-- 
2.56.0.rc0.395.gd1f3524e15dc

[PATCH iwl-next v2 2/6] ice: count number of VSIS in agg_vsi_list

[Jacob Keller]

The ice_sched_agg_info structure maintains a list of VSIs associated with
the aggregator node. A following change is going to refactor logic from
ice_lib.c, and remove the ice_agg_node wrapper structure. Doing so will
require keeping track of how many VSIs are on a given aggregator node.

To simplify this tracking, add a member to the ice_sched_agg_info structure
which counts the number of VSIs using the node. Increment it when adding a
VSI to the list, and decrement when removing the VSI from the list. This
will avoid the need to iterate the agg_vsi_list to determine this
information.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_sched.h | 1 +
 drivers/net/ethernet/intel/ice/ice_sched.c | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_sched.h b/drivers/net/ethernet/intel/ice/ice_sched.h
index f2a619fcab8b..992aafc4369d 100644
--- a/drivers/net/ethernet/intel/ice/ice_sched.h
+++ b/drivers/net/ethernet/intel/ice/ice_sched.h
@@ -68,6 +68,7 @@ struct ice_sched_agg_vsi_info {
 struct ice_sched_agg_info {
 	struct list_head agg_vsi_list;
 	DECLARE_BITMAP(tc_bitmap, ICE_MAX_TRAFFIC_CLASS);
+	u32 num_vsis;
 	u32 agg_id;
 	enum ice_agg_type agg_type;
 	/* bw_t_info saves aggregator BW information */
diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c
index bcfc9db69e17..08d0f685f6c4 100644
--- a/drivers/net/ethernet/intel/ice/ice_sched.c
+++ b/drivers/net/ethernet/intel/ice/ice_sched.c
@@ -823,8 +823,10 @@ void ice_sched_clear_agg(struct ice_hw *hw)
 					 &agg_info->agg_vsi_list, list_entry) {
 			list_del(&agg_vsi_info->list_entry);
 			devm_kfree(ice_hw_to_dev(hw), agg_vsi_info);
+			agg_info->num_vsis--;
 		}
 		xa_erase(&hw->agg_nodes, index);
+		WARN_ON(agg_info->num_vsis > 0);
 		devm_kfree(ice_hw_to_dev(hw), agg_info);
 	}
 }
@@ -2072,6 +2074,7 @@ static void ice_sched_rm_agg_vsi_info(struct ice_port_info *pi, u16 vsi_handle)
 				list_del(&agg_vsi_info->list_entry);
 				devm_kfree(ice_hw_to_dev(pi->hw),
 					   agg_vsi_info);
+				agg_info->num_vsis--;
 				return;
 			}
 	}
@@ -2473,6 +2476,7 @@ ice_move_all_vsi_to_dflt_agg(struct ice_port_info *pi,
 		if (rm_vsi_info && !agg_vsi_info->tc_bitmap[0]) {
 			list_del(&agg_vsi_info->list_entry);
 			devm_kfree(ice_hw_to_dev(pi->hw), agg_vsi_info);
+			agg_info->num_vsis--;
 		}
 	}
 
@@ -2728,6 +2732,7 @@ ice_sched_cfg_agg(struct ice_port_info *pi, u32 agg_id,
 
 		/* Initialize the aggregator VSI list head */
 		INIT_LIST_HEAD(&agg_info->agg_vsi_list);
+		agg_info->num_vsis = 0;
 
 		/* Add new entry in aggregator array */
 		status = xa_insert(&hw->agg_nodes, agg_id, agg_info,
@@ -2915,6 +2920,7 @@ ice_sched_assoc_vsi_to_agg(struct ice_port_info *pi, u32 agg_id,
 		/* add VSI ID into the aggregator list */
 		agg_vsi_info->vsi_handle = vsi_handle;
 		list_add(&agg_vsi_info->list_entry, &agg_info->agg_vsi_list);
+		agg_info->num_vsis++;
 	}
 	/* Move VSI node to new aggregator node for requested TC(s) */
 	ice_for_each_traffic_class(tc) {
@@ -2933,6 +2939,7 @@ ice_sched_assoc_vsi_to_agg(struct ice_port_info *pi, u32 agg_id,
 	if (old_agg_vsi_info && !old_agg_vsi_info->tc_bitmap[0]) {
 		list_del(&old_agg_vsi_info->list_entry);
 		devm_kfree(ice_hw_to_dev(pi->hw), old_agg_vsi_info);
+		old_agg_info->num_vsis--;
 	}
 	return status;
 }

-- 
2.56.0.rc0.395.gd1f3524e15dc

[PATCH iwl-next v2 3/6] ice: extract aggregator allocation out of ice_sched_cfg_agg

[Jacob Keller]

The ice_sched_cfg_agg() function currently takes an aggregator id and type.
It performs a lookup of the aggregator info structure by ID. If the lookup
fails, it then tries to allocate a new structure at that ID.

The only caller that currently takes advantage of the allocation behavior
is ice_cfg_agg(). The other callers are part of a replay loop which iterate
the set of nodes and thus always have a valid node at the given ID.

A future refactor is going to change how aggregator nodes get allocated and
better integrating logic from outside ice_sched.c

As a first step, refactor the allocation to a separate function,
ice_alloc_agg_info(). To prepare for the following changes, this function
takes both a minimum and maximum range and uses xa_alloc() to allow
selecting the ID from the xarray.

Refactor ice_sched_cfg_agg() to take a pointer to the agg_info. Move the
allocation out to ice_cfg_agg(). All other callers of ice_sched_cfg_agg()
now just pass their agg_info pointer instead of agg_info->agg_id,
simplifying the behavior and removing error paths that are effectively dead
code.

Suggested-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_common.c |   2 +-
 drivers/net/ethernet/intel/ice/ice_sched.c  | 125 ++++++++++++++++------------
 2 files changed, 74 insertions(+), 53 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 16f20b0feb02..b0958a11b0c1 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1052,7 +1052,7 @@ int ice_init_hw(struct ice_hw *hw)
 	hw->evb_veb = true;
 
 	xa_init_flags(&hw->sched_node_ids, XA_FLAGS_ALLOC);
-	xa_init(&hw->agg_nodes);
+	xa_init_flags(&hw->agg_nodes, XA_FLAGS_ALLOC);
 
 	/* Query the allocated resources for Tx scheduler */
 	status = ice_sched_query_res_alloc(hw);
diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c
index 08d0f685f6c4..27971cb135fa 100644
--- a/drivers/net/ethernet/intel/ice/ice_sched.c
+++ b/drivers/net/ethernet/intel/ice/ice_sched.c
@@ -2694,61 +2694,78 @@ ice_sched_add_agg_cfg(struct ice_port_info *pi, u32 agg_id, u8 tc)
 }
 
 /**
- * ice_sched_cfg_agg - configure aggregator node
- * @pi: port information structure
- * @agg_id: aggregator ID
- * @agg_type: aggregator type queue, VSI, or aggregator group
- * @tc_bitmap: bits TC bitmap
+ * ice_alloc_agg_info - Allocate aggregator info structure
+ * @hw: pointer to the HW structure
+ * @min_id: the minimum ID to allocate
+ * @max_id: the maximum ID to allocate
+ * @agg_type: the aggregator node type
  *
- * It registers a unique aggregator node into scheduler services. It
- * allows a user to register with a unique ID to track it's resources.
- * The aggregator type determines if this is a queue group, VSI group
- * or aggregator group. It then creates the aggregator node(s) for requested
- * TC(s) or removes an existing aggregator node including its configuration
- * if indicated via tc_bitmap. Call ice_rm_agg_cfg to release aggregator
- * resources and remove aggregator ID.
- * This function needs to be called with scheduler lock held.
+ * Allocates a new aggregator info structure with an ID between min_id and
+ * max_id, inserting it into the agg_nodes xarray. If an exact ID is required,
+ * use the same value for both min_id and max_id.
+ *
+ * Context: Must be called while holding the scheduler lock.
+ *
+ * Return: the allocated aggregator info structure pointer, or an ERR_PTR on
+ * failure.
  */
-static int
-ice_sched_cfg_agg(struct ice_port_info *pi, u32 agg_id,
-		  enum ice_agg_type agg_type, unsigned long *tc_bitmap)
+static struct ice_sched_agg_info *
+ice_alloc_agg_info(struct ice_hw *hw, u32 min_id, u32 max_id,
+		   enum ice_agg_type agg_type)
 {
 	struct ice_sched_agg_info *agg_info;
-	struct ice_hw *hw = pi->hw;
-	int status = 0;
+	u32 agg_id;
+	int status;
+
+	/* Create new entry for new aggregator ID */
+	agg_info = devm_kzalloc(ice_hw_to_dev(hw), sizeof(*agg_info),
+				GFP_KERNEL);
+	if (!agg_info)
+		return ERR_PTR(-ENOMEM);
+
+	agg_info->agg_type = agg_type;
+	agg_info->tc_bitmap[0] = 0;
+
+	/* Initialize the aggregator VSI list head */
+	INIT_LIST_HEAD(&agg_info->agg_vsi_list);
+	agg_info->num_vsis = 0;
+
+	status = xa_alloc(&hw->agg_nodes, &agg_id, agg_info,
+			  XA_LIMIT(min_id, max_id), GFP_KERNEL);
+	if (status) {
+		devm_kfree(ice_hw_to_dev(hw), agg_info);
+		return ERR_PTR(status);
+	}
+	agg_info->agg_id = agg_id;
+
+	return agg_info;
+}
+
+/**
+ * ice_sched_cfg_agg - configure aggregator node
+ * @pi: port information structure
+ * @agg_info: pointer to aggregator node to configure
+ * @tc_bitmap: bits TC bitmap
+ *
+ * Configure an aggregator node's traffic classes according to the provided
+ * tc_bitmap.
+ *
+ * Context: Must be called with scheduler lock held.
+ */
+static int ice_sched_cfg_agg(struct ice_port_info *pi,
+			     struct ice_sched_agg_info *agg_info,
+			     unsigned long *tc_bitmap)
+{
+	int status;
 	u8 tc;
 
-	agg_info = xa_load(&hw->agg_nodes, agg_id);
-	if (!agg_info) {
-		/* Create new entry for new aggregator ID */
-		agg_info = devm_kzalloc(ice_hw_to_dev(hw), sizeof(*agg_info),
-					GFP_KERNEL);
-		if (!agg_info)
-			return -ENOMEM;
-
-		agg_info->agg_id = agg_id;
-		agg_info->agg_type = agg_type;
-		agg_info->tc_bitmap[0] = 0;
-
-		/* Initialize the aggregator VSI list head */
-		INIT_LIST_HEAD(&agg_info->agg_vsi_list);
-		agg_info->num_vsis = 0;
-
-		/* Add new entry in aggregator array */
-		status = xa_insert(&hw->agg_nodes, agg_id, agg_info,
-				   GFP_KERNEL);
-		if (status) {
-			devm_kfree(ice_hw_to_dev(hw), agg_info);
-			return status;
-		}
-	}
 	/* Create aggregator node(s) for requested TC(s) */
 	ice_for_each_traffic_class(tc) {
 		if (!ice_is_tc_ena(*tc_bitmap, tc)) {
 			/* Delete aggregator cfg TC if it exists previously */
 			status = ice_rm_agg_cfg_tc(pi, agg_info, tc, false);
 			if (status)
-				break;
+				return status;
 			continue;
 		}
 
@@ -2757,15 +2774,15 @@ ice_sched_cfg_agg(struct ice_port_info *pi, u32 agg_id,
 			continue;
 
 		/* Create new aggregator node for TC */
-		status = ice_sched_add_agg_cfg(pi, agg_id, tc);
+		status = ice_sched_add_agg_cfg(pi, agg_info->agg_id, tc);
 		if (status)
-			break;
+			return status;
 
 		/* Save aggregator node's TC information */
 		set_bit(tc, agg_info->tc_bitmap);
 	}
 
-	return status;
+	return 0;
 }
 
 /**
@@ -2781,11 +2798,18 @@ int
 ice_cfg_agg(struct ice_port_info *pi, u32 agg_id, enum ice_agg_type agg_type,
 	    u8 tc_bitmap)
 {
+	struct ice_sched_agg_info *agg_info;
 	unsigned long bitmap = tc_bitmap;
+	struct ice_hw *hw = pi->hw;
 	int status;
 
 	mutex_lock(&pi->sched_lock);
-	status = ice_sched_cfg_agg(pi, agg_id, agg_type, &bitmap);
+	agg_info = xa_load(&hw->agg_nodes, agg_id);
+	if (!agg_info)
+		agg_info = ice_alloc_agg_info(hw, agg_id, agg_id, agg_type);
+	if (IS_ERR(agg_info))
+		return PTR_ERR(agg_info);
+	status = ice_sched_cfg_agg(pi, agg_info, &bitmap);
 	if (!status)
 		status = ice_save_agg_tc_bitmap(pi, agg_id, &bitmap);
 	mutex_unlock(&pi->sched_lock);
@@ -4321,9 +4345,7 @@ void ice_sched_replay_agg(struct ice_hw *hw)
 						    agg_info->replay_tc_bitmap,
 						    replay_bitmap);
 			status = ice_sched_cfg_agg(hw->port_info,
-						   agg_info->agg_id,
-						   ICE_AGG_TYPE_AGG,
-						   replay_bitmap);
+						   agg_info, replay_bitmap);
 			if (status) {
 				dev_info(ice_hw_to_dev(hw),
 					 "Replay agg id[%d] failed\n",
@@ -4390,8 +4412,7 @@ static int ice_sched_replay_vsi_agg(struct ice_hw *hw, u16 vsi_handle)
 	ice_sched_get_ena_tc_bitmap(pi, agg_info->replay_tc_bitmap,
 				    replay_bitmap);
 	/* Replay aggregator node associated to vsi_handle */
-	status = ice_sched_cfg_agg(hw->port_info, agg_info->agg_id,
-				   ICE_AGG_TYPE_AGG, replay_bitmap);
+	status = ice_sched_cfg_agg(hw->port_info, agg_info, replay_bitmap);
 	if (status)
 		return status;
 

-- 
2.56.0.rc0.395.gd1f3524e15dc

[PATCH iwl-next v2 4/6] ice: pass agg_info instead of performing lookup

[Jacob Keller]

Several of the functions involving aggregator nodes take the agg_id
parameter and perform a lookup via xa_load. In some cases, the functions
are called only from inside other paths that already performed the relevant
lookup and have a valid agg_info structure pointer.

Refactor these functions to take the agg_info directly instead of
performing an unnecessary lookup. This simplifies the implementation and
removes potential error paths that can't properly fail.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_sched.c | 61 ++++++++++++------------------
 1 file changed, 25 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c
index 27971cb135fa..b1c2901b23f1 100644
--- a/drivers/net/ethernet/intel/ice/ice_sched.c
+++ b/drivers/net/ethernet/intel/ice/ice_sched.c
@@ -2589,25 +2589,17 @@ ice_rm_agg_cfg_tc(struct ice_port_info *pi, struct ice_sched_agg_info *agg_info,
 
 /**
  * ice_save_agg_tc_bitmap - save aggregator TC bitmap
- * @pi: port information structure
- * @agg_id: aggregator ID
+ * @agg_info: aggregator node
  * @tc_bitmap: 8 bits TC bitmap
  *
  * Save aggregator TC bitmap. This function needs to be called with scheduler
  * lock held.
  */
-static int
-ice_save_agg_tc_bitmap(struct ice_port_info *pi, u32 agg_id,
-		       unsigned long *tc_bitmap)
+static void ice_save_agg_tc_bitmap(struct ice_sched_agg_info *agg_info,
+				   unsigned long *tc_bitmap)
 {
-	struct ice_sched_agg_info *agg_info;
-
-	agg_info = xa_load(&pi->hw->agg_nodes, agg_id);
-	if (!agg_info)
-		return -EINVAL;
 	bitmap_copy(agg_info->replay_tc_bitmap, tc_bitmap,
 		    ICE_MAX_TRAFFIC_CLASS);
-	return 0;
 }
 
 /**
@@ -2811,7 +2803,7 @@ ice_cfg_agg(struct ice_port_info *pi, u32 agg_id, enum ice_agg_type agg_type,
 		return PTR_ERR(agg_info);
 	status = ice_sched_cfg_agg(pi, agg_info, &bitmap);
 	if (!status)
-		status = ice_save_agg_tc_bitmap(pi, agg_id, &bitmap);
+		ice_save_agg_tc_bitmap(agg_info, &bitmap);
 	mutex_unlock(&pi->sched_lock);
 	return status;
 }
@@ -2863,24 +2855,19 @@ ice_get_vsi_agg_info(struct ice_hw *hw, u16 vsi_handle)
 
 /**
  * ice_save_agg_vsi_tc_bitmap - save aggregator VSI TC bitmap
- * @pi: port information structure
- * @agg_id: aggregator ID
+ * @agg_info: aggregator node
  * @vsi_handle: software VSI handle
  * @tc_bitmap: TC bitmap of enabled TC(s)
  *
  * Save VSI to aggregator TC bitmap. This function needs to call with scheduler
  * lock held.
  */
-static int
-ice_save_agg_vsi_tc_bitmap(struct ice_port_info *pi, u32 agg_id, u16 vsi_handle,
-			   unsigned long *tc_bitmap)
+static int ice_save_agg_vsi_tc_bitmap(struct ice_sched_agg_info *agg_info,
+				      u16 vsi_handle,
+				      unsigned long *tc_bitmap)
 {
 	struct ice_sched_agg_vsi_info *agg_vsi_info;
-	struct ice_sched_agg_info *agg_info;
 
-	agg_info = xa_load(&pi->hw->agg_nodes, agg_id);
-	if (!agg_info)
-		return -EINVAL;
 	/* check if entry already exist */
 	agg_vsi_info = ice_get_agg_vsi_info(agg_info, vsi_handle);
 	if (!agg_vsi_info)
@@ -2893,7 +2880,7 @@ ice_save_agg_vsi_tc_bitmap(struct ice_port_info *pi, u32 agg_id, u16 vsi_handle,
 /**
  * ice_sched_assoc_vsi_to_agg - associate/move VSI to new/default aggregator
  * @pi: port information structure
- * @agg_id: aggregator ID
+ * @agg_info: aggregator node
  * @vsi_handle: software VSI handle
  * @tc_bitmap: TC bitmap of enabled TC(s)
  *
@@ -2901,20 +2888,17 @@ ice_save_agg_vsi_tc_bitmap(struct ice_port_info *pi, u32 agg_id, u16 vsi_handle,
  * already associated to the aggregator node then no operation is performed on
  * the tree. This function needs to be called with scheduler lock held.
  */
-static int
-ice_sched_assoc_vsi_to_agg(struct ice_port_info *pi, u32 agg_id,
-			   u16 vsi_handle, unsigned long *tc_bitmap)
+static int ice_sched_assoc_vsi_to_agg(struct ice_port_info *pi,
+				      struct ice_sched_agg_info *agg_info,
+				      u16 vsi_handle, unsigned long *tc_bitmap)
 {
 	struct ice_sched_agg_vsi_info *agg_vsi_info, *iter, *old_agg_vsi_info = NULL;
-	struct ice_sched_agg_info *agg_info, *old_agg_info;
+	struct ice_sched_agg_info *old_agg_info;
 	struct ice_hw *hw = pi->hw;
 	int status = 0;
 	u8 tc;
 
-	if (!ice_is_vsi_valid(pi->hw, vsi_handle))
-		return -EINVAL;
-	agg_info = xa_load(&hw->agg_nodes, agg_id);
-	if (!agg_info)
+	if (!ice_is_vsi_valid(hw, vsi_handle))
 		return -EINVAL;
 	/* If the VSI is already part of another aggregator then update
 	 * its VSI info list
@@ -2952,7 +2936,8 @@ ice_sched_assoc_vsi_to_agg(struct ice_port_info *pi, u32 agg_id,
 			continue;
 
 		/* Move VSI to new aggregator */
-		status = ice_sched_move_vsi_to_agg(pi, vsi_handle, agg_id, tc);
+		status = ice_sched_move_vsi_to_agg(pi, vsi_handle,
+						   agg_info->agg_id, tc);
 		if (status)
 			break;
 
@@ -3080,15 +3065,19 @@ int
 ice_move_vsi_to_agg(struct ice_port_info *pi, u32 agg_id, u16 vsi_handle,
 		    u8 tc_bitmap)
 {
+	struct ice_sched_agg_info *agg_info;
 	unsigned long bitmap = tc_bitmap;
 	int status;
 
 	mutex_lock(&pi->sched_lock);
-	status = ice_sched_assoc_vsi_to_agg(pi, agg_id, vsi_handle,
-					    (unsigned long *)&bitmap);
+	agg_info = xa_load(&pi->hw->agg_nodes, agg_id);
+	if (!agg_info)
+		return -EINVAL;
+	status = ice_sched_assoc_vsi_to_agg(pi, agg_info, vsi_handle,
+					    &bitmap);
 	if (!status)
-		status = ice_save_agg_vsi_tc_bitmap(pi, agg_id, vsi_handle,
-						    (unsigned long *)&bitmap);
+		status = ice_save_agg_vsi_tc_bitmap(agg_info, vsi_handle,
+						    &bitmap);
 	mutex_unlock(&pi->sched_lock);
 	return status;
 }
@@ -4420,7 +4409,7 @@ static int ice_sched_replay_vsi_agg(struct ice_hw *hw, u16 vsi_handle)
 	ice_sched_get_ena_tc_bitmap(pi, agg_vsi_info->replay_tc_bitmap,
 				    replay_bitmap);
 	/* Move this VSI (vsi_handle) to above aggregator */
-	return ice_sched_assoc_vsi_to_agg(pi, agg_info->agg_id, vsi_handle,
+	return ice_sched_assoc_vsi_to_agg(pi, agg_info, vsi_handle,
 					  replay_bitmap);
 }
 

-- 
2.56.0.rc0.395.gd1f3524e15dc

[PATCH iwl-next v2 5/6] ice: remove ice_agg_node wrapper structure

[Jacob Keller]

The ice_set_agg_vsi() function has bespoke logic for identifying a suitable
aggregator node to use for a VSI. The main goal of the function is to
ensure that various types of VSIs do not share aggregator nodes. It uses
fixed size arrays of struct ice_agg_node in the PF structure to keep track
of which aggregator nodes are currently used.

The scheduler code already keeps track of almost all of this information
via the ice_sched_agg_info structures. It doesn't make sense to store
similar information in two places. Indeed, this leaves open the
possibility that the two have conflicting data.

The recent change to refactor the aggregator info to an xarray allows
lookup via index. This enables reworking the logic in ice_set_agg_vsi() and
removal of the ice_agg_node wrapper structure.

Introduce ice_cfg_vsi_agg() and its related helper,
ice_find_available_agg(). The helper will search the range of requested IDs
and find one of the appropriate type with enough space. If no existing node
is found, then a new one will be created.

Then ice_cfg_vsi_agg() will move the VSI into that aggregator node, and
update the scheduler tree.

Neither ice_move_vsi_to_agg() nor ice_cfg_agg() made an attempt to cleanup
a previous node. The same is true for ice_cfg_vsi_agg() with one exception:
When ice_find_available_agg() allocates a new node, if scheduler tree setup
fails, the node is removed from the xarray. This matches the old logic
where the wrapper structure "valid" bit is not set unless the scheduler
succeeds. The next time that node is reused it would get reconfigured.
Removal from the xarray is safe and correct here since the code knows that
it was newly allocated and that no VSIs have yet been added to its list.

This is similar to the logic used before, but now integrated better into
the scheduler code. It also occurs all in a single critical section of the
scheduler lock, rather than being split between multiple lock/unlock
rounds.

Remove the ice_agg_node arrays and structure, and all of its related code
including the bespoke logic in ice_set_agg_vsi() as well as the associated
cleanup in ice_vsi_decfg() and ice_pf_dis_all_vsi(). The logic in
ice_pf_dis_all_vsi() is questionable anyways. Clearing the VSI count here
appears to have been intended for reconfiguring aggregator nodes after
reset. However, the ice_cfg_agg() is not called as part of a rebuild, but
only during initialization. The aggregator nodes are instead replayed via
ice_sched_replay_agg() and ice_sched_replay_vsi_agg(), which completely
ignored the wrapper structures.

Only the VF logic actually used the previously stored aggregator node data.
To handle this, store just the ID of the associated aggregator node. Update
the ice_vf_rebuild_aggregator_node_cfg() to use ice_cfg_vsi_agg() instead
of using ice_move_vsi_to_agg(). This way the node will be recreated
properly if its ever removed for any reason.

This refactor drops the arrays in the ice_pf structure, reducing its size
by 1152 bytes, or ~5% of the structures size.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h        |  23 ++----
 drivers/net/ethernet/intel/ice/ice_sched.h  |   5 ++
 drivers/net/ethernet/intel/ice/ice_lib.c    | 114 +++++-----------------------
 drivers/net/ethernet/intel/ice/ice_main.c   |   7 --
 drivers/net/ethernet/intel/ice/ice_sched.c  | 108 ++++++++++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_vf_lib.c |  22 +++---
 6 files changed, 146 insertions(+), 133 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 9e53dc3abb9a..e22188f0980a 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -447,10 +447,10 @@ struct ice_vsi {
 	u8 old_numtc;
 	u16 old_ena_tc;
 
-	/* setup back reference, to which aggregator node this VSI
-	 * corresponds to
+	/* ID of the aggregator node this VSI was configured with at setup, or
+	 * negative if it was not configured with an aggregator node.
 	 */
-	struct ice_agg_node *agg_node;
+	s64 agg_id;
 
 	struct_group_tagged(ice_vsi_cfg_params, params,
 		struct ice_port_info *port_info; /* back pointer to port_info */
@@ -539,12 +539,10 @@ struct ice_eswitch {
 	bool is_running;
 };
 
-struct ice_agg_node {
-	u32 agg_id;
-#define ICE_MAX_VSIS_IN_AGG_NODE	64
-	u32 num_vsis;
-	u8 valid;
-};
+#define ICE_PF_AGG_NODE_ID_START	1
+#define ICE_PF_AGG_NODE_ID_END		32
+#define ICE_VF_AGG_NODE_ID_START	65
+#define ICE_VF_AGG_NODE_ID_END		96
 
 struct ice_pf_msix {
 	u32 cur;
@@ -659,13 +657,6 @@ struct ice_pf {
 	struct xarray dyn_ports;
 	struct xarray sf_nums;
 
-#define ICE_INVALID_AGG_NODE_ID		0
-#define ICE_PF_AGG_NODE_ID_START	1
-#define ICE_MAX_PF_AGG_NODES		32
-	struct ice_agg_node pf_agg_node[ICE_MAX_PF_AGG_NODES];
-#define ICE_VF_AGG_NODE_ID_START	65
-#define ICE_MAX_VF_AGG_NODES		32
-	struct ice_agg_node vf_agg_node[ICE_MAX_VF_AGG_NODES];
 	struct ice_dplls dplls;
 	struct device *hwmon_dev;
 	struct ice_health health_reporters;
diff --git a/drivers/net/ethernet/intel/ice/ice_sched.h b/drivers/net/ethernet/intel/ice/ice_sched.h
index 992aafc4369d..b9ea9174f3d9 100644
--- a/drivers/net/ethernet/intel/ice/ice_sched.h
+++ b/drivers/net/ethernet/intel/ice/ice_sched.h
@@ -65,6 +65,8 @@ struct ice_sched_agg_vsi_info {
 	DECLARE_BITMAP(replay_tc_bitmap, ICE_MAX_TRAFFIC_CLASS);
 };
 
+#define ICE_MAX_VSIS_IN_AGG_NODE	64
+
 struct ice_sched_agg_info {
 	struct list_head agg_vsi_list;
 	DECLARE_BITMAP(tc_bitmap, ICE_MAX_TRAFFIC_CLASS);
@@ -134,6 +136,9 @@ int ice_rm_vsi_lan_cfg(struct ice_port_info *pi, u16 vsi_handle);
 int ice_rm_vsi_rdma_cfg(struct ice_port_info *pi, u16 vsi_handle);
 
 /* Tx scheduler rate limiter functions */
+int ice_cfg_vsi_agg(struct ice_port_info *pi, u16 vsi_handle,
+		    u32 min_id, u32 max_id, u8 tc_bitmap,
+		    u32 *configured_id);
 int
 ice_cfg_agg(struct ice_port_info *pi, u32 agg_id,
 	    enum ice_agg_type agg_type, u8 tc_bitmap);
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 0b0feb5b9919..0b7e95d984e1 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -2236,44 +2236,28 @@ void ice_cfg_sw_rx_lldp(struct ice_pf *pf, bool enable)
 static void ice_set_agg_vsi(struct ice_vsi *vsi)
 {
 	struct device *dev = ice_pf_to_dev(vsi->back);
-	struct ice_agg_node *agg_node_iter = NULL;
-	u32 agg_id = ICE_INVALID_AGG_NODE_ID;
-	struct ice_agg_node *agg_node = NULL;
-	int node_offset, max_agg_nodes = 0;
-	struct ice_port_info *port_info;
-	struct ice_pf *pf = vsi->back;
-	u32 agg_node_id_start = 0;
-	int status;
+	u32 min_id, max_id, agg_id;
+	int err;
+
+	vsi->agg_id = -1;
 
 	/* create (as needed) scheduler aggregator node and move VSI into
 	 * corresponding aggregator node
 	 * - PF aggregator node to contains VSIs of type _PF and _CTRL
 	 * - VF aggregator nodes will contain VF VSI
 	 */
-	port_info = pf->hw.port_info;
-	if (!port_info)
-		return;
-
 	switch (vsi->type) {
 	case ICE_VSI_CTRL:
 	case ICE_VSI_CHNL:
 	case ICE_VSI_LB:
 	case ICE_VSI_PF:
 	case ICE_VSI_SF:
-		max_agg_nodes = ICE_MAX_PF_AGG_NODES;
-		agg_node_id_start = ICE_PF_AGG_NODE_ID_START;
-		agg_node_iter = &pf->pf_agg_node[0];
+		min_id = ICE_PF_AGG_NODE_ID_START;
+		max_id = ICE_PF_AGG_NODE_ID_END;
 		break;
 	case ICE_VSI_VF:
-		/* user can create 'n' VFs on a given PF, but since max children
-		 * per aggregator node can be only 64. Following code handles
-		 * aggregator(s) for VF VSIs, either selects a agg_node which
-		 * was already created provided num_vsis < 64, otherwise
-		 * select next available node, which will be created
-		 */
-		max_agg_nodes = ICE_MAX_VF_AGG_NODES;
-		agg_node_id_start = ICE_VF_AGG_NODE_ID_START;
-		agg_node_iter = &pf->vf_agg_node[0];
+		min_id = ICE_VF_AGG_NODE_ID_START;
+		max_id = ICE_VF_AGG_NODE_ID_END;
 		break;
 	default:
 		/* other VSI type, handle later if needed */
@@ -2282,70 +2266,17 @@ static void ice_set_agg_vsi(struct ice_vsi *vsi)
 		return;
 	}
 
-	/* find the appropriate aggregator node */
-	for (node_offset = 0; node_offset < max_agg_nodes; node_offset++) {
-		/* see if we can find space in previously created
-		 * node if num_vsis < 64, otherwise skip
-		 */
-		if (agg_node_iter->num_vsis &&
-		    agg_node_iter->num_vsis == ICE_MAX_VSIS_IN_AGG_NODE) {
-			agg_node_iter++;
-			continue;
-		}
-
-		if (agg_node_iter->valid &&
-		    agg_node_iter->agg_id != ICE_INVALID_AGG_NODE_ID) {
-			agg_id = agg_node_iter->agg_id;
-			agg_node = agg_node_iter;
-			break;
-		}
-
-		/* find unclaimed agg_id */
-		if (agg_node_iter->agg_id == ICE_INVALID_AGG_NODE_ID) {
-			agg_id = node_offset + agg_node_id_start;
-			agg_node = agg_node_iter;
-			break;
-		}
-		/* move to next agg_node */
-		agg_node_iter++;
-	}
-
-	if (!agg_node)
-		return;
-
-	/* if selected aggregator node was not created, create it */
-	if (!agg_node->valid) {
-		status = ice_cfg_agg(port_info, agg_id, ICE_AGG_TYPE_AGG,
-				     (u8)vsi->tc_cfg.ena_tc);
-		if (status) {
-			dev_err(dev, "unable to create aggregator node with agg_id %u\n",
-				agg_id);
-			return;
-		}
-		/* aggregator node is created, store the needed info */
-		agg_node->valid = true;
-		agg_node->agg_id = agg_id;
-	}
-
-	/* move VSI to corresponding aggregator node */
-	status = ice_move_vsi_to_agg(port_info, agg_id, vsi->idx,
-				     (u8)vsi->tc_cfg.ena_tc);
-	if (status) {
-		dev_err(dev, "unable to move VSI idx %u into aggregator %u node",
-			vsi->idx, agg_id);
+	err = ice_cfg_vsi_agg(vsi->back->hw.port_info, vsi->idx, min_id,
+			      max_id, (u8)vsi->tc_cfg.ena_tc, &agg_id);
+	if (err) {
+		dev_err(dev, "Unable to associate VSI with an aggregator node, %pe\n",
+			ERR_PTR(err));
 		return;
 	}
 
-	/* keep active children count for aggregator node */
-	agg_node->num_vsis++;
-
-	/* cache the 'agg_id' in VSI, so that after reset - VSI will be moved
-	 * to aggregator node
-	 */
-	vsi->agg_node = agg_node;
-	dev_dbg(dev, "successfully moved VSI idx %u tc_bitmap 0x%x) into aggregator node %d which has num_vsis %u\n",
-		vsi->idx, vsi->tc_cfg.ena_tc, vsi->agg_node->agg_id,
-		vsi->agg_node->num_vsis);
+	vsi->agg_id = agg_id;
+	dev_dbg(dev, "successfully moved VSI idx %u tc_bitmap 0x%x into aggregator node %u\n",
+		vsi->idx, vsi->tc_cfg.ena_tc, agg_id);
 }
 
 static int ice_vsi_cfg_tc_lan(struct ice_pf *pf, struct ice_vsi *vsi)
@@ -2602,16 +2533,6 @@ void ice_vsi_decfg(struct ice_vsi *vsi)
 	ice_vsi_free_q_vectors(vsi);
 	ice_vsi_put_qs(vsi);
 	ice_vsi_free_arrays(vsi);
-
-	/* SR-IOV determines needed MSIX resources all at once instead of per
-	 * VSI since when VFs are spawned we know how many VFs there are and how
-	 * many interrupts each VF needs. SR-IOV MSIX resources are also
-	 * cleared in the same manner.
-	 */
-
-	if (vsi->type == ICE_VSI_VF &&
-	    vsi->agg_node && vsi->agg_node->valid)
-		vsi->agg_node->num_vsis--;
 }
 
 /**
@@ -2664,8 +2585,7 @@ ice_vsi_setup(struct ice_pf *pf, struct ice_vsi_cfg_params *params)
 		ice_vsi_cfg_sw_lldp(vsi, true, true);
 	}
 
-	if (!vsi->agg_node)
-		ice_set_agg_vsi(vsi);
+	ice_set_agg_vsi(vsi);
 
 	return vsi;
 
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 94c105193c7c..484f1062d320 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -578,18 +578,11 @@ static void ice_sync_fltr_subtask(struct ice_pf *pf)
  */
 static void ice_pf_dis_all_vsi(struct ice_pf *pf, bool locked)
 {
-	int node;
 	int v;
 
 	ice_for_each_vsi(pf, v)
 		if (pf->vsi[v])
 			ice_dis_vsi(pf->vsi[v], locked);
-
-	for (node = 0; node < ICE_MAX_PF_AGG_NODES; node++)
-		pf->pf_agg_node[node].num_vsis = 0;
-
-	for (node = 0; node < ICE_MAX_VF_AGG_NODES; node++)
-		pf->vf_agg_node[node].num_vsis = 0;
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c
index b1c2901b23f1..f0b1924b0411 100644
--- a/drivers/net/ethernet/intel/ice/ice_sched.c
+++ b/drivers/net/ethernet/intel/ice/ice_sched.c
@@ -3082,6 +3082,114 @@ ice_move_vsi_to_agg(struct ice_port_info *pi, u32 agg_id, u16 vsi_handle,
 	return status;
 }
 
+/**
+ * ice_find_available_agg - Find or create aggregator node
+ * @pi: port information structure
+ * @min_id: minimum aggregator node ID to use
+ * @max_id: maximum aggregator node ID to use
+ * @agg_type: The aggregator node type
+ * @tc_bitmap: TC bitmap for a newly created node
+ *
+ * Check the existing xarray of aggregator nodes. Find the first one between
+ * min_id and max_id which has fewer than ICE_MAX_VSIS_IN_AGG_NODE (64) VSIs
+ * in use. If there is no aggregator node that fits this criteria, create
+ * a new one between that range.
+ *
+ * Context: Must be called while holding the scheduler lock.
+ *
+ * Return: A pointer to the aggregator info structure, or an ERR_PTR on
+ * failure.
+ */
+static struct ice_sched_agg_info *
+ice_find_available_agg(struct ice_port_info *pi, u32 min_id, u32 max_id,
+		       enum ice_agg_type agg_type, unsigned long *tc_bitmap)
+{
+	struct ice_sched_agg_info *agg_info;
+	struct ice_hw *hw = pi->hw;
+	unsigned long agg_id;
+	int status;
+
+	xa_for_each_range(&hw->agg_nodes, agg_id, agg_info, min_id, max_id) {
+		if (agg_info->agg_type != agg_type)
+			continue;
+
+		if (agg_info->num_vsis < ICE_MAX_VSIS_IN_AGG_NODE)
+			return agg_info;
+	}
+
+	/* No aggregator node exists with space, so create a new one */
+	agg_info = ice_alloc_agg_info(hw, min_id, max_id, agg_type);
+	if (IS_ERR(agg_info))
+		return agg_info;
+
+	/* Configure the newly created aggregator node */
+	status = ice_sched_cfg_agg(pi, agg_info, tc_bitmap);
+	if (status) {
+		xa_erase(&hw->agg_nodes, agg_info->agg_id);
+		devm_kfree(ice_hw_to_dev(hw), agg_info);
+
+		return ERR_PTR(status);
+	}
+
+	/* Save the TC bitmap for this aggregator node */
+	ice_save_agg_tc_bitmap(agg_info, tc_bitmap);
+
+	return agg_info;
+}
+
+/**
+ * ice_cfg_vsi_agg - Configure a VSI to an aggregator node
+ * @pi: port information structure
+ * @vsi_handle: software VSI handle
+ * @min_id: the minimum aggregator node ID to associate with
+ * @max_id: the maximum aggregator node ID to associate with
+ * @tc_bitmap: TC bitmap of enabled TC(s)
+ * @configured_id: If non-NULL, contains the configured ID on return
+ *
+ * Locate a suitable aggregator node for this VSI, creating a new one if
+ * necessary. Configure the aggregator node if necessary, and move the VSI
+ * into it.
+ *
+ * Context: Acquires the scheduler lock.
+ *
+ * Return: zero on success, or a negative errno on failure.
+ */
+int ice_cfg_vsi_agg(struct ice_port_info *pi, u16 vsi_handle,
+		    u32 min_id, u32 max_id, u8 tc_bitmap,
+		    u32 *configured_id)
+{
+	struct ice_sched_agg_info *agg_info;
+	unsigned long bitmap = tc_bitmap;
+	int status;
+
+	mutex_lock(&pi->sched_lock);
+
+	/* Locate an aggregator node to use */
+	agg_info = ice_find_available_agg(pi, min_id, max_id, ICE_AGG_TYPE_AGG,
+					  &bitmap);
+	if (IS_ERR(agg_info)) {
+		status = PTR_ERR(agg_info);
+		goto out_unlock;
+	}
+
+	/* Associate the VSI with this aggregator node */
+	status = ice_sched_assoc_vsi_to_agg(pi, agg_info, vsi_handle, &bitmap);
+	if (status)
+		goto out_unlock;
+
+	/* Save the VSI handle to the aggregator TC bitmap */
+	status = ice_save_agg_vsi_tc_bitmap(agg_info, vsi_handle, &bitmap);
+	if (status)
+		goto out_unlock;
+
+	if (configured_id)
+		*configured_id = agg_info->agg_id;
+
+out_unlock:
+	mutex_unlock(&pi->sched_lock);
+	return status;
+}
+
 /**
  * ice_set_clear_cir_bw - set or clear CIR BW
  * @bw_t_info: bandwidth type information structure
diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
index c56a35b8dd48..a67586f7db8b 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
@@ -471,24 +471,20 @@ static void ice_vf_rebuild_aggregator_node_cfg(struct ice_vsi *vsi)
 	struct device *dev;
 	int status;
 
-	if (!vsi->agg_node)
-		return;
-
 	dev = ice_pf_to_dev(pf);
-	if (vsi->agg_node->num_vsis == ICE_MAX_VSIS_IN_AGG_NODE) {
-		dev_dbg(dev,
-			"agg_id %u already has reached max_num_vsis %u\n",
-			vsi->agg_node->agg_id, vsi->agg_node->num_vsis);
+
+	if (vsi->agg_id < 0 || vsi->agg_id > U32_MAX) {
+		dev_dbg(dev, "VSI idx %u does not have a valid aggregator node ID stored\n",
+			vsi->idx);
 		return;
 	}
 
-	status = ice_move_vsi_to_agg(pf->hw.port_info, vsi->agg_node->agg_id,
-				     vsi->idx, vsi->tc_cfg.ena_tc);
+	status = ice_cfg_vsi_agg(pf->hw.port_info, vsi->idx,
+				 (u32)vsi->agg_id, (u32)vsi->agg_id,
+				 (u8)vsi->tc_cfg.ena_tc, NULL);
 	if (status)
-		dev_dbg(dev, "unable to move VSI idx %u into aggregator %u node",
-			vsi->idx, vsi->agg_node->agg_id);
-	else
-		vsi->agg_node->num_vsis++;
+		dev_dbg(dev, "unable to move VSI idx %u into aggregator node %lld\n",
+			vsi->idx, vsi->agg_id);
 }
 
 /**

-- 
2.56.0.rc0.395.gd1f3524e15dc

[PATCH iwl-next v2 6/6] ice: remove unused aggregator node functions

[Jacob Keller]

With the introduction of ice_cfg_vsi_agg(), the ice_cfg_agg() and
ice_move_vsi_to_agg() functions are no longer used. Remove them.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_sched.h |  6 ---
 drivers/net/ethernet/intel/ice/ice_sched.c | 61 ------------------------------
 2 files changed, 67 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_sched.h b/drivers/net/ethernet/intel/ice/ice_sched.h
index b9ea9174f3d9..30b55bab09d3 100644
--- a/drivers/net/ethernet/intel/ice/ice_sched.h
+++ b/drivers/net/ethernet/intel/ice/ice_sched.h
@@ -140,12 +140,6 @@ int ice_cfg_vsi_agg(struct ice_port_info *pi, u16 vsi_handle,
 		    u32 min_id, u32 max_id, u8 tc_bitmap,
 		    u32 *configured_id);
 int
-ice_cfg_agg(struct ice_port_info *pi, u32 agg_id,
-	    enum ice_agg_type agg_type, u8 tc_bitmap);
-int
-ice_move_vsi_to_agg(struct ice_port_info *pi, u32 agg_id, u16 vsi_handle,
-		    u8 tc_bitmap);
-int
 ice_cfg_q_bw_lmt(struct ice_port_info *pi, u16 vsi_handle, u8 tc,
 		 u16 q_handle, enum ice_rl_type rl_type, u32 bw);
 int
diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c
index f0b1924b0411..fbee7863394a 100644
--- a/drivers/net/ethernet/intel/ice/ice_sched.c
+++ b/drivers/net/ethernet/intel/ice/ice_sched.c
@@ -2777,37 +2777,6 @@ static int ice_sched_cfg_agg(struct ice_port_info *pi,
 	return 0;
 }
 
-/**
- * ice_cfg_agg - config aggregator node
- * @pi: port information structure
- * @agg_id: aggregator ID
- * @agg_type: aggregator type queue, VSI, or aggregator group
- * @tc_bitmap: bits TC bitmap
- *
- * This function configures aggregator node(s).
- */
-int
-ice_cfg_agg(struct ice_port_info *pi, u32 agg_id, enum ice_agg_type agg_type,
-	    u8 tc_bitmap)
-{
-	struct ice_sched_agg_info *agg_info;
-	unsigned long bitmap = tc_bitmap;
-	struct ice_hw *hw = pi->hw;
-	int status;
-
-	mutex_lock(&pi->sched_lock);
-	agg_info = xa_load(&hw->agg_nodes, agg_id);
-	if (!agg_info)
-		agg_info = ice_alloc_agg_info(hw, agg_id, agg_id, agg_type);
-	if (IS_ERR(agg_info))
-		return PTR_ERR(agg_info);
-	status = ice_sched_cfg_agg(pi, agg_info, &bitmap);
-	if (!status)
-		ice_save_agg_tc_bitmap(agg_info, &bitmap);
-	mutex_unlock(&pi->sched_lock);
-	return status;
-}
-
 /**
  * ice_get_agg_vsi_info - get the aggregator ID
  * @agg_info: aggregator info
@@ -3052,36 +3021,6 @@ ice_sched_cfg_node_bw_alloc(struct ice_hw *hw, struct ice_sched_node *node,
 	return ice_sched_update_elem(hw, node, &buf);
 }
 
-/**
- * ice_move_vsi_to_agg - moves VSI to new or default aggregator
- * @pi: port information structure
- * @agg_id: aggregator ID
- * @vsi_handle: software VSI handle
- * @tc_bitmap: TC bitmap of enabled TC(s)
- *
- * Move or associate VSI to a new or default aggregator node.
- */
-int
-ice_move_vsi_to_agg(struct ice_port_info *pi, u32 agg_id, u16 vsi_handle,
-		    u8 tc_bitmap)
-{
-	struct ice_sched_agg_info *agg_info;
-	unsigned long bitmap = tc_bitmap;
-	int status;
-
-	mutex_lock(&pi->sched_lock);
-	agg_info = xa_load(&pi->hw->agg_nodes, agg_id);
-	if (!agg_info)
-		return -EINVAL;
-	status = ice_sched_assoc_vsi_to_agg(pi, agg_info, vsi_handle,
-					    &bitmap);
-	if (!status)
-		status = ice_save_agg_vsi_tc_bitmap(agg_info, vsi_handle,
-						    &bitmap);
-	mutex_unlock(&pi->sched_lock);
-	return status;
-}
-
 /**
  * ice_find_available_agg - Find or create aggregator node
  * @pi: port information structure

-- 
2.56.0.rc0.395.gd1f3524e15dc

Re: [PATCH iwl-next v2 5/6] ice: remove ice_agg_node wrapper structure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ice: VF rebuild fails to re-associate VSI with fully populated aggregator node
--

commit aed63df873bf36bec0897ed1209a7e411aadbd13
Author: Jacob Keller <jacob.e.keller@intel.com>

    ice: remove ice_agg_node wrapper structure

The commit removes the ice_agg_node wrapper structure and replaces it with
existing scheduler aggregator info. It introduces ice_cfg_vsi_agg() and
uses it to look up and configure VSIs during initialization and rebuilds.

> diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c
> index b1c2901b23f12..f0b1924b0411f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sched.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sched.c
[ ... ]
> @@ -3082,6 +3082,114 @@ ice_move_vsi_to_agg(struct ice_port_info *pi, u32 agg_id, u16 vsi_handle,
>  	return status;
>  }
>  
> +/**
> + * ice_find_available_agg - Find or create aggregator node
> + * @pi: port information structure
> + * @min_id: minimum aggregator node ID to use
> + * @max_id: maximum aggregator node ID to use
> + * @agg_type: The aggregator node type
> + * @tc_bitmap: TC bitmap for a newly created node
> + *
> + * Check the existing xarray of aggregator nodes. Find the first one between
> + * min_id and max_id which has fewer than ICE_MAX_VSIS_IN_AGG_NODE (64) VSIs
> + * in use. If there is no aggregator node that fits this criteria, create
> + * a new one between that range.
> + *
> + * Context: Must be called while holding the scheduler lock.
> + *
> + * Return: A pointer to the aggregator info structure, or an ERR_PTR on
> + * failure.
> + */
> +static struct ice_sched_agg_info *
> +ice_find_available_agg(struct ice_port_info *pi, u32 min_id, u32 max_id,
> +		       enum ice_agg_type agg_type, unsigned long *tc_bitmap)
> +{
> +	struct ice_sched_agg_info *agg_info;
> +	struct ice_hw *hw = pi->hw;
> +	unsigned long agg_id;
> +	int status;
> +
> +	xa_for_each_range(&hw->agg_nodes, agg_id, agg_info, min_id, max_id) {
> +		if (agg_info->agg_type != agg_type)
> +			continue;
> +
> +		if (agg_info->num_vsis < ICE_MAX_VSIS_IN_AGG_NODE)
> +			return agg_info;

[Severity: High]
Does this check inadvertently reject fully populated aggregators when the caller
is explicitly requesting an existing node during a rebuild?

If ice_vf_rebuild_aggregator_node_cfg() attempts to re-associate the VF 
with an existing perfectly full aggregator (64 VSIs), it seems this check 
evaluates to false because the SW tree structures were preserved across the 
reset and the VF is still accounted for in num_vsis.

> +	}
> +
> +	/* No aggregator node exists with space, so create a new one */
> +	agg_info = ice_alloc_agg_info(hw, min_id, max_id, agg_type);

Since ice_find_available_agg() returns here if no space was found, won't 
this allocation attempt to create a new node with the exact same ID (because
min_id and max_id are both vsi->agg_id), causing xa_alloc() to fail?

> +	if (IS_ERR(agg_info))
> +		return agg_info;
[ ... ]
> diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
> index c56a35b8dd48f..a67586f7db8be 100644
> --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
> @@ -471,24 +471,20 @@ static void ice_vf_rebuild_aggregator_node_cfg(struct ice_vsi *vsi)
>  	struct device *dev;
>  	int status;
>  
> -	if (!vsi->agg_node)
> -		return;
> -
>  	dev = ice_pf_to_dev(pf);
> -	if (vsi->agg_node->num_vsis == ICE_MAX_VSIS_IN_AGG_NODE) {
> -		dev_dbg(dev,
> -			"agg_id %u already has reached max_num_vsis %u\n",
> -			vsi->agg_node->agg_id, vsi->agg_node->num_vsis);
> +
> +	if (vsi->agg_id < 0 || vsi->agg_id > U32_MAX) {
> +		dev_dbg(dev, "VSI idx %u does not have a valid aggregator node ID stored\n",
> +			vsi->idx);
>  		return;
>  	}
>  
> -	status = ice_move_vsi_to_agg(pf->hw.port_info, vsi->agg_node->agg_id,
> -				     vsi->idx, vsi->tc_cfg.ena_tc);
> +	status = ice_cfg_vsi_agg(pf->hw.port_info, vsi->idx,
> +				 (u32)vsi->agg_id, (u32)vsi->agg_id,
> +				 (u8)vsi->tc_cfg.ena_tc, NULL);

If ice_cfg_vsi_agg() fails when the original node is fully populated, does 
this break QoS and bandwidth isolation for any VF assigned to that full
aggregator? The previous logic specifically checked for and gracefully handled
reaching ICE_MAX_VSIS_IN_AGG_NODE.

>  	if (status)
> -		dev_dbg(dev, "unable to move VSI idx %u into aggregator %u node",
> -			vsi->idx, vsi->agg_node->agg_id);
> -	else
> -		vsi->agg_node->num_vsis++;
> +		dev_dbg(dev, "unable to move VSI idx %u into aggregator node %lld\n",
> +			vsi->idx, vsi->agg_id);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-jk-cleanup-vsi-aggregator-nodes-v2-0-313964c25c7b@intel.com?part=5

Re: [PATCH iwl-next v2 2/6] ice: count number of VSIS in agg_vsi_list

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ice: num_vsis inflated and stale entry left on VSI move failure
--

commit a9d16d503219ec18b5a73776a1a7f4b0b851cd1b
Author: Jacob Keller <jacob.e.keller@intel.com>

ice: count number of VSIS in agg_vsi_list

This patch adds a member num_vsis to the ice_sched_agg_info structure to track
the number of VSIs associated with an aggregator node. The counter is
incremented when adding a VSI to the list and decremented when removing it,
avoiding the need to iterate over the agg_vsi_list.

> diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c
> index bcfc9db69e17e..08d0f685f6c45 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sched.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sched.c

[ ... ]

> @@ -2915,24 +2920,26 @@ ice_sched_assoc_vsi_to_agg(struct ice_port_info *pi, u32 agg_id,
>  		/* add VSI ID into the aggregator list */
>  		agg_vsi_info->vsi_handle = vsi_handle;
>  		list_add(&agg_vsi_info->list_entry, &agg_info->agg_vsi_list);
> +		agg_info->num_vsis++;
>  	}
>  	/* Move VSI node to new aggregator node for requested TC(s) */
>  	ice_for_each_traffic_class(tc) {
>  		if (!ice_is_tc_ena(*tc_bitmap, tc))
>  			continue;
>  
>  		/* Move VSI to new aggregator */
>  		status = ice_sched_move_vsi_to_agg(pi, vsi_handle, agg_id, tc);
>  		if (status)
>  			break;

[Severity: Medium]
If ice_sched_move_vsi_to_agg() fails and returns an error here, the loop will
break. Should the function roll back the agg_info->num_vsis increment and remove
the newly added agg_vsi_info from agg_info->agg_vsi_list before returning?

Without a rollback, a stale VSI entry with an empty tc_bitmap remains in the
aggregator list, permanently inflating the num_vsis counter. If subsequent code
relies on num_vsis to enforce limits like ICE_MAX_VSIS_IN_AGG_NODE, this could
cause the aggregator to prematurely reject new VSIs.

>  
>  		set_bit(tc, agg_vsi_info->tc_bitmap);
>  		if (old_agg_vsi_info)
>  			clear_bit(tc, old_agg_vsi_info->tc_bitmap);
>  	}
>  	if (old_agg_vsi_info && !old_agg_vsi_info->tc_bitmap[0]) {
>  		list_del(&old_agg_vsi_info->list_entry);
>  		devm_kfree(ice_hw_to_dev(pi->hw), old_agg_vsi_info);
> +		old_agg_info->num_vsis--;
>  	}
>  	return status;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-jk-cleanup-vsi-aggregator-nodes-v2-0-313964c25c7b@intel.com?part=2

Re: [PATCH iwl-next v2 5/6] ice: remove ice_agg_node wrapper structure

[Jacob Keller]

On 10/9/2026 1:26 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] ice: VF rebuild fails to re-associate VSI with fully populated aggregator node
> --
> 
> commit aed63df873bf36bec0897ed1209a7e411aadbd13
> Author: Jacob Keller <jacob.e.keller@intel.com>
> 
>     ice: remove ice_agg_node wrapper structure
> 
> The commit removes the ice_agg_node wrapper structure and replaces it with
> existing scheduler aggregator info. It introduces ice_cfg_vsi_agg() and
> uses it to look up and configure VSIs during initialization and rebuilds.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c
>> index b1c2901b23f12..f0b1924b0411f 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_sched.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_sched.c
> [ ... ]
>> @@ -3082,6 +3082,114 @@ ice_move_vsi_to_agg(struct ice_port_info *pi, u32 agg_id, u16 vsi_handle,
>>  	return status;
>>  }
>>  
>> +/**
>> + * ice_find_available_agg - Find or create aggregator node
>> + * @pi: port information structure
>> + * @min_id: minimum aggregator node ID to use
>> + * @max_id: maximum aggregator node ID to use
>> + * @agg_type: The aggregator node type
>> + * @tc_bitmap: TC bitmap for a newly created node
>> + *
>> + * Check the existing xarray of aggregator nodes. Find the first one between
>> + * min_id and max_id which has fewer than ICE_MAX_VSIS_IN_AGG_NODE (64) VSIs
>> + * in use. If there is no aggregator node that fits this criteria, create
>> + * a new one between that range.
>> + *
>> + * Context: Must be called while holding the scheduler lock.
>> + *
>> + * Return: A pointer to the aggregator info structure, or an ERR_PTR on
>> + * failure.
>> + */
>> +static struct ice_sched_agg_info *
>> +ice_find_available_agg(struct ice_port_info *pi, u32 min_id, u32 max_id,
>> +		       enum ice_agg_type agg_type, unsigned long *tc_bitmap)
>> +{
>> +	struct ice_sched_agg_info *agg_info;
>> +	struct ice_hw *hw = pi->hw;
>> +	unsigned long agg_id;
>> +	int status;
>> +
>> +	xa_for_each_range(&hw->agg_nodes, agg_id, agg_info, min_id, max_id) {
>> +		if (agg_info->agg_type != agg_type)
>> +			continue;
>> +
>> +		if (agg_info->num_vsis < ICE_MAX_VSIS_IN_AGG_NODE)
>> +			return agg_info;
> 
> [Severity: High]
> Does this check inadvertently reject fully populated aggregators when the caller
> is explicitly requesting an existing node during a rebuild?
> 
> If ice_vf_rebuild_aggregator_node_cfg() attempts to re-associate the VF 
> with an existing perfectly full aggregator (64 VSIs), it seems this check 
> evaluates to false because the SW tree structures were preserved across the 
> reset and the VF is still accounted for in num_vsis.
> 

Two reasons this is not valid:

a) we only call this path on creating a new VSI

b) we already remove aggregator nodes through ice_vsi_decfg()

I believe we will need to investigate the first path and likely need to
recreate aggregator node config after rebuild, but I haven't actually
tracked down what the consequence of the existing logic is yet.

The existing "replay" logic replays any remaining aggregator node config
without going through this "init" path so either way this path is not
supposed to be called in that way.

>> +	}
>> +
>> +	/* No aggregator node exists with space, so create a new one */
>> +	agg_info = ice_alloc_agg_info(hw, min_id, max_id, agg_type);
> 
> Since ice_find_available_agg() returns here if no space was found, won't 
> this allocation attempt to create a new node with the exact same ID (because
> min_id and max_id are both vsi->agg_id), causing xa_alloc() to fail?
> 

it would, but only if its given a single ID, (as in the VF rebuild
case). But in that case, the VF was removed before and is re-added now.

>> +	if (IS_ERR(agg_info))
>> +		return agg_info;
> [ ... ]
>> diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
>> index c56a35b8dd48f..a67586f7db8be 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
>> @@ -471,24 +471,20 @@ static void ice_vf_rebuild_aggregator_node_cfg(struct ice_vsi *vsi)
>>  	struct device *dev;
>>  	int status;
>>  
>> -	if (!vsi->agg_node)
>> -		return;
>> -
>>  	dev = ice_pf_to_dev(pf);
>> -	if (vsi->agg_node->num_vsis == ICE_MAX_VSIS_IN_AGG_NODE) {
>> -		dev_dbg(dev,
>> -			"agg_id %u already has reached max_num_vsis %u\n",
>> -			vsi->agg_node->agg_id, vsi->agg_node->num_vsis);
>> +
>> +	if (vsi->agg_id < 0 || vsi->agg_id > U32_MAX) {
>> +		dev_dbg(dev, "VSI idx %u does not have a valid aggregator node ID stored\n",
>> +			vsi->idx);
>>  		return;
>>  	}
>>  
>> -	status = ice_move_vsi_to_agg(pf->hw.port_info, vsi->agg_node->agg_id,
>> -				     vsi->idx, vsi->tc_cfg.ena_tc);
>> +	status = ice_cfg_vsi_agg(pf->hw.port_info, vsi->idx,
>> +				 (u32)vsi->agg_id, (u32)vsi->agg_id,
>> +				 (u8)vsi->tc_cfg.ena_tc, NULL);
> 
> If ice_cfg_vsi_agg() fails when the original node is fully populated, does 
> this break QoS and bandwidth isolation for any VF assigned to that full
> aggregator? The previous logic specifically checked for and gracefully handled
> reaching ICE_MAX_VSIS_IN_AGG_NODE.
> 

This shouldn't be reachable. I'll try to investigate how that was
handled before. I don't think it actually *did* gracefully handle this
scenario.

>>  	if (status)
>> -		dev_dbg(dev, "unable to move VSI idx %u into aggregator %u node",
>> -			vsi->idx, vsi->agg_node->agg_id);
>> -	else
>> -		vsi->agg_node->num_vsis++;
>> +		dev_dbg(dev, "unable to move VSI idx %u into aggregator node %lld\n",
>> +			vsi->idx, vsi->agg_id);
>>  }
> 

Re: [PATCH iwl-next v2 2/6] ice: count number of VSIS in agg_vsi_list

[Jacob Keller]

On 10/9/2026 1:27 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] ice: num_vsis inflated and stale entry left on VSI move failure
> --
> 
> commit a9d16d503219ec18b5a73776a1a7f4b0b851cd1b
> Author: Jacob Keller <jacob.e.keller@intel.com>
> 
> ice: count number of VSIS in agg_vsi_list
> 
> This patch adds a member num_vsis to the ice_sched_agg_info structure to track
> the number of VSIs associated with an aggregator node. The counter is
> incremented when adding a VSI to the list and decremented when removing it,
> avoiding the need to iterate over the agg_vsi_list.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c
>> index bcfc9db69e17e..08d0f685f6c45 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_sched.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_sched.c
> 
> [ ... ]
> 
>> @@ -2915,24 +2920,26 @@ ice_sched_assoc_vsi_to_agg(struct ice_port_info *pi, u32 agg_id,
>>  		/* add VSI ID into the aggregator list */
>>  		agg_vsi_info->vsi_handle = vsi_handle;
>>  		list_add(&agg_vsi_info->list_entry, &agg_info->agg_vsi_list);
>> +		agg_info->num_vsis++;
>>  	}
>>  	/* Move VSI node to new aggregator node for requested TC(s) */
>>  	ice_for_each_traffic_class(tc) {
>>  		if (!ice_is_tc_ena(*tc_bitmap, tc))
>>  			continue;
>>  
>>  		/* Move VSI to new aggregator */
>>  		status = ice_sched_move_vsi_to_agg(pi, vsi_handle, agg_id, tc);
>>  		if (status)
>>  			break;
> 
> [Severity: Medium]
> If ice_sched_move_vsi_to_agg() fails and returns an error here, the loop will
> break. Should the function roll back the agg_info->num_vsis increment and remove
> the newly added agg_vsi_info from agg_info->agg_vsi_list before returning?
> 
> Without a rollback, a stale VSI entry with an empty tc_bitmap remains in the
> aggregator list, permanently inflating the num_vsis counter. If subsequent code
> relies on num_vsis to enforce limits like ICE_MAX_VSIS_IN_AGG_NODE, this could
> cause the aggregator to prematurely reject new VSIs.
> 

I'll have to check this but cleaning up seems reasonable if it can be
done properly.

Honestly the entire logic here feels like the data types and
abstractions are not very good.. but its been difficult to try and take
incremental steps forward on fixing it :\

>>  
>>  		set_bit(tc, agg_vsi_info->tc_bitmap);
>>  		if (old_agg_vsi_info)
>>  			clear_bit(tc, old_agg_vsi_info->tc_bitmap);
>>  	}
>>  	if (old_agg_vsi_info && !old_agg_vsi_info->tc_bitmap[0]) {
>>  		list_del(&old_agg_vsi_info->list_entry);
>>  		devm_kfree(ice_hw_to_dev(pi->hw), old_agg_vsi_info);
>> +		old_agg_info->num_vsis--;
>>  	}
>>  	return status;
>>  }
>