RE: [PATCH net 2/3] i40e: Fix not setting xps_cpus after reset

-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: Wednesday, September 28, 2022 3:30 AM
To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
Cc: davem@davemloft.net; pabeni@redhat.com; edumazet@google.com;
Jaron, MichalX [off-list ref]; netdev@vger.kernel.org;
Maziarz, Kamil [off-list ref]; G, GurucharanX
[off-list ref]
Subject: Re: [PATCH net 2/3] i40e: Fix not setting xps_cpus after reset

On Mon, 26 Sep 2022 13:32:13 -0700 Tony Nguyen wrote:

quoted

During tx rings configuration default XPS queue config is set and
__I40E_TX_XPS_INIT_DONE is locked. XPS CPUs maps are cleared in every
reset by netdev_set_num_tc() call regardless it was set by user or
driver. If reset with reinit occurs __I40E_TX_XPS_INIT_DONE flag is
removed and XPS mapping is set to default again but after reset
without reinit this flag is still set and XPS CPUs to queues mapping
stays cleared.

Add code to preserve xps_cpus mapping as cpumask for every queue and
restore those mapping at the end of reset.

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