[PATCH net-next v6 0/5] net: napi: add CPU affinity to napi->config

[PATCH net-next v6 0/5] net: napi: add CPU affinity to napi->config

[Ahmed Zaki]

Drivers usually need to re-apply the user-set IRQ affinity to their IRQs
after reset. However, since there can be only one IRQ affinity notifier
for each IRQ, registering IRQ notifiers conflicts with the ARFS rmap
management in the core (which also registers separate IRQ affinity
notifiers).   

Move the IRQ affinity management to the napi struct. This way we can have
a unified IRQ notifier to re-apply the user-set affinity and also manage
the ARFS rmaps. Patches 1 and 2 move the ARFS rmap management to CORE.
Patch 3 adds the IRQ affinity mask to napi_config and re-applies the mask
after reset. Patches 4-6 use the new API for bnxt, ice and idpf drivers.

Tested on bnxt, ice and idpf.
V6:
    - Modifications to have less #ifdef CONFIG_RF_ACCL guards
    - Remove rmap entry in napi_disable
    - Rebase on rc7 and use netif_napi_set_irq_locked()
    - Assume IRQ can be -1 and free resources if an old valid IRQ was
      associated with the napi. For this, I had to merge the first 2
      patches to use the new rmap API.

V5:
    - https://lore.kernel.org/netdev/20250113171042.158123-1-ahmed.zaki@intel.com/
    - Add kernel doc for new netdev flags (Simon).
    - Remove defensive (if !napi) check in napi_irq_cpu_rmap_add()
      (patch 2) since caller is already dereferencing the pointer (Simon).
    - Fix build error when CONFIG_ARFS_ACCEL is not defined (patch 3).

v4:
    - https://lore.kernel.org/netdev/20250109233107.17519-1-ahmed.zaki@intel.com/
    - Better introduction in the cover letter.
    - Fix Kernel build errors in ena_init_rx_cpu_rmap() (Patch 1)
    - Fix kernel test robot warnings reported by Dan Carpenter:
      https://lore.kernel.org/all/202501050625.nY1c97EX-lkp@intel.com/
    - Remove unrelated empty line in patch 4 (Kalesh Anakkur Purayil)
    - Fix a memleak (rmap was not freed) by calling cpu_rmap_put() in
      netif_napi_affinity_release() (patch 2).

v3:
    - https://lore.kernel.org/netdev/20250104004314.208259-1-ahmed.zaki@intel.com/
    - Assign one cpu per mask starting from local NUMA node (Shay Drori).
    - Keep the new ARFS and Affinity flags per nedev (Jakub).

v2:
    - https://lore.kernel.org/netdev/202412190454.nwvp3hU2-lkp@intel.com/T/
    - Also move the ARFS IRQ affinity management from drivers to core. Via
      netif_napi_set_irq(), drivers can ask the core to add the IRQ to the
      ARFS rmap (already allocated by the driver).

RFC -> v1:
    - https://lore.kernel.org/netdev/20241210002626.366878-1-ahmed.zaki@intel.com/
    - move static inline affinity functions to net/dev/core.c
    - add the new napi->irq_flags (patch 1)
    - add code changes to bnxt, mlx4 and ice.

Ahmed Zaki (5):
  net: move ARFS rmap management to core
  net: napi: add CPU affinity to napi_config
  bnxt: use napi's irq affinity
  ice: use napi's irq affinity
  idpf: use napi's irq affinity

 drivers/net/ethernet/amazon/ena/ena_netdev.c |  43 +----
 drivers/net/ethernet/broadcom/bnxt/bnxt.c    |  54 +-----
 drivers/net/ethernet/broadcom/bnxt/bnxt.h    |   2 -
 drivers/net/ethernet/intel/ice/ice.h         |   3 -
 drivers/net/ethernet/intel/ice/ice_arfs.c    |  17 +-
 drivers/net/ethernet/intel/ice/ice_base.c    |   7 +-
 drivers/net/ethernet/intel/ice/ice_lib.c     |   6 -
 drivers/net/ethernet/intel/ice/ice_main.c    |  47 +-----
 drivers/net/ethernet/intel/idpf/idpf_lib.c   |   1 +
 drivers/net/ethernet/intel/idpf/idpf_txrx.c  |  22 +--
 drivers/net/ethernet/intel/idpf/idpf_txrx.h  |   6 +-
 include/linux/cpu_rmap.h                     |   1 +
 include/linux/netdevice.h                    |  27 ++-
 lib/cpu_rmap.c                               |   2 +-
 net/core/dev.c                               | 167 ++++++++++++++++++-
 15 files changed, 215 insertions(+), 190 deletions(-)

-- 
2.43.0

[PATCH net-next v6 1/5] net: move ARFS rmap management to core

[Ahmed Zaki]

Add a new netdev flag "rx_cpu_rmap_auto". Drivers supporting ARFS should
set the flag via netif_enable_cpu_rmap() and core will allocate and manage
the ARFS rmap. Freeing the rmap is also done by core when the netdev is
freed.

For better IRQ affinity management, move the IRQ rmap notifier inside the
napi_struct. Consequently, add new notify.notify and notify.release
functions: netif_irq_cpu_rmap_notify() and netif_napi_affinity_release().

Acked-by: David Arinzon <darinzon@amazon.com>
Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
---
 drivers/net/ethernet/amazon/ena/ena_netdev.c |  43 +-----
 drivers/net/ethernet/broadcom/bnxt/bnxt.c    |  29 +---
 drivers/net/ethernet/intel/ice/ice_arfs.c    |  17 +--
 include/linux/cpu_rmap.h                     |   1 +
 include/linux/netdevice.h                    |  15 +-
 lib/cpu_rmap.c                               |   2 +-
 net/core/dev.c                               | 140 +++++++++++++++++++
 7 files changed, 159 insertions(+), 88 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index c1295dfad0d0..6aab85a7c60a 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -5,9 +5,6 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#ifdef CONFIG_RFS_ACCEL
-#include <linux/cpu_rmap.h>
-#endif /* CONFIG_RFS_ACCEL */
 #include <linux/ethtool.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -162,30 +159,6 @@ int ena_xmit_common(struct ena_adapter *adapter,
 	return 0;
 }
 
-static int ena_init_rx_cpu_rmap(struct ena_adapter *adapter)
-{
-#ifdef CONFIG_RFS_ACCEL
-	u32 i;
-	int rc;
-
-	adapter->netdev->rx_cpu_rmap = alloc_irq_cpu_rmap(adapter->num_io_queues);
-	if (!adapter->netdev->rx_cpu_rmap)
-		return -ENOMEM;
-	for (i = 0; i < adapter->num_io_queues; i++) {
-		int irq_idx = ENA_IO_IRQ_IDX(i);
-
-		rc = irq_cpu_rmap_add(adapter->netdev->rx_cpu_rmap,
-				      pci_irq_vector(adapter->pdev, irq_idx));
-		if (rc) {
-			free_irq_cpu_rmap(adapter->netdev->rx_cpu_rmap);
-			adapter->netdev->rx_cpu_rmap = NULL;
-			return rc;
-		}
-	}
-#endif /* CONFIG_RFS_ACCEL */
-	return 0;
-}
-
 static void ena_init_io_rings_common(struct ena_adapter *adapter,
 				     struct ena_ring *ring, u16 qid)
 {
@@ -1596,7 +1569,7 @@ static int ena_enable_msix(struct ena_adapter *adapter)
 		adapter->num_io_queues = irq_cnt - ENA_ADMIN_MSIX_VEC;
 	}
 
-	if (ena_init_rx_cpu_rmap(adapter))
+	if (netif_enable_cpu_rmap(adapter->netdev, adapter->num_io_queues))
 		netif_warn(adapter, probe, adapter->netdev,
 			   "Failed to map IRQs to CPUs\n");
 
@@ -1742,13 +1715,6 @@ static void ena_free_io_irq(struct ena_adapter *adapter)
 	struct ena_irq *irq;
 	int i;
 
-#ifdef CONFIG_RFS_ACCEL
-	if (adapter->msix_vecs >= 1) {
-		free_irq_cpu_rmap(adapter->netdev->rx_cpu_rmap);
-		adapter->netdev->rx_cpu_rmap = NULL;
-	}
-#endif /* CONFIG_RFS_ACCEL */
-
 	for (i = ENA_IO_IRQ_FIRST_IDX; i < ENA_MAX_MSIX_VEC(io_queue_count); i++) {
 		irq = &adapter->irq_tbl[i];
 		irq_set_affinity_hint(irq->vector, NULL);
@@ -4131,13 +4097,6 @@ static void __ena_shutoff(struct pci_dev *pdev, bool shutdown)
 	ena_dev = adapter->ena_dev;
 	netdev = adapter->netdev;
 
-#ifdef CONFIG_RFS_ACCEL
-	if ((adapter->msix_vecs >= 1) && (netdev->rx_cpu_rmap)) {
-		free_irq_cpu_rmap(netdev->rx_cpu_rmap);
-		netdev->rx_cpu_rmap = NULL;
-	}
-
-#endif /* CONFIG_RFS_ACCEL */
 	/* Make sure timer and reset routine won't be called after
 	 * freeing device resources.
 	 */
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 748c9b1ea701..13bcb055df88 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -49,7 +49,6 @@
 #include <linux/cache.h>
 #include <linux/log2.h>
 #include <linux/bitmap.h>
-#include <linux/cpu_rmap.h>
 #include <linux/cpumask.h>
 #include <net/pkt_cls.h>
 #include <net/page_pool/helpers.h>
@@ -10879,10 +10878,8 @@ static int bnxt_set_real_num_queues(struct bnxt *bp)
 	if (rc)
 		return rc;
 
-#ifdef CONFIG_RFS_ACCEL
 	if (bp->flags & BNXT_FLAG_RFS)
-		dev->rx_cpu_rmap = alloc_irq_cpu_rmap(bp->rx_nr_rings);
-#endif
+		return netif_enable_cpu_rmap(dev, bp->rx_nr_rings);
 
 	return rc;
 }
@@ -11235,10 +11232,6 @@ static void bnxt_free_irq(struct bnxt *bp)
 	struct bnxt_irq *irq;
 	int i;
 
