[PATCH net 0/3][pull request] Intel Wired LAN Driver Updates 2022-09-26 (i40e)

[PATCH net 0/3][pull request] Intel Wired LAN Driver Updates 2022-09-26 (i40e)

[Tony Nguyen]

This series contains updates to i40e driver only.

Slawomir fixes using incorrect register masks on X722 devices.

Michal saves then restores XPS settings after reset.

Jan fixes memory leak of DMA memory due to incorrect freeing of rx_buf.

The following are changes since commit b7ca8d5f56e6c57b671ceb8de1361d2a5a495087:
  sfc: correct filter_table_remove method for EF10 PFs
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 40GbE

Jan Sokolowski (1):
  i40e: Fix DMA mappings leak

Michal Jaron (1):
  i40e: Fix not setting xps_cpus after reset

Slawomir Laba (1):
  i40e: Fix ethtool rx-flow-hash setting for X722

 drivers/net/ethernet/intel/i40e/i40e.h        |   6 +
 .../net/ethernet/intel/i40e/i40e_ethtool.c    |  34 +++--
 drivers/net/ethernet/intel/i40e/i40e_main.c   | 122 +++++++++++++++++-
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   |  13 +-
 drivers/net/ethernet/intel/i40e/i40e_txrx.h   |   1 -
 drivers/net/ethernet/intel/i40e/i40e_type.h   |   4 +
 drivers/net/ethernet/intel/i40e/i40e_xsk.c    |  67 ++++++++--
 drivers/net/ethernet/intel/i40e/i40e_xsk.h    |   2 +-
 8 files changed, 213 insertions(+), 36 deletions(-)

-- 
2.35.1

[PATCH net 1/3] i40e: Fix ethtool rx-flow-hash setting for X722

[Tony Nguyen]

From: Slawomir Laba <slawomirx.laba@intel.com>

When enabling flow type for RSS hash via ethtool:

ethtool -N $pf rx-flow-hash tcp4|tcp6|udp4|udp6 s|d

the driver would fail to setup this setting on X722
device since it was using the mask on the register
dedicated for X710 devices.

Apply a different mask on the register when setting the
RSS hash for the X722 device.

When displaying the flow types enabled via ethtool:

ethtool -n $pf rx-flow-hash tcp4|tcp6|udp4|udp6

the driver would print wrong values for X722 device.

Fix this issue by testing masks for X722 device in
i40e_get_rss_hash_opts function.

Fixes: eb0dd6e4a3b3 ("i40e: Allow RSS Hash set with less than four parameters")
Signed-off-by: Slawomir Laba <slawomirx.laba@intel.com>
Signed-off-by: Michal Jaron <michalx.jaron@intel.com>
Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 .../net/ethernet/intel/i40e/i40e_ethtool.c    | 31 ++++++++++++++-----
 drivers/net/ethernet/intel/i40e/i40e_type.h   |  4 +++
 2 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index e9cd0fa6a0d2..e518aaa2c0ca 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -3188,10 +3188,17 @@ static int i40e_get_rss_hash_opts(struct i40e_pf *pf, struct ethtool_rxnfc *cmd)
 
 		if (cmd->flow_type == TCP_V4_FLOW ||
 		    cmd->flow_type == UDP_V4_FLOW) {
-			if (i_set & I40E_L3_SRC_MASK)
-				cmd->data |= RXH_IP_SRC;
-			if (i_set & I40E_L3_DST_MASK)
-				cmd->data |= RXH_IP_DST;
+			if (hw->mac.type == I40E_MAC_X722) {
+				if (i_set & I40E_X722_L3_SRC_MASK)
+					cmd->data |= RXH_IP_SRC;
+				if (i_set & I40E_X722_L3_DST_MASK)
+					cmd->data |= RXH_IP_DST;
+			} else {
+				if (i_set & I40E_L3_SRC_MASK)
+					cmd->data |= RXH_IP_SRC;
+				if (i_set & I40E_L3_DST_MASK)
+					cmd->data |= RXH_IP_DST;
+			}
 		} else if (cmd->flow_type == TCP_V6_FLOW ||
 			  cmd->flow_type == UDP_V6_FLOW) {
 			if (i_set & I40E_L3_V6_SRC_MASK)
@@ -3549,12 +3556,15 @@ static int i40e_get_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd,
 
 /**
  * i40e_get_rss_hash_bits - Read RSS Hash bits from register
+ * @hw: hw structure
  * @nfc: pointer to user request
  * @i_setc: bits currently set
  *
  * Returns value of bits to be set per user request
  **/
-static u64 i40e_get_rss_hash_bits(struct ethtool_rxnfc *nfc, u64 i_setc)
+static u64 i40e_get_rss_hash_bits(struct i40e_hw *hw,
+				  struct ethtool_rxnfc *nfc,
+				  u64 i_setc)
 {
 	u64 i_set = i_setc;
 	u64 src_l3 = 0, dst_l3 = 0;
@@ -3573,8 +3583,13 @@ static u64 i40e_get_rss_hash_bits(struct ethtool_rxnfc *nfc, u64 i_setc)
 		dst_l3 = I40E_L3_V6_DST_MASK;
 	} else if (nfc->flow_type == TCP_V4_FLOW ||
 		  nfc->flow_type == UDP_V4_FLOW) {
-		src_l3 = I40E_L3_SRC_MASK;
-		dst_l3 = I40E_L3_DST_MASK;
+		if (hw->mac.type == I40E_MAC_X722) {
+			src_l3 = I40E_X722_L3_SRC_MASK;
+			dst_l3 = I40E_X722_L3_DST_MASK;
+		} else {
+			src_l3 = I40E_L3_SRC_MASK;
+			dst_l3 = I40E_L3_DST_MASK;
+		}
 	} else {
 		/* Any other flow type are not supported here */
 		return i_set;
@@ -3689,7 +3704,7 @@ static int i40e_set_rss_hash_opt(struct i40e_pf *pf, struct ethtool_rxnfc *nfc)
 					       flow_pctype)) |
 			((u64)i40e_read_rx_ctl(hw, I40E_GLQF_HASH_INSET(1,
 					       flow_pctype)) << 32);
