On 2025-01-14 3:08 p.m., Jakub Kicinski wrote:
On Mon, 13 Jan 2025 10:10:37 -0700 Ahmed Zaki wrote:
-#endif /* CONFIG_RFS_ACCEL */
+ return netif_enable_cpu_rmap(adapter->netdev, adapter->num_io_queues);
+#else
return 0;
+#endif /* CONFIG_RFS_ACCEL */
Let's try to eliminate some of the ifdef-ery on the driver side.
netif_enable_cpu_rmap() should simply do nothing if !CONFIG_RFS_ACCEL
@@ -2398,6 +2401,9 @@ struct net_device {
struct lock_class_key *qdisc_tx_busylock;
bool proto_down;
bool threaded;
+#ifdef CONFIG_RFS_ACCEL
+ bool rx_cpu_rmap_auto;
+#endif
similar point, don't hide it, it's just one byte and we can just leave
it as false if !CONFIG_RFS_ACCEL. It can save us a bunch of other ifdefs
Ok, makes sense.
+#ifdef CONFIG_RFS_ACCEL
+static void netif_disable_cpu_rmap(struct net_device *dev)
+{
+ free_irq_cpu_rmap(dev->rx_cpu_rmap);
+ dev->rx_cpu_rmap = NULL;
+ dev->rx_cpu_rmap_auto = false;
+}
Better do do:
static void netif_disable_cpu_rmap(struct net_device *dev)
{
#ifdef CONFIG_RFS_ACCEL
free_irq_cpu_rmap(dev->rx_cpu_rmap);
dev->rx_cpu_rmap = NULL;
dev->rx_cpu_rmap_auto = false;
#endif
}
Sure.
IOW if not relevant the function should do nothing
+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);
here you can depend on dead code elimination:
int netif_enable_cpu_rmap(struct net_device *dev, unsigned int num_irqs)
{
if (!IS_ENABLED(CONFIG_RFS_ACCEL))
return 0;
...
}
netdev->rx_cpu_rmap is declared inside #ifdef CONFIG_RFS_ACCEL, so I
still need #ifdef inside netif_enable_cpu_rmap(). I will do the same as
in netif_disable_cpu_rmap() though, that will keep the function visible.
+#endif
+
+void netif_napi_set_irq(struct napi_struct *napi, int irq)
+{
+#ifdef CONFIG_RFS_ACCEL
+ int rc;
+#endif
+ napi->irq = irq;
+
+#ifdef CONFIG_RFS_ACCEL
+ if (napi->dev->rx_cpu_rmap && napi->dev->rx_cpu_rmap_auto) {
+ rc = irq_cpu_rmap_add(napi->dev->rx_cpu_rmap, irq);
+ if (rc) {
+ netdev_warn(napi->dev, "Unable to update ARFS map
(%d)\n",
+ rc);
+ netif_disable_cpu_rmap(napi->dev);
+ }
+ }
+#endif
Declare rc inside the if to avoid the extra ifdef on variable decl
The CONFIG_RFS_ACCEL is removed in a later patch (3) when the
irq_affinity_auto is introduced and rc is re-used.
Instead, I will move "napi->irq = irq;" to the end and merge the 2
RFS_ACCL blocks.
+}
+EXPORT_SYMBOL(netif_napi_set_irq);
+
static void napi_restore_config(struct napi_struct *n)
{
n->defer_hard_irqs = n->config->defer_hard_irqs;
@@ -11421,6 +11461,10 @@ void free_netdev(struct net_device *dev)
/* Flush device addresses */
dev_addr_flush(dev);
+#ifdef CONFIG_RFS_ACCEL
+ if (dev->rx_cpu_rmap && dev->rx_cpu_rmap_auto)
don't check dev->rx_cpu_rmap, dev->rx_cpu_rmap_auto is enough
yes, also a good point.
+ netif_disable_cpu_rmap(dev);
+#endif
list_for_each_entry_safe(p, n, &dev->napi_list, dev_list)
netif_napi_del(p);
IRQs are often allocated in ndo_open and freed in ndo_stop, so
you need to catch netif_napi_del or napi_disable and remove
the IRQ from the map.
Ok, I will look into that too.
Similarly netif_napi_set_irq() may get called with -1 to clear
the IRQ number, which you currently treat at a real IRQ id, AFAICT.
correct there is no handling for irq = -1. So netif_napi_set_irq() needs
to add the irq to the rmap only if it is > 0.
I need to clarify expectation of netif_napi_set_irq() because I only see
it called with irq = -1 in napi_add_weight. But you say it can be called
with irq = -1 to "clear" the IRQ.
Does this mean that, if irq = -1, we need to "delete" the irq from rmap
if a valid irq already existed (which means this can happen as an
alternative to napi_del()/napi_diable())? or just skip adding irq to
rmap is enough?
Thanks,
Ahmed