-#ifdef CONFIG_RFS_ACCEL
-	free_irq_cpu_rmap(bp->dev->rx_cpu_rmap);
-	bp->dev->rx_cpu_rmap = NULL;
-#endif
 	if (!bp->irq_tbl || !bp->bnapi)
 		return;
 
@@ -11261,11 +11254,8 @@ static void bnxt_free_irq(struct bnxt *bp)
 
 static int bnxt_request_irq(struct bnxt *bp)
 {
-	int i, j, rc = 0;
+	int i, rc = 0;
 	unsigned long flags = 0;
-#ifdef CONFIG_RFS_ACCEL
-	struct cpu_rmap *rmap;
-#endif
 
 	rc = bnxt_setup_int_mode(bp);
 	if (rc) {
@@ -11273,22 +11263,11 @@ static int bnxt_request_irq(struct bnxt *bp)
 			   rc);
 		return rc;
 	}
-#ifdef CONFIG_RFS_ACCEL
-	rmap = bp->dev->rx_cpu_rmap;
-#endif
-	for (i = 0, j = 0; i < bp->cp_nr_rings; i++) {
+
+	for (i = 0; i < bp->cp_nr_rings; i++) {
 		int map_idx = bnxt_cp_num_to_irq_num(bp, i);
 		struct bnxt_irq *irq = &bp->irq_tbl[map_idx];
 
-#ifdef CONFIG_RFS_ACCEL
-		if (rmap && bp->bnapi[i]->rx_ring) {
-			rc = irq_cpu_rmap_add(rmap, irq->vector);
-			if (rc)
-				netdev_warn(bp->dev, "failed adding irq rmap for ring %d\n",
-					    j);
-			j++;
-		}
-#endif
 		rc = request_irq(irq->vector, irq->handler, flags, irq->name,
 				 bp->bnapi[i]);
 		if (rc)
diff --git a/drivers/net/ethernet/intel/ice/ice_arfs.c b/drivers/net/ethernet/intel/ice/ice_arfs.c
index 7cee365cc7d1..3b1b892e6958 100644
--- a/drivers/net/ethernet/intel/ice/ice_arfs.c
+++ b/drivers/net/ethernet/intel/ice/ice_arfs.c
@@ -584,9 +584,6 @@ void ice_free_cpu_rx_rmap(struct ice_vsi *vsi)
 	netdev = vsi->netdev;
 	if (!netdev || !netdev->rx_cpu_rmap)
 		return;
-
-	free_irq_cpu_rmap(netdev->rx_cpu_rmap);
-	netdev->rx_cpu_rmap = NULL;
 }
 
 /**
@@ -597,7 +594,6 @@ int ice_set_cpu_rx_rmap(struct ice_vsi *vsi)
 {
 	struct net_device *netdev;
 	struct ice_pf *pf;
-	int i;
 
 	if (!vsi || vsi->type != ICE_VSI_PF)
 		return 0;
@@ -610,18 +606,7 @@ int ice_set_cpu_rx_rmap(struct ice_vsi *vsi)
 	netdev_dbg(netdev, "Setup CPU RMAP: vsi type 0x%x, ifname %s, q_vectors %d\n",
 		   vsi->type, netdev->name, vsi->num_q_vectors);
 
-	netdev->rx_cpu_rmap = alloc_irq_cpu_rmap(vsi->num_q_vectors);
-	if (unlikely(!netdev->rx_cpu_rmap))
-		return -EINVAL;
-
-	ice_for_each_q_vector(vsi, i)
-		if (irq_cpu_rmap_add(netdev->rx_cpu_rmap,
-				     vsi->q_vectors[i]->irq.virq)) {
-			ice_free_cpu_rx_rmap(vsi);
-			return -EINVAL;
-		}
-
-	return 0;
+	return netif_enable_cpu_rmap(netdev, vsi->num_q_vectors);
 }
 
 /**
diff --git a/include/linux/cpu_rmap.h b/include/linux/cpu_rmap.h
index 20b5729903d7..2fd7ba75362a 100644
--- a/include/linux/cpu_rmap.h
+++ b/include/linux/cpu_rmap.h
@@ -32,6 +32,7 @@ struct cpu_rmap {
 #define CPU_RMAP_DIST_INF 0xffff
 
 extern struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags);
+extern void cpu_rmap_get(struct cpu_rmap *rmap);
 extern int cpu_rmap_put(struct cpu_rmap *rmap);
 
 extern int cpu_rmap_add(struct cpu_rmap *rmap, void *obj);
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 8308d9c75918..98259f19c627 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -393,6 +393,10 @@ struct napi_struct {
 	struct list_head	dev_list;
 	struct hlist_node	napi_hash_node;
 	int			irq;
+#ifdef CONFIG_RFS_ACCEL
+	struct irq_affinity_notify notify;
+	int			napi_rmap_idx;
+#endif
 	int			index;
 	struct napi_config	*config;
 };
@@ -1987,6 +1991,9 @@ enum netdev_reg_state {
  *
  *	@threaded:	napi threaded mode is enabled
  *
+ *	@rx_cpu_rmap_auto: driver wants the core to manage the ARFS rmap.
+ *	                   Set by calling netif_enable_cpu_rmap().
+ *
  *	@see_all_hwtstamp_requests: device wants to see calls to
  *			ndo_hwtstamp_set() for all timestamp requests
  *			regardless of source, even if those aren't
@@ -2394,6 +2401,7 @@ struct net_device {
 	struct lock_class_key	*qdisc_tx_busylock;
 	bool			proto_down;
 	bool			threaded;
+	bool			rx_cpu_rmap_auto;
 
 	/* priv_flags_slow, ungrouped to save space */
 	unsigned long		see_all_hwtstamp_requests:1;
@@ -2708,10 +2716,7 @@ static inline void netdev_assert_locked_or_invisible(struct net_device *dev)
 		netdev_assert_locked(dev);
 }
 
-static inline void netif_napi_set_irq_locked(struct napi_struct *napi, int irq)
-{
-	napi->irq = irq;
-}
+void netif_napi_set_irq_locked(struct napi_struct *napi, int irq);
 
 static inline void netif_napi_set_irq(struct napi_struct *napi, int irq)
 {
@@ -2849,6 +2854,8 @@ static inline void netif_napi_del(struct napi_struct *napi)
 	synchronize_net();
 }
 