-		i_set = i40e_get_rss_hash_bits(nfc, i_setc);
+		i_set = i40e_get_rss_hash_bits(&pf->hw, nfc, i_setc);
 		i40e_write_rx_ctl(hw, I40E_GLQF_HASH_INSET(0, flow_pctype),
 				  (u32)i_set);
 		i40e_write_rx_ctl(hw, I40E_GLQF_HASH_INSET(1, flow_pctype),
diff --git a/drivers/net/ethernet/intel/i40e/i40e_type.h b/drivers/net/ethernet/intel/i40e/i40e_type.h
index 7b3f30beb757..388c3d36d96a 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_type.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_type.h
@@ -1404,6 +1404,10 @@ struct i40e_lldp_variables {
 #define I40E_PFQF_CTL_0_HASHLUTSIZE_512	0x00010000
 
 /* INPUT SET MASK for RSS, flow director, and flexible payload */
+#define I40E_X722_L3_SRC_SHIFT		49
+#define I40E_X722_L3_SRC_MASK		(0x3ULL << I40E_X722_L3_SRC_SHIFT)
+#define I40E_X722_L3_DST_SHIFT		41
+#define I40E_X722_L3_DST_MASK		(0x3ULL << I40E_X722_L3_DST_SHIFT)
 #define I40E_L3_SRC_SHIFT		47
 #define I40E_L3_SRC_MASK		(0x3ULL << I40E_L3_SRC_SHIFT)
 #define I40E_L3_V6_SRC_SHIFT		43
-- 
2.35.1

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

[Tony Nguyen]

From: Michal Jaron <michalx.jaron@intel.com>

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.

Fixes: 6f853d4f8e93 ("i40e: allow XPS with QoS enabled")
Signed-off-by: Michal Jaron <michalx.jaron@intel.com>
Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e.h      |   6 ++
 drivers/net/ethernet/intel/i40e/i40e_main.c | 109 ++++++++++++++++++++
 2 files changed, 115 insertions(+)

diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
index d86b6d349ea9..238d00d3423a 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -1306,4 +1306,10 @@ static inline u32 i40e_is_tc_mqprio_enabled(struct i40e_pf *pf)
 	return pf->flags & I40E_FLAG_TC_MQPRIO;
 }
 
+/* reverse XPS CPUs to tx queues map */
+struct i40e_qmap_rev {
+	struct cpumask cpus;
+	int vsi_id;
+};
+
 #endif /* _I40E_H_ */
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index e3d9804aeb25..452ed104766e 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -10793,6 +10793,83 @@ static int i40e_reset(struct i40e_pf *pf)
 	return ret;
 }
 
