Re: [Intel-wired-lan] [PATCH net-next v7 2/5] net: napi: add CPU affinity to napi_config

[Ahmed Zaki]

On 2025-02-04 3:43 p.m., Joe Damato wrote:
On Tue, Feb 04, 2025 at 03:06:19PM -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.

Not sure, but maybe the above should be documented somewhere like
Documentation/networking/napi.rst or similar?

Maybe that's too nit-picky, though, since the per-NAPI config stuff
never made it into the docs (I'll propose a patch to fix that).


Yeah, and not all API is there (like netif_napi_set_irq()).


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

[...]
  
diff --git a/net/core/dev.c b/net/core/dev.c
index 33e84477c9c2..4cde7ac31e74 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

[...]

@@ -6968,17 +6983,28 @@ void netif_napi_set_irq_locked(struct napi_struct 
*napi, int irq)
  {
        int rc;
  
-	/* Remove existing rmap entries */
-       if (napi->dev->rx_cpu_rmap_auto &&
+       /* Remove existing resources */
+       if ((napi->dev->rx_cpu_rmap_auto || napi->dev->irq_affinity_auto) &&
            napi->irq != irq && napi->irq > 0)
                irq_set_affinity_notifier(napi->irq, NULL);
  
  	napi->irq = irq;
-       if (irq > 0) {
+       if (irq < 0)
+               return;
+
+       if (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);
+       } else if (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);
        }

Should there be a WARN_ON or WARN_ON_ONCE in here somewhere if the
driver calls netif_napi_set_irq_locked but did not link NAPI config
with a call to netif_napi_add_config?

It seems like in that case the driver is buggy and a warning might
be helpful.


I think that is a good idea, if there is a new version I can add this in 
the second part of the if:


if (WARN_ON_ONCE(!napi->config))
        return;