> -----Original Message----- > From: Intel-wired-lan <[email protected]> On Behalf Of > Simon Horman > Sent: Sunday, December 10, 2023 12:45 PM > To: Keller, Jacob E <[email protected]> > Cc: ivecera <[email protected]>; [email protected]; Nguyen, Anthony > L <[email protected]>; Intel Wired LAN <intel-wired- > [email protected]>; Kitszel, Przemyslaw <[email protected]> > Subject: Re: [Intel-wired-lan] [PATCH iwl-net] ice: stop trashing VF VSI > aggregator node ID information > > + Ivan Vecera <[email protected]> > > 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 <[email protected]> > > Reviewed-by: Przemek Kitszel <[email protected]> > > --- > > 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 <[email protected]> > > 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 <[email protected]> _______________________________________________ Intel-wired-lan mailing list [email protected] https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