+#ifdef CONFIG_XPS
+/**
+ * i40e_preserve_xps_settings - preserve XPS maps before reset
+ * @vsi: pointer to the targeted VSI
+ * @qr: pointer to the structure with XPS mapping
+ *
+ * Read queues mapping from every CPU and save it as a CPU mask for every
+ * queue.
+ **/
+static void
+i40e_preserve_xps_settings(struct i40e_vsi *vsi, struct i40e_qmap_rev *qr)
+{
+	int cpu, q_idx, cpu_idx, cpus = num_online_cpus();
+	struct net_device *netdev = vsi->netdev;
+	struct xps_dev_maps *dev_maps;
+	struct xps_map *map;
+	u64 bitmap_arr;
+
+	if (!netdev || vsi->type != I40E_VSI_MAIN)
+		return;
+
+	if (cpus < vsi->num_queue_pairs) {
+		dev_warn(&vsi->back->pdev->dev,
+			 "There are more queues than cpus. To set xps maps properly reinitialize queues.\n");
+		return;
+	}
+
+	rcu_read_lock();
+
+	if (!netdev->xps_maps[XPS_CPUS])
+		goto out;
+
+	dev_maps = rcu_dereference(netdev->xps_maps[XPS_CPUS]);
+
+	for (cpu = 0; cpu < cpus; cpu++) {
+		cpu_idx = cpumask_local_spread(cpu, -1);
+		if (!dev_maps->attr_map[cpu_idx])
+			continue;
+
+		map = rcu_dereference(dev_maps->attr_map[cpu_idx]);
+		bitmap_arr = cpu_idx;
+		do_div(bitmap_arr, BITS_PER_LONG);
+		for (q_idx = 0; q_idx < map->len; q_idx++) {
+			qr[map->queues[q_idx]].vsi_id = vsi->id;
+			qr[map->queues[q_idx]].cpus.bits[bitmap_arr] |=
+				BIT(cpu_idx);
+		}
+	}
+
+out:
+	rcu_read_unlock();
+}
+
+/**
+ * i40e_restore_xps_settings - restore XPS maps after reset
+ * @vsi: pointer to the targeted VSI
+ * @qr: pointer to the structure with XPS mapping
+ *
+ * Set previously preserved XPS CPUs to queues mapping.
+ **/
+static void
+i40e_restore_xps_settings(struct i40e_vsi *vsi, struct i40e_qmap_rev *qr)
+{
+	struct net_device *netdev = vsi->netdev;
+	int q_count, q;
+
+	q_count = min_t(unsigned int, num_online_cpus(), vsi->num_queue_pairs);
+
+	if (vsi->type != I40E_VSI_MAIN)
+		return;
+
+	for (q = 0; q < q_count; q++)
+		if (qr[q].vsi_id == vsi->id)
+			netif_set_xps_queue(netdev, &qr[q].cpus, q);
+}
+
+#endif /* CONFIG_XPS */
 /**
  * i40e_rebuild - rebuild using a saved config
  * @pf: board private structure
@@ -10804,6 +10881,9 @@ static void i40e_rebuild(struct i40e_pf *pf, bool reinit, bool lock_acquired)
 {
 	const bool is_recovery_mode_reported = i40e_check_recovery_mode(pf);
 	struct i40e_vsi *vsi = pf->vsi[pf->lan_vsi];
+#ifdef CONFIG_XPS
+	struct i40e_qmap_rev *qr = NULL;
+#endif /* CONFIG_XPS */
 	struct i40e_hw *hw = &pf->hw;
 	i40e_status ret;
 	u32 val;
@@ -10919,6 +10999,22 @@ static void i40e_rebuild(struct i40e_pf *pf, bool reinit, bool lock_acquired)
 	}
 
 #endif /* CONFIG_I40E_DCB */
+#ifdef CONFIG_XPS
+	if (!reinit) {
+		int cpus = num_possible_cpus();
+
+		qr = kcalloc(cpus, sizeof(struct i40e_qmap_rev), GFP_KERNEL);
+		if (!qr) {
+			ret = -ENOMEM;
+			goto end_unlock;
+		}
+
+		for (v = 0; v < pf->num_alloc_vsi; v++)
+			if (pf->vsi[v])
+				i40e_preserve_xps_settings(pf->vsi[v], qr);
+	}
+
+#endif /* CONFIG_XPS */
 	if (!lock_acquired)
 		rtnl_lock();
 	ret = i40e_setup_pf_switch(pf, reinit, true);
