| 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
| ||
|
Message-ID: <1536b866-debe-4861-9c7b-082682bc966c@intel.com>
Date: Wed, 5 Mar 2025 07:00:44 -0700
From: Ahmed Zaki <ahmed.zaki@...el.com>
To: "Arinzon, David" <darinzon@...zon.com>, "netdev@...r.kernel.org"
<netdev@...r.kernel.org>, "jdamato@...tly.com" <jdamato@...tly.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
"andrew+netdev@...n.ch" <andrew+netdev@...n.ch>, "edumazet@...gle.com"
<edumazet@...gle.com>, "kuba@...nel.org" <kuba@...nel.org>,
"horms@...nel.org" <horms@...nel.org>, "pabeni@...hat.com"
<pabeni@...hat.com>, "davem@...emloft.net" <davem@...emloft.net>,
"michael.chan@...adcom.com" <michael.chan@...adcom.com>, "tariqt@...dia.com"
<tariqt@...dia.com>, "anthony.l.nguyen@...el.com"
<anthony.l.nguyen@...el.com>, "przemyslaw.kitszel@...el.com"
<przemyslaw.kitszel@...el.com>, "shayd@...dia.com" <shayd@...dia.com>,
"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>, "Allen, Neil"
<shayagr@...zon.com>
Subject: Re: [Intel-wired-lan] [PATCH net-next v9 2/6] net: ena: use napi's
aRFS rmap notifers
On 2025-03-04 11:33 p.m., Arinzon, David wrote:
>> [RE-SEND] I just realized I sent this only to iwl, sorry for spamming.
>>
>>
>> On 2025-03-03 10:11 a.m., Arinzon, David wrote:
>>>> Use the core's rmap notifiers and delete our own.
>>>>
>>>> Acked-by: David Arinzon <darinzon@...zon.com>
>>>> Signed-off-by: Ahmed Zaki <ahmed.zaki@...el.com>
>>>> ---
>>>> drivers/net/ethernet/amazon/ena/ena_netdev.c | 43 +-------------------
>>>> 1 file changed, 1 insertion(+), 42 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
>>>> b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>>>> index c1295dfad0d0..6aab85a7c60a 100644
>>>> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
>>>> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>>>> @@ -5,9 +5,6 @@
>>>>
>>>> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>>
>>>> -#ifdef CONFIG_RFS_ACCEL
>>>> -#include <linux/cpu_rmap.h>
>>>> -#endif /* CONFIG_RFS_ACCEL */
>>>> #include <linux/ethtool.h>
>>>> #include <linux/kernel.h>
>>>> #include <linux/module.h>
>>>> @@ -162,30 +159,6 @@ int ena_xmit_common(struct ena_adapter
>> *adapter,
>>>> return 0;
>>>> }
>>>>
>>>> -static int ena_init_rx_cpu_rmap(struct ena_adapter *adapter) -{
>>>> -#ifdef CONFIG_RFS_ACCEL
>>>> - u32 i;
>>>> - int rc;
>>>> -
>>>> - adapter->netdev->rx_cpu_rmap = alloc_irq_cpu_rmap(adapter-
>>>>> num_io_queues);
>>>> - if (!adapter->netdev->rx_cpu_rmap)
>>>> - return -ENOMEM;
>>>> - for (i = 0; i < adapter->num_io_queues; i++) {
>>>> - int irq_idx = ENA_IO_IRQ_IDX(i);
>>>> -
>>>> - rc = irq_cpu_rmap_add(adapter->netdev->rx_cpu_rmap,
>>>> - pci_irq_vector(adapter->pdev, irq_idx));
>>>> - if (rc) {
>>>> - free_irq_cpu_rmap(adapter->netdev->rx_cpu_rmap);
>>>> - adapter->netdev->rx_cpu_rmap = NULL;
>>>> - return rc;
>>>> - }
>>>> - }
>>>> -#endif /* CONFIG_RFS_ACCEL */
>>>> - return 0;
>>>> -}
>>>> -
>>>> static void ena_init_io_rings_common(struct ena_adapter *adapter,
>>>> struct ena_ring *ring, u16 qid)
>>>> { @@ -1596,7 +1569,7 @@ static int ena_enable_msix(struct ena_adapter
>> *adapter)
>>>> adapter->num_io_queues = irq_cnt - ENA_ADMIN_MSIX_VEC;
>>>> }
>>>>
>>>> - if (ena_init_rx_cpu_rmap(adapter))
>>>> + if (netif_enable_cpu_rmap(adapter->netdev,
>>>> + adapter->num_io_queues))
>>>> netif_warn(adapter, probe, adapter->netdev,
>>>> "Failed to map IRQs to CPUs\n");
>>>>
>>>> @@ -1742,13 +1715,6 @@ static void ena_free_io_irq(struct ena_adapter
>>>> *adapter)
>>>> struct ena_irq *irq;
>>>> int i;
>>>>
>>>> -#ifdef CONFIG_RFS_ACCEL
>>>> - if (adapter->msix_vecs >= 1) {
>>>> - free_irq_cpu_rmap(adapter->netdev->rx_cpu_rmap);
>>>> - adapter->netdev->rx_cpu_rmap = NULL;
>>>> - }
>>>> -#endif /* CONFIG_RFS_ACCEL */
>>>> -
>>>> for (i = ENA_IO_IRQ_FIRST_IDX; i <
>>>> ENA_MAX_MSIX_VEC(io_queue_count); i++) {
>>>> irq = &adapter->irq_tbl[i];
>>>> irq_set_affinity_hint(irq->vector, NULL); @@
>>>> -4131,13 +4097,6 @@ static void __ena_shutoff(struct pci_dev *pdev,
>> bool shutdown)
>>>> ena_dev = adapter->ena_dev;
>>>> netdev = adapter->netdev;
>>>>
>>>> -#ifdef CONFIG_RFS_ACCEL
>>>> - if ((adapter->msix_vecs >= 1) && (netdev->rx_cpu_rmap)) {
>>>> - free_irq_cpu_rmap(netdev->rx_cpu_rmap);
>>>> - netdev->rx_cpu_rmap = NULL;
>>>> - }
>>>> -
>>>> -#endif /* CONFIG_RFS_ACCEL */
>>>> /* Make sure timer and reset routine won't be called after
>>>> * freeing device resources.
>>>> */
>>>> --
>>>> 2.43.0
>>>
>>> Hi Ahmed,
>>>
>>> After the merging of this patch, I see the below stack trace when the IRQs
>> are freed.
>>> It can be reproduced by unloading and loading the driver using
>>> `modprobe -r ena; modprobe ena` (happens during unload)
>>>
>>> Based on the patchset and the changes to other drivers, I think
>>> there's a missing call to the function that releases the affinity
>>> notifier (The warn is in
>>> https://web.git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.gi
>>> t/tree/kernel/irq/manage.c#n2031)
>>>
>>> I saw in the intel code in the patchset that ` netif_napi_set_irq(<napi>, -1);`
>> is added?
>>>
>>> After adding the code snippet I don't see this anymore, but I want to
>> understand whether it's the right call by design.
>>
>> Yes, in ena_down() the IRQs are freed before napis are deleted (where IRQ
>> notifiers are released). The code below is fine (and is better IMO) but you
>> can also delete napis then free IRQs.
>>
>>
>
> Thanks for the clarification. Some book-keeping, as this change fixes the issue.
> The need to use `netif_napi_set_irq` was introduced in https://lore.kernel.org/netdev/20241002001331.65444-2-jdamato@fastly.com/,
> But, technically, there was not need to use the call with the -1 until the introduction of this patch.
> Is my understanding correct?
Correct. The new patch attaches resources (IRQ notifieres) to the napi
instance that should be released before freeing IRQs.
>
> If it's correct, then the fix is for this patch.
>
> (Also adding Joe who authored the mentioned patch)
>
I guess so since there was no need to call set_irq(-1) previoulsy.
Powered by blists - more mailing lists