+int netif_enable_cpu_rmap(struct net_device *dev, unsigned int num_irqs);
+
 struct packet_type {
 	__be16			type;	/* This is really htons(ether_type). */
 	bool			ignore_outgoing;
diff --git a/lib/cpu_rmap.c b/lib/cpu_rmap.c
index 4c348670da31..f03d9be3f06b 100644
--- a/lib/cpu_rmap.c
+++ b/lib/cpu_rmap.c
@@ -73,7 +73,7 @@ static void cpu_rmap_release(struct kref *ref)
  * cpu_rmap_get - internal helper to get new ref on a cpu_rmap
  * @rmap: reverse-map allocated with alloc_cpu_rmap()
  */
-static inline void cpu_rmap_get(struct cpu_rmap *rmap)
+void cpu_rmap_get(struct cpu_rmap *rmap)
 {
 	kref_get(&rmap->refcount);
 }
diff --git a/net/core/dev.c b/net/core/dev.c
index fe5f5855593d..dbb63005bc2b 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6862,6 +6862,141 @@ void netif_queue_set_napi(struct net_device *dev, unsigned int queue_index,
 }
 EXPORT_SYMBOL(netif_queue_set_napi);
 
+#ifdef CONFIG_RFS_ACCEL
+static void
+netif_irq_cpu_rmap_notify(struct irq_affinity_notify *notify,
+			  const cpumask_t *mask)
+{
+	struct napi_struct *napi =
+		container_of(notify, struct napi_struct, notify);
+	struct cpu_rmap *rmap = napi->dev->rx_cpu_rmap;
+	int err;
+
+	if (napi->dev->rx_cpu_rmap_auto) {
+		err = cpu_rmap_update(rmap, napi->napi_rmap_idx, mask);
+		if (err)
+			pr_warn("%s: RMAP update failed (%d)\n",
+				__func__, err);
+	}
+}
+
+static void netif_napi_affinity_release(struct kref *ref)
+{
+	struct napi_struct *napi =
+		container_of(ref, struct napi_struct, notify.kref);
+	struct cpu_rmap *rmap = napi->dev->rx_cpu_rmap;
+
+	if (!napi->dev->rx_cpu_rmap_auto)
+		return;
+	rmap->obj[napi->napi_rmap_idx] = NULL;
+	napi->napi_rmap_idx = -1;
+	cpu_rmap_put(rmap);
+}
+
+static int napi_irq_cpu_rmap_add(struct napi_struct *napi, int irq)
+{
+	struct cpu_rmap *rmap = napi->dev->rx_cpu_rmap;
+	int rc;
+
+	if (!rmap)
+		return -EINVAL;
+
+	napi->notify.notify = netif_irq_cpu_rmap_notify;
+	napi->notify.release = netif_napi_affinity_release;
+	cpu_rmap_get(rmap);
+	rc = cpu_rmap_add(rmap, napi);
+	if (rc < 0)
+		goto err_add;
+
+	napi->napi_rmap_idx = rc;
+	rc = irq_set_affinity_notifier(irq, &napi->notify);
+	if (rc)
+		goto err_set;
+
+	return 0;
+
+err_set:
+	rmap->obj[napi->napi_rmap_idx] = NULL;
+	napi->napi_rmap_idx = -1;
+err_add:
+	cpu_rmap_put(rmap);
+	return rc;
+}
+
+int netif_enable_cpu_rmap(struct net_device *dev, unsigned int num_irqs)
+{
+	dev->rx_cpu_rmap = alloc_irq_cpu_rmap(num_irqs);
+	if (!dev->rx_cpu_rmap)
+		return -ENOMEM;
+
+	dev->rx_cpu_rmap_auto = true;
+	return 0;
+}
+EXPORT_SYMBOL(netif_enable_cpu_rmap);
+
+static void netif_disable_cpu_rmap(struct net_device *dev)
+{
+	struct cpu_rmap *rmap = dev->rx_cpu_rmap;
+	struct napi_struct *napi;
+	u16 index;
+
+	if (!dev->rx_cpu_rmap_auto)
+		return;
+
+	for (index = 0; index < rmap->size; index++) {
+		napi = rmap->obj[index];
+		if (napi && napi->irq > 0)
+			irq_set_affinity_notifier(napi->irq, NULL);
+	}
+
+	/* Free the rmap */
+	cpu_rmap_put(rmap);
+	dev->rx_cpu_rmap = NULL;
+	dev->rx_cpu_rmap_auto = false;
+}
+
+#else
+static int napi_irq_cpu_rmap_add(struct napi_struct *napi, int irq)
+{
+	return 0;
+}
+
+int netif_enable_cpu_rmap(struct net_device *dev, unsigned int num_irqs)
+{
+	return 0;
+}
+EXPORT_SYMBOL(netif_enable_cpu_rmap);
+
+static void netif_disable_cpu_rmap(struct net_device *dev)
+{
+}
+#endif
+
+void netif_napi_set_irq_locked(struct napi_struct *napi, int irq)
+{
+	int rc;
+
+	if (!napi->dev->rx_cpu_rmap_auto)
+		goto out;
+
+	/* Remove existing rmap entries */
+	if (napi->irq != irq && napi->irq > 0)
+		irq_set_affinity_notifier(napi->irq, NULL);
+
+	if (irq > 0) {
+		rc = napi_irq_cpu_rmap_add(napi, irq);
+		if (rc) {
+			netdev_warn(napi->dev, "Unable to update ARFS map (%d)\n",
+				    rc);
+			netif_disable_cpu_rmap(napi->dev);
+		}
+	}
+
+out:
+	napi->irq = irq;
+}
+EXPORT_SYMBOL(netif_napi_set_irq_locked);
+
 static void napi_restore_config(struct napi_struct *n)
 {
 	n->defer_hard_irqs = n->config->defer_hard_irqs;
@@ -6991,6 +7126,9 @@ void napi_disable_locked(struct napi_struct *n)
 	else
 		napi_hash_del(n);
 
+	if (n->irq > 0 && n->dev->rx_cpu_rmap_auto)
+		irq_set_affinity_notifier(n->irq, NULL);
+
 	clear_bit(NAPI_STATE_DISABLE, &n->state);
 }
 EXPORT_SYMBOL(napi_disable_locked);
@@ -11607,6 +11745,8 @@ void free_netdev(struct net_device *dev)
 	list_for_each_entry_safe(p, n, &dev->napi_list, dev_list)
 		netif_napi_del(p);
 
+	netif_disable_cpu_rmap(dev);
+
 	kvfree(dev->napi_config);
 
 	ref_tracker_dir_exit(&dev->refcnt_tracker);
-- 
2.43.0

[PATCH net-next v6 2/5] net: napi: add CPU affinity to napi_config

[Ahmed Zaki]

A common task for most drivers is to remember the user-set CPU affinity
to its IRQs. On each netdev reset, the driver should re-assign the
user's settings to the IRQs.

Add CPU affinity mask to napi_config. To delegate the CPU affinity
management to the core, drivers must:
 1 - set the new netdev flag "irq_affinity_auto":
                                       netif_enable_irq_affinity(netdev)
 2 - create the napi with persistent config:
                                       netif_napi_add_config()
 3 - bind an IRQ to the napi instance: netif_napi_set_irq()

the core will then make sure to use re-assign affinity to the napi's
IRQ.

The default IRQ mask is set to one cpu starting from the closest NUMA.

Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
---
 include/linux/netdevice.h | 14 ++++++++++-
 net/core/dev.c            | 51 +++++++++++++++++++++++++++++----------
 2 files changed, 51 insertions(+), 14 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 98259f19c627..d576e5c91c43 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -351,6 +351,7 @@ struct napi_config {
 	u64 gro_flush_timeout;
 	u64 irq_suspend_timeout;
 	u32 defer_hard_irqs;
+	cpumask_t affinity_mask;
 	unsigned int napi_id;
 };
 
@@ -393,8 +394,8 @@ struct napi_struct {
 	struct list_head	dev_list;
 	struct hlist_node	napi_hash_node;
 	int			irq;
-#ifdef CONFIG_RFS_ACCEL
 	struct irq_affinity_notify notify;
+#ifdef CONFIG_RFS_ACCEL
 	int			napi_rmap_idx;
 #endif
 	int			index;
@@ -1991,6 +1992,11 @@ enum netdev_reg_state {
  *
  *	@threaded:	napi threaded mode is enabled
  *
+ *	@irq_affinity_auto: driver wants the core to manage the IRQ affinity.
+ *			    Set by netif_enable_irq_affinity(), then driver must
+ *			    create persistent napi by netif_napi_add_config()
+ *			    and finally bind napi to IRQ (netif_napi_set_irq).
+ *
  *	@rx_cpu_rmap_auto: driver wants the core to manage the ARFS rmap.
  *	                   Set by calling netif_enable_cpu_rmap().
  *
@@ -2401,6 +2407,7 @@ struct net_device {
 	struct lock_class_key	*qdisc_tx_busylock;
 	bool			proto_down;
 	bool			threaded;
+	bool			irq_affinity_auto;
 	bool			rx_cpu_rmap_auto;
 
 	/* priv_flags_slow, ungrouped to save space */
@@ -2653,6 +2660,11 @@ static inline void netdev_set_ml_priv(struct net_device *dev,
 	dev->ml_priv_type = type;
 }
 
+static inline void netif_enable_irq_affinity(struct net_device *dev)
+{
+	dev->irq_affinity_auto = true;
+}
+
 /*
  * Net namespace inlines
  */
diff --git a/net/core/dev.c b/net/core/dev.c
index dbb63005bc2b..bc82c7f621b3 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6862,24 +6862,31 @@ void netif_queue_set_napi(struct net_device *dev, unsigned int queue_index,
 }
 EXPORT_SYMBOL(netif_queue_set_napi);
 
-#ifdef CONFIG_RFS_ACCEL
 static void
-netif_irq_cpu_rmap_notify(struct irq_affinity_notify *notify,
-			  const cpumask_t *mask)
+netif_napi_irq_notify(struct irq_affinity_notify *notify,
+		      const cpumask_t *mask)
 {
 	struct napi_struct *napi =
 		container_of(notify, struct napi_struct, notify);
+#ifdef CONFIG_RFS_ACCEL
 	struct cpu_rmap *rmap = napi->dev->rx_cpu_rmap;
 	int err;
+#endif
 
+	if (napi->config && napi->dev->irq_affinity_auto)
+		cpumask_copy(&napi->config->affinity_mask, mask);
+
+#ifdef CONFIG_RFS_ACCEL
 	if (napi->dev->rx_cpu_rmap_auto) {
 		err = cpu_rmap_update(rmap, napi->napi_rmap_idx, mask);
 		if (err)
 			pr_warn("%s: RMAP update failed (%d)\n",
 				__func__, err);
 	}
+#endif
 }
 
+#ifdef CONFIG_RFS_ACCEL
 static void netif_napi_affinity_release(struct kref *ref)
 {
 	struct napi_struct *napi =
@@ -6901,7 +6908,7 @@ static int napi_irq_cpu_rmap_add(struct napi_struct *napi, int irq)
 	if (!rmap)
 		return -EINVAL;
 
-	napi->notify.notify = netif_irq_cpu_rmap_notify;
+	napi->notify.notify = netif_napi_irq_notify;
 	napi->notify.release = netif_napi_affinity_release;
 	cpu_rmap_get(rmap);
 	rc = cpu_rmap_add(rmap, napi);
@@ -6956,6 +6963,10 @@ static void netif_disable_cpu_rmap(struct net_device *dev)
 }
 
 #else
+static void netif_napi_affinity_release(struct kref *ref)
+{
+}
+
 static int napi_irq_cpu_rmap_add(struct napi_struct *napi, int irq)
 {
 	return 0;
@@ -6976,23 +6987,28 @@ void netif_napi_set_irq_locked(struct napi_struct *napi, int irq)
 {
 	int rc;
 
-	if (!napi->dev->rx_cpu_rmap_auto)
-		goto out;
-
-	/* Remove existing rmap entries */
-	if (napi->irq != irq && napi->irq > 0)
+	/* Remove existing resources */
+	if ((napi->dev->rx_cpu_rmap_auto || napi->dev->irq_affinity_auto) &&
+	    napi->irq > 0 && napi->irq != irq)
 		irq_set_affinity_notifier(napi->irq, NULL);
 
-	if (irq > 0) {
+	if (irq > 0 && napi->dev->rx_cpu_rmap_auto) {
 		rc = napi_irq_cpu_rmap_add(napi, irq);
 		if (rc) {
 			netdev_warn(napi->dev, "Unable to update ARFS map (%d)\n",
 				    rc);
 			netif_disable_cpu_rmap(napi->dev);
 		}
+	} else if (irq > 0 && napi->config && napi->dev->irq_affinity_auto) {
+		napi->notify.notify = netif_napi_irq_notify;
+		napi->notify.release = netif_napi_affinity_release;
+
+		rc = irq_set_affinity_notifier(irq, &napi->notify);
+		if (rc)
+			netdev_warn(napi->dev, "Unable to set IRQ notifier (%d)\n",
+				    rc);
 	}
 
-out:
 	napi->irq = irq;
 }
 EXPORT_SYMBOL(netif_napi_set_irq_locked);
@@ -7002,6 +7018,10 @@ static void napi_restore_config(struct napi_struct *n)
 	n->defer_hard_irqs = n->config->defer_hard_irqs;
 	n->gro_flush_timeout = n->config->gro_flush_timeout;
 	n->irq_suspend_timeout = n->config->irq_suspend_timeout;
+
+	if (n->irq > 0 && n->dev->irq_affinity_auto)
+		irq_set_affinity(n->irq, &n->config->affinity_mask);
+
 	/* a NAPI ID might be stored in the config, if so use it. if not, use
 	 * napi_hash_add to generate one for us.
 	 */
@@ -7126,7 +7146,8 @@ void napi_disable_locked(struct napi_struct *n)
 	else
 		napi_hash_del(n);
 
-	if (n->irq > 0 && n->dev->rx_cpu_rmap_auto)
+	if (n->irq > 0 &&
+	    (n->dev->irq_affinity_auto || n->dev->rx_cpu_rmap_auto))
 		irq_set_affinity_notifier(n->irq, NULL);
 
 	clear_bit(NAPI_STATE_DISABLE, &n->state);
@@ -11585,7 +11606,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 {
 	struct net_device *dev;
 	size_t napi_config_sz;
-	unsigned int maxqs;
+	unsigned int maxqs, i, numa;
 
 	BUG_ON(strlen(name) >= sizeof(dev->name));
 
@@ -11681,6 +11702,10 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 	dev->napi_config = kvzalloc(napi_config_sz, GFP_KERNEL_ACCOUNT);
 	if (!dev->napi_config)
 		goto free_all;
+	numa = dev_to_node(&dev->dev);
+	for (i = 0; i < maxqs; i++)
+		cpumask_set_cpu(cpumask_local_spread(i, numa),
+				&dev->napi_config[i].affinity_mask);
 
 	strscpy(dev->name, name);
 	dev->name_assign_type = name_assign_type;
-- 
2.43.0

[PATCH net-next v6 3/5] bnxt: use napi's irq affinity

[Ahmed Zaki]

Delete the driver CPU affinity info and use the core's napi config
instead.

Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 25 +++--------------------
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |  2 --
 2 files changed, 3 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 13bcb055df88..2056bc098e60 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -11239,14 +11239,8 @@ static void bnxt_free_irq(struct bnxt *bp)
 		int map_idx = bnxt_cp_num_to_irq_num(bp, i);
 
 		irq = &bp->irq_tbl[map_idx];
-		if (irq->requested) {
-			if (irq->have_cpumask) {
-				irq_update_affinity_hint(irq->vector, NULL);
-				free_cpumask_var(irq->cpu_mask);
-				irq->have_cpumask = 0;
-			}
+		if (irq->requested)
 			free_irq(irq->vector, bp->bnapi[i]);
-		}
 
 		irq->requested = 0;
 	}
@@ -11275,21 +11269,6 @@ static int bnxt_request_irq(struct bnxt *bp)
 
 		netif_napi_set_irq(&bp->bnapi[i]->napi, irq->vector);
 		irq->requested = 1;
-
-		if (zalloc_cpumask_var(&irq->cpu_mask, GFP_KERNEL)) {
-			int numa_node = dev_to_node(&bp->pdev->dev);
-
-			irq->have_cpumask = 1;
-			cpumask_set_cpu(cpumask_local_spread(i, numa_node),
-					irq->cpu_mask);
-			rc = irq_update_affinity_hint(irq->vector, irq->cpu_mask);
-			if (rc) {
-				netdev_warn(bp->dev,
-					    "Update affinity hint failed, IRQ = %d\n",
-					    irq->vector);
-				break;
-			}
-		}
 	}
 	return rc;
 }
@@ -16218,6 +16197,8 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	dev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
 			    NETDEV_XDP_ACT_RX_SG;
 
+	netif_enable_irq_affinity(dev);
+
 #ifdef CONFIG_BNXT_SRIOV
 	init_waitqueue_head(&bp->sriov_cfg_wait);
 #endif
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 2373f423a523..9e6984458b46 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -1231,9 +1231,7 @@ struct bnxt_irq {
 	irq_handler_t	handler;
 	unsigned int	vector;
 	u8		requested:1;
-	u8		have_cpumask:1;
 	char		name[IFNAMSIZ + BNXT_IRQ_NAME_EXTRA];
-	cpumask_var_t	cpu_mask;
 };
 
 #define HWRM_RING_ALLOC_TX	0x1
-- 
2.43.0

[PATCH net-next v6 4/5] ice: use napi's irq affinity

[Ahmed Zaki]

Delete the driver CPU affinity info and use the core's napi config
instead.

Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h      |  3 --
 drivers/net/ethernet/intel/ice/ice_base.c |  7 +---
 drivers/net/ethernet/intel/ice/ice_lib.c  |  6 ---
 drivers/net/ethernet/intel/ice/ice_main.c | 47 ++---------------------
 4 files changed, 5 insertions(+), 58 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 71e05d30f0fd..a6e6c9e1edc1 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -478,9 +478,6 @@ struct ice_q_vector {
 	struct ice_ring_container rx;
 	struct ice_ring_container tx;
 
-	cpumask_t affinity_mask;
-	struct irq_affinity_notify affinity_notify;
-
 	struct ice_channel *ch;
 
 	char name[ICE_INT_NAME_STR_LEN];
diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
index b2af8e3586f7..86cf715de00f 100644
--- a/drivers/net/ethernet/intel/ice/ice_base.c
+++ b/drivers/net/ethernet/intel/ice/ice_base.c
@@ -147,10 +147,6 @@ static int ice_vsi_alloc_q_vector(struct ice_vsi *vsi, u16 v_idx)
 	q_vector->reg_idx = q_vector->irq.index;
 	q_vector->vf_reg_idx = q_vector->irq.index;
 
-	/* only set affinity_mask if the CPU is online */
-	if (cpu_online(v_idx))
-		cpumask_set_cpu(v_idx, &q_vector->affinity_mask);
-
 	/* This will not be called in the driver load path because the netdev
 	 * will not be created yet. All other cases with register the NAPI
 	 * handler here (i.e. resume, reset/rebuild, etc.)
@@ -276,7 +272,8 @@ static void ice_cfg_xps_tx_ring(struct ice_tx_ring *ring)
 	if (test_and_set_bit(ICE_TX_XPS_INIT_DONE, ring->xps_state))
 		return;
 
-	netif_set_xps_queue(ring->netdev, &ring->q_vector->affinity_mask,
+	netif_set_xps_queue(ring->netdev,
+			    &ring->q_vector->napi.config->affinity_mask,
 			    ring->q_index);
 }
 
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index a7d45a8ce7ac..b5b93a426933 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -2589,12 +2589,6 @@ void ice_vsi_free_irq(struct ice_vsi *vsi)
 		      vsi->q_vectors[i]->num_ring_rx))
 			continue;
 
-		/* clear the affinity notifier in the IRQ descriptor */
-		if (!IS_ENABLED(CONFIG_RFS_ACCEL))
-			irq_set_affinity_notifier(irq_num, NULL);
-
-		/* clear the affinity_hint in the IRQ descriptor */
-		irq_update_affinity_hint(irq_num, NULL);
 		synchronize_irq(irq_num);
 		devm_free_irq(ice_pf_to_dev(pf), irq_num, vsi->q_vectors[i]);
 	}
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 8fd522093dfe..4be6155855ec 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -2515,34 +2515,6 @@ int ice_schedule_reset(struct ice_pf *pf, enum ice_reset_req reset)
 	return 0;
 }
 
-/**
- * ice_irq_affinity_notify - Callback for affinity changes
- * @notify: context as to what irq was changed
- * @mask: the new affinity mask
- *
- * This is a callback function used by the irq_set_affinity_notifier function
- * so that we may register to receive changes to the irq affinity masks.
- */
-static void
-ice_irq_affinity_notify(struct irq_affinity_notify *notify,
-			const cpumask_t *mask)
-{
-	struct ice_q_vector *q_vector =
-		container_of(notify, struct ice_q_vector, affinity_notify);
-
-	cpumask_copy(&q_vector->affinity_mask, mask);
-}
-
-/**
- * ice_irq_affinity_release - Callback for affinity notifier release
- * @ref: internal core kernel usage
- *
- * This is a callback function used by the irq_set_affinity_notifier function
- * to inform the current notification subscriber that they will no longer
- * receive notifications.
- */
-static void ice_irq_affinity_release(struct kref __always_unused *ref) {}
-
 /**
  * ice_vsi_ena_irq - Enable IRQ for the given VSI
  * @vsi: the VSI being configured
@@ -2606,19 +2578,6 @@ static int ice_vsi_req_irq_msix(struct ice_vsi *vsi, char *basename)
 				   err);
 			goto free_q_irqs;
 		}
-
-		/* register for affinity change notifications */
-		if (!IS_ENABLED(CONFIG_RFS_ACCEL)) {
-			struct irq_affinity_notify *affinity_notify;
-
-			affinity_notify = &q_vector->affinity_notify;
-			affinity_notify->notify = ice_irq_affinity_notify;
-			affinity_notify->release = ice_irq_affinity_release;
-			irq_set_affinity_notifier(irq_num, affinity_notify);
-		}
-
-		/* assign the mask for this irq */
-		irq_update_affinity_hint(irq_num, &q_vector->affinity_mask);
 	}
 
 	err = ice_set_cpu_rx_rmap(vsi);
@@ -2634,9 +2593,6 @@ static int ice_vsi_req_irq_msix(struct ice_vsi *vsi, char *basename)
 free_q_irqs:
 	while (vector--) {
 		irq_num = vsi->q_vectors[vector]->irq.virq;
-		if (!IS_ENABLED(CONFIG_RFS_ACCEL))
-			irq_set_affinity_notifier(irq_num, NULL);
-		irq_update_affinity_hint(irq_num, NULL);
 		devm_free_irq(dev, irq_num, &vsi->q_vectors[vector]);
 	}
 	return err;
@@ -3677,6 +3633,9 @@ void ice_set_netdev_features(struct net_device *netdev)
 	 */
 	netdev->hw_features |= NETIF_F_RXFCS;
 
+	/* Allow core to manage IRQs affinity */
+	netif_enable_irq_affinity(netdev);
+
 	netif_set_tso_max_size(netdev, ICE_MAX_TSO_SIZE);
 }
 
-- 
2.43.0

[PATCH net-next v6 5/5] idpf: use napi's irq affinity

[Ahmed Zaki]

Delete the driver CPU affinity info and use the core's napi config
instead.

Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_lib.c  |  1 +
 drivers/net/ethernet/intel/idpf/idpf_txrx.c | 22 +++++++--------------
 drivers/net/ethernet/intel/idpf/idpf_txrx.h |  6 ++----
 3 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
index b4fbb99bfad2..d54be068f53f 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -814,6 +814,7 @@ static int idpf_cfg_netdev(struct idpf_vport *vport)
 	netdev->hw_features |= dflt_features | offloads;
 	netdev->hw_enc_features |= dflt_features | offloads;
 	idpf_set_ethtool_ops(netdev);
+	netif_enable_irq_affinity(netdev);
 	SET_NETDEV_DEV(netdev, &adapter->pdev->dev);
 
 	/* carrier off on init to avoid Tx hangs */
diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
index 2fa9c36e33c9..f6b5b45a061c 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
@@ -3554,8 +3554,6 @@ void idpf_vport_intr_rel(struct idpf_vport *vport)
 		q_vector->tx = NULL;
 		kfree(q_vector->rx);
 		q_vector->rx = NULL;
-
-		free_cpumask_var(q_vector->affinity_mask);
 	}
 
 	kfree(vport->q_vectors);
