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
> +#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
}
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;
...
}
> +#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
> +}
> +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
> + 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.
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.
--
pw-bot: cr
