netdev - RE: [PATCH net 2/3] i40e: Fix not setting xps

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 29 Sep 2022 11:58:52 +0000
From:   "Jaron, MichalX" <michalx.jaron@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Maziarz, Kamil" <kamil.maziarz@...el.com>,
        "G, GurucharanX" <gurucharanx.g@...el.com>,
        "Dziedziuch, SylwesterX" <sylwesterx.dziedziuch@...el.com>,
        "Brandeburg, Jesse" <jesse.brandeburg@...el.com>
Subject: RE: [PATCH net 2/3] i40e: Fix not setting xps_cpus after reset



> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Wednesday, September 28, 2022 4:11 PM
> To: Jaron, MichalX <michalx.jaron@...el.com>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@...el.com>;
> davem@...emloft.net; pabeni@...hat.com; edumazet@...gle.com;
> netdev@...r.kernel.org; Maziarz, Kamil <kamil.maziarz@...el.com>; G,
> GurucharanX <gurucharanx.g@...el.com>; Dziedziuch, SylwesterX
> <sylwesterx.dziedziuch@...el.com>; Brandeburg, Jesse
> <jesse.brandeburg@...el.com>
> Subject: Re: [PATCH net 2/3] i40e: Fix not setting xps_cpus after reset
> 
> On Wed, 28 Sep 2022 13:32:41 +0000 Jaron, MichalX wrote:
> > > Not sure this is a fix, are there other drivers in the tree which do
> > > this? In the drivers I work with IRQ mapping and XPS are just
> > > seemingly randomly reset on reconfiguration changes. User space
> > > needs to rerun its affinitization script after all changes it makes.
> > >
> > > Apart from the fact that I don't think this is a fix, if we were to
> > > solve it we should shoot for a more generic solution and not
> > > sprinkle all drivers with #ifdef CONFIG_XPS blocks :S
> >
> > XPS to CPUs maps are configured by i40e driver, based on active cpus,
> > after initialization or after drivers reset with reinit (i.e. when
> > queues count changes). User may want to leave this mapping or set his
> > own mapping by writing to xps_cpus file. In case when we do reset on
> > our network interface without changing number of queues(when reinit is
> > not true), i.e. by calling ethtool -t <interface>, in
> > i40e_rebuild() those maps were cleared (set to 0) for every tx by
> > netdev_set_num_tc(). After reset those maps were still set to 0
> > despite that it was set by driver or by user and user was not informed
> > about it.
> 
> Set to 0 or reset to default (which I would hope is spread across the CPUs in
> the same fashion as affinity hint)?
> 

Current driver behavior is that maps are cleared(set to 0) after every reset. Then they are reinitialized to default values when driver rebuild queues during reset i.e. the number of queues changed, number of VFs changed, XDP is turning on/off(we reset and rebuild rings) or fw lldp agent is turning on/off. Reinitialization is done by netif_set_xps_queue() from XPS API. In every other case of reset maps will remain cleared.

With this fix, when there is a reset without rebuilding queues, maps are restored to the same values as before reset.
I changed commit message a bit to be more descriptive and changed one goto; as it was not correct. New version should be sent already to review.

> > With this fix maps are preserved and restored after reset to not
> > surprise user that maps have changed when user doesn't want it.
> > Mapping restoration is based on CPUs mapping and is done by
> > netif_set_xps_queue() which is XPS function, then I think this
> > affinization should be performed well.
> >
> > If user doesn't want to change queues then those maps should be
> > restored to the way it was.