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 <[email protected]>
Signed-off-by: Ahmed Zaki <[email protected]>
[...]
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.