@@ -3582,8 +3580,6 @@ static void idpf_vport_intr_rel_irq(struct idpf_vport *vport)
 		vidx = vport->q_vector_idxs[vector];
 		irq_num = adapter->msix_entries[vidx].vector;
 
-		/* clear the affinity_mask in the IRQ descriptor */
-		irq_set_affinity_hint(irq_num, NULL);
 		kfree(free_irq(irq_num, q_vector));
 	}
 }
@@ -3771,8 +3767,6 @@ static int idpf_vport_intr_req_irq(struct idpf_vport *vport)
 				   "Request_irq failed, error: %d\n", err);
 			goto free_q_irqs;
 		}
-		/* assign the mask for this irq */
-		irq_set_affinity_hint(irq_num, q_vector->affinity_mask);
 	}
 
 	return 0;
@@ -4184,7 +4178,8 @@ static int idpf_vport_intr_init_vec_idx(struct idpf_vport *vport)
 static void idpf_vport_intr_napi_add_all(struct idpf_vport *vport)
 {
 	int (*napi_poll)(struct napi_struct *napi, int budget);
-	u16 v_idx;
+	u16 v_idx, qv_idx;
+	int irq_num;
 
 	if (idpf_is_queue_model_split(vport->txq_model))
 		napi_poll = idpf_vport_splitq_napi_poll;
@@ -4193,12 +4188,12 @@ static void idpf_vport_intr_napi_add_all(struct idpf_vport *vport)
 
 	for (v_idx = 0; v_idx < vport->num_q_vectors; v_idx++) {
 		struct idpf_q_vector *q_vector = &vport->q_vectors[v_idx];
+		qv_idx = vport->q_vector_idxs[v_idx];
+		irq_num = vport->adapter->msix_entries[qv_idx].vector;
 
-		netif_napi_add(vport->netdev, &q_vector->napi, napi_poll);
-
-		/* only set affinity_mask if the CPU is online */
-		if (cpu_online(v_idx))
-			cpumask_set_cpu(v_idx, q_vector->affinity_mask);
+		netif_napi_add_config(vport->netdev, &q_vector->napi,
+				      napi_poll, v_idx);
+		netif_napi_set_irq(&q_vector->napi, irq_num);
 	}
 }
 
