lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<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

Powered by Openwall GNU/*/Linux Powered by OpenVZ