+ Ivan Vecera <ivec...@redhat.com>
On Wed, Dec 06, 2023 at 12:19:05PM -0800, Jacob Keller wrote:
> When creating new VSIs, they are assigned into an aggregator node in the
> scheduler tree. Information about which aggregator node a VSI is assigned
> into is maintained by the vsi->agg_node structure. In ice_vsi_decfg(), this
> information is being destroyed, by overwriting the valid flag and the
> agg_id field to zero.
>
> For VF VSIs, this breaks the aggregator node configuration replay, which
> depends on this information. This results in VFs being inserted into the
> default aggregator node. The resulting configuration will have unexpected
> Tx bandwidth sharing behavior.
>
> This was broken by commit 6624e780a577 ("ice: split ice_vsi_setup into
> smaller functions"), which added the block to reset the agg_node data.
>
> The vsi->agg_node structure is not managed by the scheduler code, but is
> instead a wrapper around an aggregator node ID that is tracked at the VSI
> layer. Its been around for a long time, and its primary purpose was for
> handling VFs. The SR-IOV VF reset flow does not make use of the standard VSI
> rebuild/replay logic, and uses vsi->agg_node as part of its handling to
> rebuild the aggregator node configuration.
>
> The logic for aggregator nodes stretches back to early ice driver code from
> commit b126bd6bcd67 ("ice: create scheduler aggregator node config and move
> VSIs")
>
> The logic in ice_vsi_decfg() which trashes the ice_agg_node data is clearly
> wrong. It destroys information that is necessary for handling VF reset,. It
> is also not the correct way to actually remove a VSI from an aggregator
> node. For that, we need to implement logic in the scheduler code. Further,
> non-VF VSIs properly replay their aggregator configuration using existing
> scheduler replay logic.
>
> To fix the VF replay logic, remove this broken aggregator node cleanup
> logic. This is the simplest way to immediately fix this.
>
> This ensures that VFs will have proper aggregate configuration after a
> reset. This is especially important since VFs often perform resets as part
> of their reconfiguration flows. Without fixing this, VFs will be placed in
> the default aggregator node and Tx bandwidth will not be shared in the
> expected and configured manner.
>
> Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions")
> Signed-off-by: Jacob Keller <jacob.e.kel...@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kits...@intel.com>
> ---
> This is the simplest fix to resolve the aggregator node problem. However, I
> think we should clean this up properly. I don't know why the VF VSIs have
> their own custom code for replaying aggregator configuration. I also think
> its odd that there is both structures to track aggregator information in
> ice_sched.c, but we use a separate structure in ice.h for the ice_vsi
> structure. I plan to investigate this and clean it up in next. However, I
> wanted to get a smaller fix out to net sooner rather than later.
Less is more, for now :)
Reviewed-by: Simon Horman <ho...@kernel.org>
I've added Ivan to the CC list in case he wants to review this too.
>
> drivers/net/ethernet/intel/ice/ice_lib.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c
> b/drivers/net/ethernet/intel/ice/ice_lib.c
> index 4b1e56396293..de7ba87af45d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -2620,10 +2620,6 @@ void ice_vsi_decfg(struct ice_vsi *vsi)
> if (vsi->type == ICE_VSI_VF &&
> vsi->agg_node && vsi->agg_node->valid)
> vsi->agg_node->num_vsis--;
> - if (vsi->agg_node) {
> - vsi->agg_node->valid = false;
> - vsi->agg_node->agg_id = 0;
> - }
> }
>
> /**
> --
> 2.41.0
>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