@@ -11073,6 +11169,16 @@ static void i40e_rebuild(struct i40e_pf *pf, bool reinit, bool lock_acquired)
 
 	i40e_reset_all_vfs(pf, true);
 
+#ifdef CONFIG_XPS
+	if (!reinit) {
+		for (v = 0; v < pf->num_alloc_vsi; v++)
+			if (pf->vsi[v])
+				i40e_restore_xps_settings(pf->vsi[v], qr);
+	} else {
+		dev_info(&pf->pdev->dev, "XPS maps were reset to default after queue re-initialization");
+	}
+
+#endif /* CONFIG_XPS */
 	/* tell the firmware that we're starting */
 	i40e_send_version(pf);
 
@@ -11082,6 +11188,9 @@ static void i40e_rebuild(struct i40e_pf *pf, bool reinit, bool lock_acquired)
 end_unlock:
 	if (!lock_acquired)
 		rtnl_unlock();
+#ifdef CONFIG_XPS
+	kfree(qr);
+#endif /* CONFIG_XPS */
 end_core_reset:
 	clear_bit(__I40E_RESET_FAILED, pf->state);
 clear_recovery:
-- 
2.35.1

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

[Jakub Kicinski]

On Mon, 26 Sep 2022 13:32:13 -0700 Tony Nguyen wrote:
> 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

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

[Jaron, MichalX]

> -----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 <michalx.jaron@intel.com>; netdev@vger.kernel.org;
> Maziarz, Kamil <kamil.maziarz@intel.com>; G, GurucharanX
> <gurucharanx.g@intel.com>
> 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:
> > 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.

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

[Jakub Kicinski]

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)?

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

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

[Jaron, MichalX]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, September 28, 2022 4:11 PM
> To: Jaron, MichalX <michalx.jaron@intel.com>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>;
> davem@davemloft.net; pabeni@redhat.com; edumazet@google.com;
> netdev@vger.kernel.org; Maziarz, Kamil <kamil.maziarz@intel.com>; G,
> GurucharanX <gurucharanx.g@intel.com>; Dziedziuch, SylwesterX
> <sylwesterx.dziedziuch@intel.com>; Brandeburg, Jesse
> <jesse.brandeburg@intel.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.

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

[Tony Nguyen]

On 9/29/2022 4:58 AM, Jaron, MichalX wrote:
>> -----Original Message-----
>> From: Jakub Kicinski <kuba@kernel.org>
>> Sent: Wednesday, September 28, 2022 4:11 PM
>> To: Jaron, MichalX <michalx.jaron@intel.com>
>> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>;
>> davem@davemloft.net; pabeni@redhat.com; edumazet@google.com;
>> netdev@vger.kernel.org; Maziarz, Kamil <kamil.maziarz@intel.com>; G,
>> GurucharanX <gurucharanx.g@intel.com>; Dziedziuch, SylwesterX
>> <sylwesterx.dziedziuch@intel.com>; Brandeburg, Jesse
>> <jesse.brandeburg@intel.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.

Hi Jakub,

The version with updated commit message[1] is on Intel Wired LAN but I'm 
not sure whether your comment still stands or if this is ok after the 
explanation/updated message.

Thanks,
Tony

[1] 
https://lore.kernel.org/intel-wired-lan/20221005132209.3599781-1-kamil.maziarz@intel.com/

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

[Jakub Kicinski]

On Thu, 6 Oct 2022 15:28:47 -0700 Tony Nguyen wrote:
> The version with updated commit message[1] is on Intel Wired LAN but I'm 
> not sure whether your comment still stands or if this is ok after the 
> explanation/updated message.

As far as I know wiping XPS on ethtool -L is what most drivers do. We
need to have a discussion about who is expecting the config to not get
wiped and under what conditions. Then think about ways of doing this
generically.

The local driver patches are a no-go from my PoV.

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

[Tony Nguyen]

On 9/27/2022 6:29 PM, Jakub Kicinski wrote:
> On Mon, 26 Sep 2022 13:32:13 -0700 Tony Nguyen wrote:
>> 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.

In the interest of the other patches, hopefully, making this kernel, I'm 
going to drop this from the pull request while it's being discussed.

Thanks,
Tony

[PATCH net 3/3] i40e: Fix DMA mappings leak

[Tony Nguyen]

From: Jan Sokolowski <jan.sokolowski@intel.com>