@@ -4242,9 +4237,6 @@ int idpf_vport_intr_alloc(struct idpf_vport *vport)
 		q_vector->rx_intr_mode = IDPF_ITR_DYNAMIC;
 		q_vector->rx_itr_idx = VIRTCHNL2_ITR_IDX_0;
 
-		if (!zalloc_cpumask_var(&q_vector->affinity_mask, GFP_KERNEL))
-			goto error;
-
 		q_vector->tx = kcalloc(txqs_per_vector, sizeof(*q_vector->tx),
 				       GFP_KERNEL);
 		if (!q_vector->tx)
diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.h b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
index 0f71a6f5557b..13251f63c7c3 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_txrx.h
+++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
@@ -401,7 +401,6 @@ struct idpf_intr_reg {
  * @rx_intr_mode: Dynamic ITR or not
  * @rx_itr_idx: RX ITR index
  * @v_idx: Vector index
- * @affinity_mask: CPU affinity mask
  */
 struct idpf_q_vector {
 	__cacheline_group_begin_aligned(read_mostly);
@@ -438,13 +437,12 @@ struct idpf_q_vector {
 	__cacheline_group_begin_aligned(cold);
 	u16 v_idx;
 
-	cpumask_var_t affinity_mask;
 	__cacheline_group_end_aligned(cold);
 };
 libeth_cacheline_set_assert(struct idpf_q_vector, 120,
 			    24 + sizeof(struct napi_struct) +
 			    2 * sizeof(struct dim),
-			    8 + sizeof(cpumask_var_t));
+			    8);
 
 struct idpf_rx_queue_stats {
 	u64_stats_t packets;
@@ -940,7 +938,7 @@ static inline int idpf_q_vector_to_mem(const struct idpf_q_vector *q_vector)
 	if (!q_vector)
 		return NUMA_NO_NODE;
 
-	cpu = cpumask_first(q_vector->affinity_mask);
+	cpu = cpumask_first(&q_vector->napi.config->affinity_mask);
 
 	return cpu < nr_cpu_ids ? cpu_to_mem(cpu) : NUMA_NO_NODE;
 }
-- 
2.43.0

Re: [PATCH net-next v6 1/5] net: move ARFS rmap management to core

[Jakub Kicinski]

On Fri, 17 Jan 2025 17:33:31 -0700 Ahmed Zaki wrote:
> +#ifdef CONFIG_RFS_ACCEL
> +static void
> +netif_irq_cpu_rmap_notify(struct irq_affinity_notify *notify,
> +			  const cpumask_t *mask)
> +{
> +	struct napi_struct *napi =
> +		container_of(notify, struct napi_struct, notify);
> +	struct cpu_rmap *rmap = napi->dev->rx_cpu_rmap;
> +	int err;
> +
> +	if (napi->dev->rx_cpu_rmap_auto) {

Can this ever not be true?

> +		err = cpu_rmap_update(rmap, napi->napi_rmap_idx, mask);
> +		if (err)
> +			pr_warn("%s: RMAP update failed (%d)\n",
> +				__func__, err);

netdev_warn(napi->dev, "...) ?

> +	}
> +}
> +
> +static void netif_napi_affinity_release(struct kref *ref)
> +{
> +	struct napi_struct *napi =
> +		container_of(ref, struct napi_struct, notify.kref);
> +	struct cpu_rmap *rmap = napi->dev->rx_cpu_rmap;
> +
> +	if (!napi->dev->rx_cpu_rmap_auto)

Similar question, can it possibly be false without driver bugs?
I think you disable rmap completely if we can't add a single IRQ,
that may be too drastic. Better miss one IRQ than the whole rmap, no?

> +		return;
> +	rmap->obj[napi->napi_rmap_idx] = NULL;
> +	napi->napi_rmap_idx = -1;

Why do we modify NAPI here? Shouldn't caller be responsible for this?

> +	cpu_rmap_put(rmap);
> +}
> +
> +static int napi_irq_cpu_rmap_add(struct napi_struct *napi, int irq)
> +{
> +	struct cpu_rmap *rmap = napi->dev->rx_cpu_rmap;
> +	int rc;
> +
> +	if (!rmap)

Should never happen, I'd ignore this and let the kernel crash below.

> +		return -EINVAL;
> +
> +	napi->notify.notify = netif_irq_cpu_rmap_notify;
> +	napi->notify.release = netif_napi_affinity_release;
> +	cpu_rmap_get(rmap);
> +	rc = cpu_rmap_add(rmap, napi);
> +	if (rc < 0)
> +		goto err_add;
> +
> +	napi->napi_rmap_idx = rc;
> +	rc = irq_set_affinity_notifier(irq, &napi->notify);
> +	if (rc)
> +		goto err_set;
> +
> +	return 0;
> +
> +err_set:
> +	rmap->obj[napi->napi_rmap_idx] = NULL;
> +	napi->napi_rmap_idx = -1;
> +err_add:
> +	cpu_rmap_put(rmap);
> +	return rc;
> +}

Re: [PATCH net-next v6 2/5] net: napi: add CPU affinity to napi_config

[Jakub Kicinski]

On Fri, 17 Jan 2025 17:33:32 -0700 Ahmed Zaki wrote:
> A common task for most drivers is to remember the user-set CPU affinity
> to its IRQs. On each netdev reset, the driver should re-assign the
> user's settings to the IRQs.

> @@ -393,8 +394,8 @@ struct napi_struct {
>  	struct list_head	dev_list;
>  	struct hlist_node	napi_hash_node;
>  	int			irq;
> -#ifdef CONFIG_RFS_ACCEL
>  	struct irq_affinity_notify notify;
> +#ifdef CONFIG_RFS_ACCEL

Let's remove the #ifdef completely, one int won't make much difference,
it's ~1% of the whole struct..

> @@ -11585,7 +11606,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
>  {
>  	struct net_device *dev;
>  	size_t napi_config_sz;
> -	unsigned int maxqs;
> +	unsigned int maxqs, i, numa;

nit: reverse xmas tree

>  	BUG_ON(strlen(name) >= sizeof(dev->name));
>  
> @@ -11681,6 +11702,10 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
>  	dev->napi_config = kvzalloc(napi_config_sz, GFP_KERNEL_ACCOUNT);
>  	if (!dev->napi_config)
>  		goto free_all;

nit: empty line here

> +	numa = dev_to_node(&dev->dev);
> +	for (i = 0; i < maxqs; i++)
> +		cpumask_set_cpu(cpumask_local_spread(i, numa),
> +				&dev->napi_config[i].affinity_mask);
>  
>  	strscpy(dev->name, name);
>  	dev->name_assign_type = name_assign_type;

Re: [PATCH net-next v6 0/5] net: napi: add CPU affinity to napi->config

[Jakub Kicinski]

On Fri, 17 Jan 2025 17:33:30 -0700 Ahmed Zaki wrote:
> Drivers usually need to re-apply the user-set IRQ affinity to their IRQs
> after reset. However, since there can be only one IRQ affinity notifier
> for each IRQ, registering IRQ notifiers conflicts with the ARFS rmap
> management in the core (which also registers separate IRQ affinity
> notifiers).   
> 
> Move the IRQ affinity management to the napi struct. This way we can have
> a unified IRQ notifier to re-apply the user-set affinity and also manage
> the ARFS rmaps. Patches 1 and 2 move the ARFS rmap management to CORE.
> Patch 3 adds the IRQ affinity mask to napi_config and re-applies the mask
> after reset. Patches 4-6 use the new API for bnxt, ice and idpf drivers.

Sorry for not-super-in-depth reviews, the patches didn't apply for me :(
But feels like we are pretty close.
-- 
pw-bot: cr

Re: [PATCH net-next v6 1/5] net: move ARFS rmap management to core

[Ahmed Zaki]

On 2025-01-20 5:59 p.m., Jakub Kicinski wrote:
> On Fri, 17 Jan 2025 17:33:31 -0700 Ahmed Zaki wrote:
>> +#ifdef CONFIG_RFS_ACCEL
>> +static void
>> +netif_irq_cpu_rmap_notify(struct irq_affinity_notify *notify,
>> +			  const cpumask_t *mask)
>> +{
>> +	struct napi_struct *napi =
>> +		container_of(notify, struct napi_struct, notify);
>> +	struct cpu_rmap *rmap = napi->dev->rx_cpu_rmap;
>> +	int err;
>> +
>> +	if (napi->dev->rx_cpu_rmap_auto) {
> 
> Can this ever not be true?

It can, but not in this patch. I will it move to next one.

> 
>> +		err = cpu_rmap_update(rmap, napi->napi_rmap_idx, mask);
>> +		if (err)
>> +			pr_warn("%s: RMAP update failed (%d)\n",
>> +				__func__, err);
> 
> netdev_warn(napi->dev, "...) ?
> 
>> +	}
>> +}
>> +
>> +static void netif_napi_affinity_release(struct kref *ref)
>> +{
>> +	struct napi_struct *napi =
>> +		container_of(ref, struct napi_struct, notify.kref);
>> +	struct cpu_rmap *rmap = napi->dev->rx_cpu_rmap;
>> +
>> +	if (!napi->dev->rx_cpu_rmap_auto)
> 
> Similar question, can it possibly be false without driver bugs?
> I think you disable rmap completely if we can't add a single IRQ,
> that may be too drastic. Better miss one IRQ than the whole rmap, no?
> 

Same. This belongs to next patch (when the notifier can be set because 
of ARFS rmap or napi.irq affinity).

>> +		return;
>> +	rmap->obj[napi->napi_rmap_idx] = NULL;
>> +	napi->napi_rmap_idx = -1;
> 
> Why do we modify NAPI here? Shouldn't caller be responsible for this?
> 

This is called when the notifier is set to NULL.

Setting  napi_rmap_idx is not strictly needed but the idea was to have 
netif_napi_affinity_release() unwind all changes made in 
napi_irq_cpu_rmap_add().

Re: [PATCH net-next v6 0/5] net: napi: add CPU affinity to napi->config

[Ahmed Zaki]

On 2025-01-20 6:03 p.m., Jakub Kicinski wrote:
> On Fri, 17 Jan 2025 17:33:30 -0700 Ahmed Zaki wrote:
>> Drivers usually need to re-apply the user-set IRQ affinity to their IRQs
>> after reset. However, since there can be only one IRQ affinity notifier
>> for each IRQ, registering IRQ notifiers conflicts with the ARFS rmap
>> management in the core (which also registers separate IRQ affinity
>> notifiers).
>>
>> Move the IRQ affinity management to the napi struct. This way we can have
>> a unified IRQ notifier to re-apply the user-set affinity and also manage
>> the ARFS rmaps. Patches 1 and 2 move the ARFS rmap management to CORE.
>> Patch 3 adds the IRQ affinity mask to napi_config and re-applies the mask
>> after reset. Patches 4-6 use the new API for bnxt, ice and idpf drivers.
> 
> Sorry for not-super-in-depth reviews, the patches didn't apply for me :(
> But feels like we are pretty close.

No worries, I will rebase and send when net-next re-opens.

Thanks for the review.

Re: [PATCH net-next v6 1/5] net: move ARFS rmap management to core

[Joe Damato]

On Fri, Jan 17, 2025 at 05:33:31PM -0700, Ahmed Zaki wrote:
> Add a new netdev flag "rx_cpu_rmap_auto". Drivers supporting ARFS should
> set the flag via netif_enable_cpu_rmap() and core will allocate and manage
> the ARFS rmap. Freeing the rmap is also done by core when the netdev is
> freed.
> 
> For better IRQ affinity management, move the IRQ rmap notifier inside the
> napi_struct. Consequently, add new notify.notify and notify.release
> functions: netif_irq_cpu_rmap_notify() and netif_napi_affinity_release().
> 
> Acked-by: David Arinzon <darinzon@amazon.com>
> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>

[...]

> diff --git a/net/core/dev.c b/net/core/dev.c
> index fe5f5855593d..dbb63005bc2b 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6862,6 +6862,141 @@ void netif_queue_set_napi(struct net_device *dev, unsigned int queue_index,
>  }
>  EXPORT_SYMBOL(netif_queue_set_napi);
>  
> +#ifdef CONFIG_RFS_ACCEL
> +static void
> +netif_irq_cpu_rmap_notify(struct irq_affinity_notify *notify,
> +			  const cpumask_t *mask)
> +{
> +	struct napi_struct *napi =
> +		container_of(notify, struct napi_struct, notify);
> +	struct cpu_rmap *rmap = napi->dev->rx_cpu_rmap;
> +	int err;

I wonder if this generates a warning with some compilers? err is
defined not used if !napi->dev->rx_cpu_rmap_auto ? Not sure.

> +	if (napi->dev->rx_cpu_rmap_auto) {
> +		err = cpu_rmap_update(rmap, napi->napi_rmap_idx, mask);
> +		if (err)
> +			pr_warn("%s: RMAP update failed (%d)\n",
> +				__func__, err);
> +	}
> +}
> +
> +static void netif_napi_affinity_release(struct kref *ref)
> +{
> +	struct napi_struct *napi =
> +		container_of(ref, struct napi_struct, notify.kref);
> +	struct cpu_rmap *rmap = napi->dev->rx_cpu_rmap;
> +
> +	if (!napi->dev->rx_cpu_rmap_auto)
> +		return;
> +	rmap->obj[napi->napi_rmap_idx] = NULL;
> +	napi->napi_rmap_idx = -1;
> +	cpu_rmap_put(rmap);
> +}
> +
> +static int napi_irq_cpu_rmap_add(struct napi_struct *napi, int irq)
> +{
> +	struct cpu_rmap *rmap = napi->dev->rx_cpu_rmap;
> +	int rc;
> +
> +	if (!rmap)
> +		return -EINVAL;
> +
> +	napi->notify.notify = netif_irq_cpu_rmap_notify;
> +	napi->notify.release = netif_napi_affinity_release;

Maybe the callbacks should only be set at the end after everything
else is successful, just before the return 0 ?

> +	cpu_rmap_get(rmap);
> +	rc = cpu_rmap_add(rmap, napi);
> +	if (rc < 0)
> +		goto err_add;
> +
> +	napi->napi_rmap_idx = rc;
> +	rc = irq_set_affinity_notifier(irq, &napi->notify);
> +	if (rc)
> +		goto err_set;
> +
> +	return 0;
> +
> +err_set:
> +	rmap->obj[napi->napi_rmap_idx] = NULL;
> +	napi->napi_rmap_idx = -1;
> +err_add:
> +	cpu_rmap_put(rmap);
> +	return rc;
> +}

[...]

> +void netif_napi_set_irq_locked(struct napi_struct *napi, int irq)
> +{
> +	int rc;
> +
> +	if (!napi->dev->rx_cpu_rmap_auto)
> +		goto out;

Maybe the above if statement could be extended to be something like:

if (!napi->dev->rx_cpu_rmap_auto || napi->irq < 0)
  goto out;

then you can omit the irq > 0 checks in the code below, potentially?

> +	/* Remove existing rmap entries */
> +	if (napi->irq != irq && napi->irq > 0)
> +		irq_set_affinity_notifier(napi->irq, NULL);
> +
> +	if (irq > 0) {
> +		rc = napi_irq_cpu_rmap_add(napi, irq);
> +		if (rc) {
> +			netdev_warn(napi->dev, "Unable to update ARFS map (%d)\n",
> +				    rc);
> +			netif_disable_cpu_rmap(napi->dev);
> +		}
> +	}
> +
> +out:
> +	napi->irq = irq;
> +}
> +EXPORT_SYMBOL(netif_napi_set_irq_locked);
> +

Re: [PATCH net-next v6 1/5] net: move ARFS rmap management to core

[Ahmed Zaki]

On 2025-01-23 12:28 p.m., Joe Damato wrote:
> On Fri, Jan 17, 2025 at 05:33:31PM -0700, Ahmed Zaki wrote:
>> Add a new netdev flag "rx_cpu_rmap_auto". Drivers supporting ARFS should
>> set the flag via netif_enable_cpu_rmap() and core will allocate and manage
>> the ARFS rmap. Freeing the rmap is also done by core when the netdev is
>> freed.
>>
>> For better IRQ affinity management, move the IRQ rmap notifier inside the
>> napi_struct. Consequently, add new notify.notify and notify.release
>> functions: netif_irq_cpu_rmap_notify() and netif_napi_affinity_release().
>>
>> Acked-by: David Arinzon <darinzon@amazon.com>
>> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
> 
> [...]
> 
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index fe5f5855593d..dbb63005bc2b 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -6862,6 +6862,141 @@ void netif_queue_set_napi(struct net_device *dev, unsigned int queue_index,
>>   }
>>   EXPORT_SYMBOL(netif_queue_set_napi);
>>   
>> +#ifdef CONFIG_RFS_ACCEL
>> +static void
>> +netif_irq_cpu_rmap_notify(struct irq_affinity_notify *notify,
>> +			  const cpumask_t *mask)
>> +{
>> +	struct napi_struct *napi =
>> +		container_of(notify, struct napi_struct, notify);
>> +	struct cpu_rmap *rmap = napi->dev->rx_cpu_rmap;
>> +	int err;
> 
> I wonder if this generates a warning with some compilers? err is
> defined not used if !napi->dev->rx_cpu_rmap_auto ? Not sure.
> 
>> +	if (napi->dev->rx_cpu_rmap_auto) {
>> +		err = cpu_rmap_update(rmap, napi->napi_rmap_idx, mask);
>> +		if (err)
>> +			pr_warn("%s: RMAP update failed (%d)\n",
>> +				__func__, err);
>> +	}
>> +}
>> +
>> +static void netif_napi_affinity_release(struct kref *ref)
>> +{
>> +	struct napi_struct *napi =
>> +		container_of(ref, struct napi_struct, notify.kref);
>> +	struct cpu_rmap *rmap = napi->dev->rx_cpu_rmap;
>> +
>> +	if (!napi->dev->rx_cpu_rmap_auto)
>> +		return;
>> +	rmap->obj[napi->napi_rmap_idx] = NULL;
>> +	napi->napi_rmap_idx = -1;
>> +	cpu_rmap_put(rmap);
>> +}
>> +
>> +static int napi_irq_cpu_rmap_add(struct napi_struct *napi, int irq)
>> +{
>> +	struct cpu_rmap *rmap = napi->dev->rx_cpu_rmap;
>> +	int rc;
>> +
>> +	if (!rmap)
>> +		return -EINVAL;
>> +
>> +	napi->notify.notify = netif_irq_cpu_rmap_notify;
>> +	napi->notify.release = netif_napi_affinity_release;
> 
> Maybe the callbacks should only be set at the end after everything
> else is successful, just before the return 0 ?
> 

I believe this is needed before irq_set_affinity_notifier(), OW we could 
have some racing. I can move it there if you like.

>> +	cpu_rmap_get(rmap);
>> +	rc = cpu_rmap_add(rmap, napi);
>> +	if (rc < 0)
>> +		goto err_add;
>> +
>> +	napi->napi_rmap_idx = rc;
>> +	rc = irq_set_affinity_notifier(irq, &napi->notify);
>> +	if (rc)
>> +		goto err_set;
>> +
>> +	return 0;
>> +
>> +err_set:
>> +	rmap->obj[napi->napi_rmap_idx] = NULL;
>> +	napi->napi_rmap_idx = -1;
>> +err_add:
>> +	cpu_rmap_put(rmap);
>> +	return rc;
>> +}
> 
> [...]
> 
>> +void netif_napi_set_irq_locked(struct napi_struct *napi, int irq)
>> +{
>> +	int rc;
>> +
>> +	if (!napi->dev->rx_cpu_rmap_auto)
>> +		goto out;
> 
> Maybe the above if statement could be extended to be something like:
> 
> if (!napi->dev->rx_cpu_rmap_auto || napi->irq < 0)
>    goto out;
> 
> then you can omit the irq > 0 checks in the code below, potentially?

I am afraid I don't get this, the checks below one is for the new irq 
(could be valid or -1) and one for the existing (nap->irq).

> 
>> +	/* Remove existing rmap entries */
>> +	if (napi->irq != irq && napi->irq > 0)
>> +		irq_set_affinity_notifier(napi->irq, NULL);
>> +
>> +	if (irq > 0) {
>> +		rc = napi_irq_cpu_rmap_add(napi, irq);
>> +		if (rc) {
>> +			netdev_warn(napi->dev, "Unable to update ARFS map (%d)\n",
>> +				    rc);
>> +			netif_disable_cpu_rmap(napi->dev);
>> +		}
>> +	}
>> +
>> +out:
>> +	napi->irq = irq;
>> +}
>> +EXPORT_SYMBOL(netif_napi_set_irq_locked);
>> +

Thanks.

Re: [PATCH net-next v6 2/5] net: napi: add CPU affinity to napi_config

[Joe Damato]

On Fri, Jan 17, 2025 at 05:33:32PM -0700, Ahmed Zaki wrote:
> A common task for most drivers is to remember the user-set CPU affinity
> to its IRQs. On each netdev reset, the driver should re-assign the
> user's settings to the IRQs.
> 
> Add CPU affinity mask to napi_config. To delegate the CPU affinity
> management to the core, drivers must:
>  1 - set the new netdev flag "irq_affinity_auto":
>                                        netif_enable_irq_affinity(netdev)
>  2 - create the napi with persistent config:
>                                        netif_napi_add_config()
>  3 - bind an IRQ to the napi instance: netif_napi_set_irq()
> 
> the core will then make sure to use re-assign affinity to the napi's
> IRQ.
> 
> The default IRQ mask is set to one cpu starting from the closest NUMA.

Maybe the above is helpful to add to
Documentation/networking/napi.rst ?

> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
> ---
>  include/linux/netdevice.h | 14 ++++++++++-
>  net/core/dev.c            | 51 +++++++++++++++++++++++++++++----------
>  2 files changed, 51 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 98259f19c627..d576e5c91c43 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -351,6 +351,7 @@ struct napi_config {
>  	u64 gro_flush_timeout;
>  	u64 irq_suspend_timeout;
>  	u32 defer_hard_irqs;
> +	cpumask_t affinity_mask;
>  	unsigned int napi_id;
>  };
>  
> @@ -393,8 +394,8 @@ struct napi_struct {
>  	struct list_head	dev_list;
>  	struct hlist_node	napi_hash_node;
>  	int			irq;
> -#ifdef CONFIG_RFS_ACCEL
>  	struct irq_affinity_notify notify;
> +#ifdef CONFIG_RFS_ACCEL
>  	int			napi_rmap_idx;
>  #endif
>  	int			index;
> @@ -1991,6 +1992,11 @@ enum netdev_reg_state {
>   *
>   *	@threaded:	napi threaded mode is enabled
>   *
> + *	@irq_affinity_auto: driver wants the core to manage the IRQ affinity.
> + *			    Set by netif_enable_irq_affinity(), then driver must
> + *			    create persistent napi by netif_napi_add_config()
> + *			    and finally bind napi to IRQ (netif_napi_set_irq).
> + *
>   *	@rx_cpu_rmap_auto: driver wants the core to manage the ARFS rmap.
>   *	                   Set by calling netif_enable_cpu_rmap().
>   *
> @@ -2401,6 +2407,7 @@ struct net_device {
>  	struct lock_class_key	*qdisc_tx_busylock;
>  	bool			proto_down;
>  	bool			threaded;
> +	bool			irq_affinity_auto;
>  	bool			rx_cpu_rmap_auto;
>  
>  	/* priv_flags_slow, ungrouped to save space */
> @@ -2653,6 +2660,11 @@ static inline void netdev_set_ml_priv(struct net_device *dev,
>  	dev->ml_priv_type = type;
>  }
>  
> +static inline void netif_enable_irq_affinity(struct net_device *dev)
> +{
> +	dev->irq_affinity_auto = true;
> +}

I'll have to look at the patches which use the above function, but
the first thing that came to mind when I saw this was does the above
need a WRITE_ONCE ?

The reads below seem to be protected by a lock; I haven't yet looked
at the other patches so maybe the write is also protected by
netdev->lock ?

>  /*
>   * Net namespace inlines
>   */
> diff --git a/net/core/dev.c b/net/core/dev.c
> index dbb63005bc2b..bc82c7f621b3 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c

[...]

>  
> +#ifdef CONFIG_RFS_ACCEL
>  static void netif_napi_affinity_release(struct kref *ref)
>  {
>  	struct napi_struct *napi =
> @@ -6901,7 +6908,7 @@ static int napi_irq_cpu_rmap_add(struct napi_struct *napi, int irq)
>  	if (!rmap)
>  		return -EINVAL;
>  
> -	napi->notify.notify = netif_irq_cpu_rmap_notify;
> +	napi->notify.notify = netif_napi_irq_notify;

Same question as previous patch: does it make sense to only set the
callbacks below when all other operations have succeeded?

>  	napi->notify.release = netif_napi_affinity_release;
>  	cpu_rmap_get(rmap);
>  	rc = cpu_rmap_add(rmap, napi);

[...]

> @@ -6976,23 +6987,28 @@ void netif_napi_set_irq_locked(struct napi_struct *napi, int irq)
>  {
>  	int rc;
>  
> -	if (!napi->dev->rx_cpu_rmap_auto)
> -		goto out;
> -
> -	/* Remove existing rmap entries */
> -	if (napi->irq != irq && napi->irq > 0)
> +	/* Remove existing resources */
> +	if ((napi->dev->rx_cpu_rmap_auto || napi->dev->irq_affinity_auto) &&
> +	    napi->irq > 0 && napi->irq != irq)
>  		irq_set_affinity_notifier(napi->irq, NULL);
>  
> -	if (irq > 0) {
> +	if (irq > 0 && napi->dev->rx_cpu_rmap_auto) {
>  		rc = napi_irq_cpu_rmap_add(napi, irq);
>  		if (rc) {
>  			netdev_warn(napi->dev, "Unable to update ARFS map (%d)\n",
>  				    rc);
>  			netif_disable_cpu_rmap(napi->dev);
>  		}
> +	} else if (irq > 0 && napi->config && napi->dev->irq_affinity_auto) {
> +		napi->notify.notify = netif_napi_irq_notify;
> +		napi->notify.release = netif_napi_affinity_release;
> +
> +		rc = irq_set_affinity_notifier(irq, &napi->notify);
> +		if (rc)
> +			netdev_warn(napi->dev, "Unable to set IRQ notifier (%d)\n",
> +				    rc);

I see now that my comments on the previous patch are stale after
this patch is applied. I wonder if the "irq > 0" part can be pulled
out to simplify the branches here?

Re: [PATCH net-next v6 1/5] net: move ARFS rmap management to core

[Joe Damato]

On Thu, Jan 23, 2025 at 01:13:10PM -0700, Ahmed Zaki wrote:
> 
> 
> On 2025-01-23 12:28 p.m., Joe Damato wrote:
> > On Fri, Jan 17, 2025 at 05:33:31PM -0700, Ahmed Zaki wrote:
> > > Add a new netdev flag "rx_cpu_rmap_auto". Drivers supporting ARFS should
> > > set the flag via netif_enable_cpu_rmap() and core will allocate and manage
> > > the ARFS rmap. Freeing the rmap is also done by core when the netdev is
> > > freed.
> > > 
> > > For better IRQ affinity management, move the IRQ rmap notifier inside the
> > > napi_struct. Consequently, add new notify.notify and notify.release
> > > functions: netif_irq_cpu_rmap_notify() and netif_napi_affinity_release().
> > > 
> > > Acked-by: David Arinzon <darinzon@amazon.com>
> > > Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
> > 
> > [...]
> > 
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index fe5f5855593d..dbb63005bc2b 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -6862,6 +6862,141 @@ void netif_queue_set_napi(struct net_device *dev, unsigned int queue_index,
> > >   }
> > >   EXPORT_SYMBOL(netif_queue_set_napi);
> > > +#ifdef CONFIG_RFS_ACCEL
> > > +static void
> > > +netif_irq_cpu_rmap_notify(struct irq_affinity_notify *notify,
> > > +			  const cpumask_t *mask)
> > > +{
> > > +	struct napi_struct *napi =
> > > +		container_of(notify, struct napi_struct, notify);
> > > +	struct cpu_rmap *rmap = napi->dev->rx_cpu_rmap;
> > > +	int err;
> > 
> > I wonder if this generates a warning with some compilers? err is
> > defined not used if !napi->dev->rx_cpu_rmap_auto ? Not sure.
> > 
> > > +	if (napi->dev->rx_cpu_rmap_auto) {
> > > +		err = cpu_rmap_update(rmap, napi->napi_rmap_idx, mask);
> > > +		if (err)
> > > +			pr_warn("%s: RMAP update failed (%d)\n",
> > > +				__func__, err);
> > > +	}
> > > +}
> > > +
> > > +static void netif_napi_affinity_release(struct kref *ref)
> > > +{
> > > +	struct napi_struct *napi =
> > > +		container_of(ref, struct napi_struct, notify.kref);
> > > +	struct cpu_rmap *rmap = napi->dev->rx_cpu_rmap;
> > > +
> > > +	if (!napi->dev->rx_cpu_rmap_auto)
> > > +		return;
> > > +	rmap->obj[napi->napi_rmap_idx] = NULL;
> > > +	napi->napi_rmap_idx = -1;
> > > +	cpu_rmap_put(rmap);
> > > +}
> > > +
> > > +static int napi_irq_cpu_rmap_add(struct napi_struct *napi, int irq)
> > > +{
> > > +	struct cpu_rmap *rmap = napi->dev->rx_cpu_rmap;
> > > +	int rc;
> > > +
> > > +	if (!rmap)
> > > +		return -EINVAL;
> > > +
> > > +	napi->notify.notify = netif_irq_cpu_rmap_notify;
> > > +	napi->notify.release = netif_napi_affinity_release;
> > 
> > Maybe the callbacks should only be set at the end after everything
> > else is successful, just before the return 0 ?
> > 
> 
> I believe this is needed before irq_set_affinity_notifier(), OW we could
> have some racing. I can move it there if you like.
> 
> > > +	cpu_rmap_get(rmap);
> > > +	rc = cpu_rmap_add(rmap, napi);
> > > +	if (rc < 0)
> > > +		goto err_add;
> > > +
> > > +	napi->napi_rmap_idx = rc;
> > > +	rc = irq_set_affinity_notifier(irq, &napi->notify);
> > > +	if (rc)
> > > +		goto err_set;
> > > +
> > > +	return 0;
> > > +
> > > +err_set:
> > > +	rmap->obj[napi->napi_rmap_idx] = NULL;
> > > +	napi->napi_rmap_idx = -1;
> > > +err_add:
> > > +	cpu_rmap_put(rmap);
> > > +	return rc;
> > > +}
> > 
> > [...]
> > 
> > > +void netif_napi_set_irq_locked(struct napi_struct *napi, int irq)
> > > +{
> > > +	int rc;
> > > +
> > > +	if (!napi->dev->rx_cpu_rmap_auto)
> > > +		goto out;
> > 
> > Maybe the above if statement could be extended to be something like:
> > 
> > if (!napi->dev->rx_cpu_rmap_auto || napi->irq < 0)
> >    goto out;
> > 
> > then you can omit the irq > 0 checks in the code below, potentially?
> 
> I am afraid I don't get this, the checks below one is for the new irq (could
> be valid or -1) and one for the existing (nap->irq).

Ah yes, my mistake; I misread the other half of the if statement
below. My apologies.

> > 
> > > +	/* Remove existing rmap entries */
> > > +	if (napi->irq != irq && napi->irq > 0)
> > > +		irq_set_affinity_notifier(napi->irq, NULL);
> > > +
> > > +	if (irq > 0) {
> > > +		rc = napi_irq_cpu_rmap_add(napi, irq);
> > > +		if (rc) {
> > > +			netdev_warn(napi->dev, "Unable to update ARFS map (%d)\n",
> > > +				    rc);
> > > +			netif_disable_cpu_rmap(napi->dev);
> > > +		}
> > > +	}
> > > +
> > > +out:
> > > +	napi->irq = irq;
> > > +}
> > > +EXPORT_SYMBOL(netif_napi_set_irq_locked);
> > > +
> 
> Thanks.
> 

Re: [PATCH net-next v6 0/5] net: napi: add CPU affinity to napi->config

[Joe Damato]

On Fri, Jan 17, 2025 at 05:33:30PM -0700, Ahmed Zaki wrote:
> Drivers usually need to re-apply the user-set IRQ affinity to their IRQs
> after reset. However, since there can be only one IRQ affinity notifier
> for each IRQ, registering IRQ notifiers conflicts with the ARFS rmap
> management in the core (which also registers separate IRQ affinity
> notifiers).   
> 
> Move the IRQ affinity management to the napi struct. This way we can have
> a unified IRQ notifier to re-apply the user-set affinity and also manage
> the ARFS rmaps. Patches 1 and 2 move the ARFS rmap management to CORE.
> Patch 3 adds the IRQ affinity mask to napi_config and re-applies the mask
> after reset. Patches 4-6 use the new API for bnxt, ice and idpf drivers.

Thanks for your work on this; I like the direction this is going and
I think providing this functionality via the core is very cool.

I am hoping that once this is merged, a change can be made that
builds on this work to eliminate the duplicated

  if (!cpumask_test_cpu(cpu, affinity_mask)) 

that a few drivers have in their napi poll functions with something
more generic for drivers which have persistent NAPI configs.

Re: [PATCH net-next v6 2/5] net: napi: add CPU affinity to napi_config

[Ahmed Zaki]

On 2025-01-23 1:18 p.m., Joe Damato wrote:

Sorry for the late reply. I was pulled into other stuff and next was 
closed anyway.

> On Fri, Jan 17, 2025 at 05:33:32PM -0700, Ahmed Zaki wrote:
>> A common task for most drivers is to remember the user-set CPU affinity
>> to its IRQs. On each netdev reset, the driver should re-assign the
>> user's settings to the IRQs.
>>
>> Add CPU affinity mask to napi_config. To delegate the CPU affinity
>> management to the core, drivers must:
>>   1 - set the new netdev flag "irq_affinity_auto":
>>                                         netif_enable_irq_affinity(netdev)
>>   2 - create the napi with persistent config:
>>                                         netif_napi_add_config()
>>   3 - bind an IRQ to the napi instance: netif_napi_set_irq()
>>
>> the core will then make sure to use re-assign affinity to the napi's
>> IRQ.
>>
>> The default IRQ mask is set to one cpu starting from the closest NUMA.
> 
> Maybe the above is helpful to add to
> Documentation/networking/napi.rst ?
> 
>> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
>> ---
>>   include/linux/netdevice.h | 14 ++++++++++-
>>   net/core/dev.c            | 51 +++++++++++++++++++++++++++++----------
>>   2 files changed, 51 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 98259f19c627..d576e5c91c43 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -351,6 +351,7 @@ struct napi_config {
>>   	u64 gro_flush_timeout;
>>   	u64 irq_suspend_timeout;
>>   	u32 defer_hard_irqs;
>> +	cpumask_t affinity_mask;
>>   	unsigned int napi_id;
>>   };
>>   
>> @@ -393,8 +394,8 @@ struct napi_struct {
>>   	struct list_head	dev_list;
>>   	struct hlist_node	napi_hash_node;
>>   	int			irq;
>> -#ifdef CONFIG_RFS_ACCEL
>>   	struct irq_affinity_notify notify;
>> +#ifdef CONFIG_RFS_ACCEL
>>   	int			napi_rmap_idx;
>>   #endif
>>   	int			index;
>> @@ -1991,6 +1992,11 @@ enum netdev_reg_state {
>>    *
>>    *	@threaded:	napi threaded mode is enabled
>>    *
>> + *	@irq_affinity_auto: driver wants the core to manage the IRQ affinity.
>> + *			    Set by netif_enable_irq_affinity(), then driver must
>> + *			    create persistent napi by netif_napi_add_config()
>> + *			    and finally bind napi to IRQ (netif_napi_set_irq).
>> + *
>>    *	@rx_cpu_rmap_auto: driver wants the core to manage the ARFS rmap.
>>    *	                   Set by calling netif_enable_cpu_rmap().
>>    *
>> @@ -2401,6 +2407,7 @@ struct net_device {
>>   	struct lock_class_key	*qdisc_tx_busylock;
>>   	bool			proto_down;
>>   	bool			threaded;
>> +	bool			irq_affinity_auto;
>>   	bool			rx_cpu_rmap_auto;
>>   
>>   	/* priv_flags_slow, ungrouped to save space */
>> @@ -2653,6 +2660,11 @@ static inline void netdev_set_ml_priv(struct net_device *dev,
>>   	dev->ml_priv_type = type;
>>   }
>>   
>> +static inline void netif_enable_irq_affinity(struct net_device *dev)
>> +{
>> +	dev->irq_affinity_auto = true;
>> +}
> 
> I'll have to look at the patches which use the above function, but
> the first thing that came to mind when I saw this was does the above
> need a WRITE_ONCE ?
> 

Why?

> The reads below seem to be protected by a lock; I haven't yet looked
> at the other patches so maybe the write is also protected by
> netdev->lock ?

All functions are protected by netdev_lock/unlock. Except notifier 
callback (netif_napi_irq_notify) which is protected by the fact that all 
napis are disabled (so IRQ notifier is removed by cancel_work_sync) 
before the napis are deleted.

Other nits are OK IMO and will be fixed in the next version.

Thanks.