Re: [Intel-wired-lan] [PATCH net-next v7 2/5] net: napi: add CPU affinity to napi_config

[Ahmed Zaki <ahmed.zaki@intel.com>]

On 2025-02-04 3:43 p.m., Joe Damato wrote:
> On Tue, Feb 04, 2025 at 03:06:19PM -0700, Ahmed Zaki wrote:
>> A common task for most drivers is to remember the user-set CPU affinity
>> to its IRQs. On each netdev reset, the driver should re-assign the
>> user's settings to the IRQs.
>>
>> Add CPU affinity mask to napi_config. To delegate the CPU affinity
>> management to the core, drivers must:
>>   1 - set the new netdev flag "irq_affinity_auto":
>>                                         netif_enable_irq_affinity(netdev)
>>   2 - create the napi with persistent config:
>>                                         netif_napi_add_config()
>>   3 - bind an IRQ to the napi instance: netif_napi_set_irq()
>>
>> the core will then make sure to use re-assign affinity to the napi's
>> IRQ.
>>
>> The default IRQ mask is set to one cpu starting from the closest NUMA.
> 
> Not sure, but maybe the above should be documented somewhere like
> Documentation/networking/napi.rst or similar?
> 
> Maybe that's too nit-picky, though, since the per-NAPI config stuff
> never made it into the docs (I'll propose a patch to fix that).


Yeah, and not all API is there (like netif_napi_set_irq()).

> 
>> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
>> ---
>>   include/linux/netdevice.h | 14 +++++++--
>>   net/core/dev.c            | 62 +++++++++++++++++++++++++++++++--------
>>   2 files changed, 61 insertions(+), 15 deletions(-)
> 
> [...]
>   
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 33e84477c9c2..4cde7ac31e74 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
> 
> [...]
> 
>> @@ -6968,17 +6983,28 @@ void netif_napi_set_irq_locked(struct napi_struct *napi, int irq)
>>   {
>>   	int rc;
>>   
>> -	/* Remove existing rmap entries */
>> -	if (napi->dev->rx_cpu_rmap_auto &&
>> +	/* Remove existing resources */
>> +	if ((napi->dev->rx_cpu_rmap_auto || napi->dev->irq_affinity_auto) &&
>>   	    napi->irq != irq && napi->irq > 0)
>>   		irq_set_affinity_notifier(napi->irq, NULL);
>>   
>>   	napi->irq = irq;
>> -	if (irq > 0) {
>> +	if (irq < 0)
>> +		return;
>> +
>> +	if (napi->dev->rx_cpu_rmap_auto) {
>>   		rc = napi_irq_cpu_rmap_add(napi, irq);
>>   		if (rc)
>>   			netdev_warn(napi->dev, "Unable to update ARFS map (%d)\n",
>>   				    rc);
>> +	} else if (napi->config && napi->dev->irq_affinity_auto) {
>> +		napi->notify.notify = netif_napi_irq_notify;
>> +		napi->notify.release = netif_napi_affinity_release;
>> +
>> +		rc = irq_set_affinity_notifier(irq, &napi->notify);
>> +		if (rc)
>> +			netdev_warn(napi->dev, "Unable to set IRQ notifier (%d)\n",
>> +				    rc);
>>   	}
> 
> Should there be a WARN_ON or WARN_ON_ONCE in here somewhere if the
> driver calls netif_napi_set_irq_locked but did not link NAPI config
> with a call to netif_napi_add_config?
> 
> It seems like in that case the driver is buggy and a warning might
> be helpful.
> 

I think that is a good idea, if there is a new version I can add this in 
the second part of the if:


if (WARN_ON_ONCE(!napi->config))
	return;