[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5e9659aa-d21f-4ee9-8c0a-1d9191bbeb8c@intel.com>
Date: Tue, 14 Jan 2025 18:00:30 -0700
From: Ahmed Zaki <ahmed.zaki@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: <netdev@...r.kernel.org>, <intel-wired-lan@...ts.osuosl.org>,
<andrew+netdev@...n.ch>, <edumazet@...gle.com>, <horms@...nel.org>,
<pabeni@...hat.com>, <davem@...emloft.net>, <michael.chan@...adcom.com>,
<tariqt@...dia.com>, <anthony.l.nguyen@...el.com>,
<przemyslaw.kitszel@...el.com>, <jdamato@...tly.com>, <shayd@...dia.com>,
<akpm@...ux-foundation.org>, <shayagr@...zon.com>,
<kalesh-anakkur.purayil@...adcom.com>, <pavan.chebbi@...adcom.com>,
<yury.norov@...il.com>, <darinzon@...zon.com>
Subject: Re: [PATCH net-next v5 1/6] net: move ARFS rmap management to core
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
Powered by blists - more mailing lists