During reallocation of RX buffers, new DMA mappings are created for
those buffers. New buffers with different RX ring count should
substitute older ones, but those buffers were freed in
i40e_configure_rx_ring and reallocated again with i40e_alloc_rx_bi,
thus kfree on rx_bi caused leak of already mapped DMA.

In case of non XDP ring, do not free rx_bi and reuse already existing
buffer, move kfree to XDP rings only, remove unused i40e_alloc_rx_bi
function.

steps for reproduction:
while :
do
for ((i=0; i<=8160; i=i+32))
do
ethtool -G enp130s0f0 rx $i tx $i
sleep 0.5
ethtool -g enp130s0f0
done
done

Fixes: be1222b585fd ("i40e: Separate kernel allocated rx_bi rings from AF_XDP rings")
Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com>
Tested-by: Chandan <chandanx.rout@intel.com> (A Contingent Worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 .../net/ethernet/intel/i40e/i40e_ethtool.c    |  3 -
 drivers/net/ethernet/intel/i40e/i40e_main.c   | 13 ++--
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 13 ++--
 drivers/net/ethernet/intel/i40e/i40e_txrx.h   |  1 -
 drivers/net/ethernet/intel/i40e/i40e_xsk.c    | 67 ++++++++++++++++---
 drivers/net/ethernet/intel/i40e/i40e_xsk.h    |  2 +-
 6 files changed, 71 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index e518aaa2c0ca..0f2042f1597c 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -2181,9 +2181,6 @@ static int i40e_set_ringparam(struct net_device *netdev,
 			 */
 			rx_rings[i].tail = hw->hw_addr + I40E_PRTGEN_STATUS;
 			err = i40e_setup_rx_descriptors(&rx_rings[i]);
-			if (err)
-				goto rx_unwind;
-			err = i40e_alloc_rx_bi(&rx_rings[i]);
 			if (err)
 				goto rx_unwind;
 
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 452ed104766e..e8d1fb8a966b 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -3565,12 +3565,8 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring)
 	if (ring->vsi->type == I40E_VSI_MAIN)
 		xdp_rxq_info_unreg_mem_model(&ring->xdp_rxq);
 
-	kfree(ring->rx_bi);
 	ring->xsk_pool = i40e_xsk_pool(ring);
 	if (ring->xsk_pool) {
-		ret = i40e_alloc_rx_bi_zc(ring);
-		if (ret)
-			return ret;
 		ring->rx_buf_len =
 		  xsk_pool_get_rx_frame_size(ring->xsk_pool);
 		/* For AF_XDP ZC, we disallow packets to span on
@@ -3588,9 +3584,6 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring)
 			 ring->queue_index);
 
 	} else {
-		ret = i40e_alloc_rx_bi(ring);
-		if (ret)
-			return ret;
 		ring->rx_buf_len = vsi->rx_buf_len;
 		if (ring->vsi->type == I40E_VSI_MAIN) {
 			ret = xdp_rxq_info_reg_mem_model(&ring->xdp_rxq,
@@ -13413,6 +13406,11 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog,
 		i40e_reset_and_rebuild(pf, true, true);
 	}
 
+	if (!i40e_enabled_xdp_vsi(vsi) && prog)
+		i40e_realloc_rx_bi_zc(vsi, true);
+	else if (i40e_enabled_xdp_vsi(vsi) && !prog)
+		i40e_realloc_rx_bi_zc(vsi, false);
+
 	for (i = 0; i < vsi->num_queue_pairs; i++)
 		WRITE_ONCE(vsi->rx_rings[i]->xdp_prog, vsi->xdp_prog);
 
@@ -13645,6 +13643,7 @@ int i40e_queue_pair_disable(struct i40e_vsi *vsi, int queue_pair)
 
 	i40e_queue_pair_disable_irq(vsi, queue_pair);
 	err = i40e_queue_pair_toggle_rings(vsi, queue_pair, false /* off */);
+	i40e_clean_rx_ring(vsi->rx_rings[queue_pair]);
 	i40e_queue_pair_toggle_napi(vsi, queue_pair, false /* off */);
 	i40e_queue_pair_clean_rings(vsi, queue_pair);
 	i40e_queue_pair_reset_stats(vsi, queue_pair);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 69e67eb6aea7..b97c95f89fa0 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -1457,14 +1457,6 @@ int i40e_setup_tx_descriptors(struct i40e_ring *tx_ring)
 	return -ENOMEM;
 }
 
