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() in ice_sched.c. This function will locate a suitable aggregator node between the provided minimum and maximum ID. If no node exists, it will allocate a new one. It will then configure the node and move the VSI into the node immediately. We do not free a newly allocated node if the device fails to configure the scheduler. The node will continue to exist within the aggregator node xarray list, and be reused in the future when another VSI is configured. 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, since it reset the num_vsis count without clearing the valid flag or the agg_node pointers in the VSI structures. With the refactor, the VSI count for each aggregator node now always matches with what the scheduler actually has configured. Only the VF logic actually uses the previously stored aggregator node data. To avoid use-after-free issues, don't store a pointer to the aggregator node. Instead, store just the ID of the associated node. Update ice_vf_rebuild_aggregator_node_cfg() to use ice_cfg_vsi_agg() instead of using ice_move_vsi_to_agg(). This ensures that the node will be created if it was ever removed for any reason. Store the agg_id in the VSI structure using a signed 64-bit value to allow storing -1 in the case where no aggregator node was configured. 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 <[email protected]> Reviewed-by: Aleksandr Loktionov <[email protected]> --- 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 | 102 +++++++++++++++++++++++++ drivers/net/ethernet/intel/ice/ice_vf_lib.c | 22 +++--- 6 files changed, 140 insertions(+), 133 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h index f9a43daf04fe..e5ccbf931502 100644 --- a/drivers/net/ethernet/intel/ice/ice.h +++ b/drivers/net/ethernet/intel/ice/ice.h @@ -448,10 +448,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 */ @@ -541,12 +541,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; @@ -660,13 +658,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 bf2ceaa49dec..f285e63cd641 100644 --- a/drivers/net/ethernet/intel/ice/ice_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_lib.c @@ -2172,44 +2172,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 */ @@ -2218,70 +2202,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) @@ -2538,16 +2469,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--; } /** @@ -2600,8 +2521,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 c79125dca56b..21b4904aa541 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -512,18 +512,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 88392ebf4454..a7f31e84ee14 100644 --- a/drivers/net/ethernet/intel/ice/ice_sched.c +++ b/drivers/net/ethernet/intel/ice/ice_sched.c @@ -3099,6 +3099,108 @@ 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 + * @hw: pointer to the HW structure + * @min_id: minimum aggregator node ID to use + * @max_id: maximum aggregator node ID to use + * @agg_type: The aggregator node type + * + * 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_hw *hw, u32 min_id, u32 max_id, + enum ice_agg_type agg_type) +{ + struct ice_sched_agg_info *agg_info; + unsigned long agg_id; + + xa_for_each_range(&hw->agg_list, 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 */ + return ice_alloc_agg_info(hw, min_id, max_id, agg_type); +} + +/** + * 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; + struct ice_hw *hw = pi->hw; + int status; + + mutex_lock(&pi->sched_lock); + + /* Locate an aggregator node to use */ + agg_info = ice_find_available_agg(hw, min_id, max_id, ICE_AGG_TYPE_AGG); + if (IS_ERR(agg_info)) { + status = PTR_ERR(agg_info); + goto out_unlock; + } + + /* Configure the aggregator node */ + status = ice_sched_cfg_agg(pi, agg_info->agg_id, ICE_AGG_TYPE_AGG, + &bitmap); + if (status) + goto out_unlock; + + /* Save the TC bitmap for this aggregator node */ + status = ice_save_agg_tc_bitmap(pi, agg_info->agg_id, &bitmap); + if (status) + goto out_unlock; + + /* Associate the VSI with this aggregator node */ + status = ice_sched_assoc_vsi_to_agg(pi, agg_info->agg_id, vsi_handle, + &bitmap); + if (status) + goto out_unlock; + + /* Save the VSI handle to the aggregator TC bitmap */ + status = ice_save_agg_vsi_tc_bitmap(pi, agg_info->agg_id, 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 47a30b255007..6b5c9be3a198 100644 --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c @@ -467,24 +467,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.54.0.1064.gd145956f57df
