lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <BL0PR11MB3521FE2860D9AABE3A4312D48F8CA@BL0PR11MB3521.namprd11.prod.outlook.com> Date: Thu, 14 Dec 2023 09:55:22 +0000 From: "Romanowski, Rafal" <rafal.romanowski@...el.com> To: Simon Horman <horms@...nel.org>, "Keller, Jacob E" <jacob.e.keller@...el.com> CC: ivecera <ivecera@...hat.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, Intel Wired LAN <intel-wired-lan@...ts.osuosl.org>, "Kitszel, Przemyslaw" <przemyslaw.kitszel@...el.com> Subject: RE: [Intel-wired-lan] [PATCH iwl-net] ice: stop trashing VF VSI aggregator node ID information > -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@...osl.org> On Behalf Of > Simon Horman > Sent: Sunday, December 10, 2023 12:45 PM > To: Keller, Jacob E <jacob.e.keller@...el.com> > Cc: ivecera <ivecera@...hat.com>; netdev@...r.kernel.org; Nguyen, Anthony > L <anthony.l.nguyen@...el.com>; Intel Wired LAN <intel-wired- > lan@...ts.osuosl.org>; Kitszel, Przemyslaw <przemyslaw.kitszel@...el.com> > Subject: Re: [Intel-wired-lan] [PATCH iwl-net] ice: stop trashing VF VSI > aggregator node ID information > > + Ivan Vecera <ivecera@...hat.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@...el.com> > > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.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@...nel.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@...el.com>
Powered by blists - more mailing lists