-int i40e_alloc_rx_bi(struct i40e_ring *rx_ring)
-{
-	unsigned long sz = sizeof(*rx_ring->rx_bi) * rx_ring->count;
-
-	rx_ring->rx_bi = kzalloc(sz, GFP_KERNEL);
-	return rx_ring->rx_bi ? 0 : -ENOMEM;
-}
-
 static void i40e_clear_rx_bi(struct i40e_ring *rx_ring)
 {
 	memset(rx_ring->rx_bi, 0, sizeof(*rx_ring->rx_bi) * rx_ring->count);
@@ -1593,6 +1585,11 @@ int i40e_setup_rx_descriptors(struct i40e_ring *rx_ring)
 
 	rx_ring->xdp_prog = rx_ring->vsi->xdp_prog;
 
+	rx_ring->rx_bi =
+		kcalloc(rx_ring->count, sizeof(*rx_ring->rx_bi), GFP_KERNEL);
+	if (!rx_ring->rx_bi)
+		return -ENOMEM;
+
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
index 41f86e9535a0..768290dc6f48 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
@@ -469,7 +469,6 @@ int __i40e_maybe_stop_tx(struct i40e_ring *tx_ring, int size);
 bool __i40e_chk_linearize(struct sk_buff *skb);
 int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames,
 		  u32 flags);
-int i40e_alloc_rx_bi(struct i40e_ring *rx_ring);
 
 /**
  * i40e_get_head - Retrieve head from head writeback
diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 6d4009e0cbd6..790aaeff1b47 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -10,14 +10,6 @@
 #include "i40e_txrx_common.h"
 #include "i40e_xsk.h"
 
-int i40e_alloc_rx_bi_zc(struct i40e_ring *rx_ring)
-{
-	unsigned long sz = sizeof(*rx_ring->rx_bi_zc) * rx_ring->count;
-
-	rx_ring->rx_bi_zc = kzalloc(sz, GFP_KERNEL);
-	return rx_ring->rx_bi_zc ? 0 : -ENOMEM;
-}
-
 void i40e_clear_rx_bi_zc(struct i40e_ring *rx_ring)
 {
 	memset(rx_ring->rx_bi_zc, 0,
@@ -29,6 +21,58 @@ static struct xdp_buff **i40e_rx_bi(struct i40e_ring *rx_ring, u32 idx)
 	return &rx_ring->rx_bi_zc[idx];
 }
 
+/**
+ * i40e_realloc_rx_xdp_bi - reallocate for either XSK or normal buffer
+ * @rx_ring: Current rx ring
+ * @pool_present: is pool for XSK present
+ *
+ * Try allocating memory and return ENOMEM, if failed to allocate.
+ * If allocation was successful, substitute buffer with allocated one.
+ * Returns 0 on success, negative on failure
+ */
+static int i40e_realloc_rx_xdp_bi(struct i40e_ring *rx_ring, bool pool_present)
+{
+	size_t elem_size = pool_present ? sizeof(*rx_ring->rx_bi_zc) :
+					  sizeof(*rx_ring->rx_bi);
+	void *sw_ring = kcalloc(rx_ring->count, elem_size, GFP_KERNEL);
+
+	if (!sw_ring)
+		return -ENOMEM;
+
+	if (pool_present) {
+		kfree(rx_ring->rx_bi);
+		rx_ring->rx_bi = NULL;
+		rx_ring->rx_bi_zc = sw_ring;
+	} else {
+		kfree(rx_ring->rx_bi_zc);
+		rx_ring->rx_bi_zc = NULL;
+		rx_ring->rx_bi = sw_ring;
+	}
+	return 0;
+}
+
+/**
+ * i40e_realloc_rx_bi_zc - reallocate xdp queue pairs
+ * @vsi: Current VSI
+ * @zc: is zero copy set
+ *
+ * Reallocate buffer for rx_rings that might be used by XSK.
+ * XDP requires more memory, than rx_buf provides.
+ * Returns 0 on success, negative on failure
+ */
+int i40e_realloc_rx_bi_zc(struct i40e_vsi *vsi, bool zc)
+{
+	struct i40e_ring *rx_ring;
+	unsigned long q;
+
+	for_each_set_bit(q, vsi->af_xdp_zc_qps, vsi->alloc_queue_pairs) {
+		rx_ring = vsi->rx_rings[q];
+		if (i40e_realloc_rx_xdp_bi(rx_ring, zc))
+			return -ENOMEM;
+	}
+	return 0;
+}
+
 /**
  * i40e_xsk_pool_enable - Enable/associate an AF_XDP buffer pool to a
  * certain ring/qid
@@ -69,6 +113,10 @@ static int i40e_xsk_pool_enable(struct i40e_vsi *vsi,
 		if (err)
 			return err;
 
+		err = i40e_realloc_rx_xdp_bi(vsi->rx_rings[qid], true);
+		if (err)
+			return err;
+
 		err = i40e_queue_pair_enable(vsi, qid);
 		if (err)
 			return err;
@@ -113,6 +161,9 @@ static int i40e_xsk_pool_disable(struct i40e_vsi *vsi, u16 qid)
 	xsk_pool_dma_unmap(pool, I40E_RX_DMA_ATTR);
 
 	if (if_running) {
+		err = i40e_realloc_rx_xdp_bi(vsi->rx_rings[qid], false);
+		if (err)
+			return err;
 		err = i40e_queue_pair_enable(vsi, qid);
 		if (err)
 			return err;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.h b/drivers/net/ethernet/intel/i40e/i40e_xsk.h
index bb962987f300..821df248f8be 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.h
@@ -32,7 +32,7 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget);
 
 bool i40e_clean_xdp_tx_irq(struct i40e_vsi *vsi, struct i40e_ring *tx_ring);
 int i40e_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags);
-int i40e_alloc_rx_bi_zc(struct i40e_ring *rx_ring);
+int i40e_realloc_rx_bi_zc(struct i40e_vsi *vsi, bool zc);
 void i40e_clear_rx_bi_zc(struct i40e_ring *rx_ring);
 
 #endif /* _I40E_XSK_H_ */
