Re: [PATCH iwl-net] ice: stop trashing VF VSI aggregator node ID information

[Simon Horman <horms@kernel.org>]

+ Ivan Vecera <ivecera@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.keller@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@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 <horms@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
>