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

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

[Jacob Keller]

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.

 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

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

[Simon Horman]

+ 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
> 

RE: [Intel-wired-lan] [PATCH iwl-net] ice: stop trashing VF VSI aggregator node ID information

[Romanowski, Rafal]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Simon Horman
> Sent: Sunday, December 10, 2023 12:45 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: ivecera <ivecera@redhat.com>; netdev@vger.kernel.org; Nguyen, Anthony
> L <anthony.l.nguyen@intel.com>; Intel Wired LAN <intel-wired-
> lan@lists.osuosl.org>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH iwl-net] ice: stop trashing VF VSI
> aggregator node ID information
> 
> + 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

Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>