-- 
2.35.1

RE: [PATCH net 3/3] i40e: Fix DMA mappings leak

[Rout, ChandanX]

>-----Original Message-----
>From: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
>Sent: Tuesday, September 27, 2022 2:02 AM
>To: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
>edumazet@google.com
>Cc: Sokolowski, Jan <jan.sokolowski@intel.com>; netdev@vger.kernel.org;
>Nguyen, Anthony L <anthony.l.nguyen@intel.com>; bjorn@kernel.org;
>Fijalkowski, Maciej <maciej.fijalkowski@intel.com>; Karlsson, Magnus
><magnus.karlsson@intel.com>; ast@kernel.org; daniel@iogearbox.net;
>hawk@kernel.org; john.fastabend@gmail.com; bpf@vger.kernel.org; Rout,
>ChandanX <chandanx.rout@intel.com>
>Subject: [PATCH net 3/3] i40e: Fix DMA mappings leak
>
>From: Jan Sokolowski <jan.sokolowski@intel.com>
>
>During reallocation of RX buffers, new DMA mappings are created for those
>buffers. New buffers with different RX ring count should substitute older
>ones, but those buffers were freed in i40e_configure_rx_ring and reallocated
>again with i40e_alloc_rx_bi, thus kfree on rx_bi caused leak of already
>mapped DMA.
>
>In case of non XDP ring, do not free rx_bi and reuse already existing buffer,
>move kfree to XDP rings only, remove unused i40e_alloc_rx_bi function.
>
>steps for reproduction:
>while :
>do
>for ((i=0; i<=8160; i=i+32))
>do
>ethtool -G enp130s0f0 rx $i tx $i
>sleep 0.5
>ethtool -g enp130s0f0
>done
>done
>
>Fixes: be1222b585fd ("i40e: Separate kernel allocated rx_bi rings from AF_XDP
>rings")
>Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com>
>Tested-by: Chandan <chandanx.rout@intel.com> (A Contingent Worker at
>Intel)
>Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>---
> .../net/ethernet/intel/i40e/i40e_ethtool.c    |  3 -
> drivers/net/ethernet/intel/i40e/i40e_main.c   | 13 ++--
> drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 13 ++--
> drivers/net/ethernet/intel/i40e/i40e_txrx.h   |  1 -
> drivers/net/ethernet/intel/i40e/i40e_xsk.c    | 67 ++++++++++++++++---
> drivers/net/ethernet/intel/i40e/i40e_xsk.h    |  2 +-
> 6 files changed, 71 insertions(+), 28 deletions(-)

Tested-by: Chandan <chandanx.rout@intel.com> (A Contingent Worker at Intel)