[PATCH iwl-next v2 0/4] igb: XDP/ZC follow up

[PATCH iwl-next v2 0/4] igb: XDP/ZC follow up

[Kurt Kanzenbach]

This is a follow up for the igb XDP/ZC implementation. The first two 
patches link the IRQs and queues to NAPI instances. This is required to 
bring back the XDP/ZC busy polling support. The last patch removes 
undesired IRQs (injected via igb watchdog) while busy polling with 
napi_defer_hard_irqs and gro_flush_timeout set.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
Changes in v2:
- Take RTNL lock in PCI error handlers (Joe)
- Fix typo in commit message (Gerhard)
- Use netif_napi_add_config() (Joe)
- Link to v1: https://lore.kernel.org/r/20250210-igb_irq-v1-0-bde078cdb9df@linutronix.de

---
Kurt Kanzenbach (4):
      igb: Link IRQs to NAPI instances
      igb: Link queues to NAPI instances
      igb: Add support for persistent NAPI config
      igb: Get rid of spurious interrupts

 drivers/net/ethernet/intel/igb/igb.h      |  5 +-
 drivers/net/ethernet/intel/igb/igb_main.c | 78 +++++++++++++++++++++++++++----
 drivers/net/ethernet/intel/igb/igb_xsk.c  |  3 ++
 3 files changed, 75 insertions(+), 11 deletions(-)
---
base-commit: 0784d83df3bfc977c13252a0599be924f0afa68d
change-id: 20250206-igb_irq-f5a4d4deb207

Best regards,
-- 
Kurt Kanzenbach <kurt@linutronix.de>

[PATCH iwl-next v2 1/4] igb: Link IRQs to NAPI instances

[Kurt Kanzenbach]

Link IRQs to NAPI instances via netdev-genl API. This allows users to query
that information via netlink:

|$ ./tools/net/ynl/pyynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
|                               --dump napi-get --json='{"ifindex": 2}'
|[{'defer-hard-irqs': 0,
|  'gro-flush-timeout': 0,
|  'id': 8204,
|  'ifindex': 2,
|  'irq': 127,
|  'irq-suspend-timeout': 0},
| {'defer-hard-irqs': 0,
|  'gro-flush-timeout': 0,
|  'id': 8203,
|  'ifindex': 2,
|  'irq': 126,
|  'irq-suspend-timeout': 0},
| {'defer-hard-irqs': 0,
|  'gro-flush-timeout': 0,
|  'id': 8202,
|  'ifindex': 2,
|  'irq': 125,
|  'irq-suspend-timeout': 0},
| {'defer-hard-irqs': 0,
|  'gro-flush-timeout': 0,
|  'id': 8201,
|  'ifindex': 2,
|  'irq': 124,
|  'irq-suspend-timeout': 0}]
|$ cat /proc/interrupts | grep enp2s0
|123:          0          1 IR-PCI-MSIX-0000:02:00.0   0-edge      enp2s0
|124:          0          7 IR-PCI-MSIX-0000:02:00.0   1-edge      enp2s0-TxRx-0
|125:          0          0 IR-PCI-MSIX-0000:02:00.0   2-edge      enp2s0-TxRx-1
|126:          0          5 IR-PCI-MSIX-0000:02:00.0   3-edge      enp2s0-TxRx-2
|127:          0          0 IR-PCI-MSIX-0000:02:00.0   4-edge      enp2s0-TxRx-3

Reviewed-by: Joe Damato <jdamato@fastly.com>
Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index d368b753a4675d01b5dfa50dee4cd218e6a5e14b..d4128d19cc08f62f95682069bb5ed9b8bbbf10cb 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -947,6 +947,9 @@ static int igb_request_msix(struct igb_adapter *adapter)
 				  q_vector);
 		if (err)
 			goto err_free;
+
+		netif_napi_set_irq(&q_vector->napi,
+				   adapter->msix_entries[vector].vector);
 	}
 
 	igb_configure_msix(adapter);

-- 
2.39.5

RE: [Intel-wired-lan] [PATCH iwl-next v2 1/4] igb: Link IRQs to NAPI instances

[Rinitha, SX]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Kurt Kanzenbach
> Sent: 17 February 2025 17:01
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
> Cc: Andrew Lunn <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Sebastian Andrzej Siewior <bigeasy@linutronix.de>; Damato, Joe <jdamato@fastly.com>; Gerhard Engleder <gerhard@engleder-embedded.com>; intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Kurt Kanzenbach <kurt@linutronix.de>
> Subject: [Intel-wired-lan] [PATCH iwl-next v2 1/4] igb: Link IRQs to NAPI instances
>
> Link IRQs to NAPI instances via netdev-genl API. This allows users to query that information via netlink:
>
> |$ ./tools/net/ynl/pyynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
> |                               --dump napi-get --json='{"ifindex": 2}'
> |[{'defer-hard-irqs': 0,
> |  'gro-flush-timeout': 0,
> |  'id': 8204,
> |  'ifindex': 2,
> |  'irq': 127,
> |  'irq-suspend-timeout': 0},
> | {'defer-hard-irqs': 0,
> |  'gro-flush-timeout': 0,
> |  'id': 8203,
> |  'ifindex': 2,
> |  'irq': 126,
> |  'irq-suspend-timeout': 0},
> | {'defer-hard-irqs': 0,
> |  'gro-flush-timeout': 0,
> |  'id': 8202,
> |  'ifindex': 2,
> |  'irq': 125,
> |  'irq-suspend-timeout': 0},
> | {'defer-hard-irqs': 0,
> |  'gro-flush-timeout': 0,
> |  'id': 8201,
> |  'ifindex': 2,
> |  'irq': 124,
> |  'irq-suspend-timeout': 0}]
> |$ cat /proc/interrupts | grep enp2s0
> |123:          0          1 IR-PCI-MSIX-0000:02:00.0   0-edge      enp2s0
> |124:          0          7 IR-PCI-MSIX-0000:02:00.0   1-edge      enp2s0-TxRx-0
> |125:          0          0 IR-PCI-MSIX-0000:02:00.0   2-edge      enp2s0-TxRx-1
> |126:          0          5 IR-PCI-MSIX-0000:02:00.0   3-edge      enp2s0-TxRx-2
> |127:          0          0 IR-PCI-MSIX-0000:02:00.0   4-edge      enp2s0-TxRx-3
>
> Reviewed-by: Joe Damato <jdamato@fastly.com>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
> drivers/net/ethernet/intel/igb/igb_main.c | 3 +++
> 1 file changed, 3 insertions(+)
>

Tested-by: Rinitha S <sx.rinitha@intel.com> (A Contingent worker at Intel)

[PATCH iwl-next v2 2/4] igb: Link queues to NAPI instances

[Kurt Kanzenbach]

Link queues to NAPI instances via netdev-genl API. This is required to use
XDP/ZC busy polling. See commit 5ef44b3cb43b ("xsk: Bring back busy polling
support") for details.

This also allows users to query the info with netlink:

|$ ./tools/net/ynl/pyynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
|                               --dump queue-get --json='{"ifindex": 2}'
|[{'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'rx'},
| {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'rx'},
| {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'rx'},
| {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'rx'},
| {'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'tx'},
| {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'tx'},
| {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'tx'},
| {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'tx'}]

Add rtnl locking to PCI error handlers, because netif_queue_set_napi()
requires the lock held.

While at __igb_open() use RCT coding style.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/ethernet/intel/igb/igb.h      |  2 ++
 drivers/net/ethernet/intel/igb/igb_main.c | 43 +++++++++++++++++++++++++++----
 drivers/net/ethernet/intel/igb/igb_xsk.c  |  2 ++
 3 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 02f340280d20a6f7e32bbd3dfcbb9c1c7b4c6662..79eca385a751bfdafdf384928b6cc1b350b22560 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -722,6 +722,8 @@ enum igb_boards {
 
 extern char igb_driver_name[];
 
+void igb_set_queue_napi(struct igb_adapter *adapter, int q_idx,
+			struct napi_struct *napi);
 int igb_xmit_xdp_ring(struct igb_adapter *adapter,
 		      struct igb_ring *ring,
 		      struct xdp_frame *xdpf);
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index d4128d19cc08f62f95682069bb5ed9b8bbbf10cb..6870803a42455aa1d31f39beb027cf282064388f 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2099,6 +2099,22 @@ static void igb_check_swap_media(struct igb_adapter *adapter)
 	wr32(E1000_CTRL_EXT, ctrl_ext);
 }
 
+void igb_set_queue_napi(struct igb_adapter *adapter, int vector,
+			struct napi_struct *napi)
+{
+	struct igb_q_vector *q_vector = adapter->q_vector[vector];
+
+	if (q_vector->rx.ring)
+		netif_queue_set_napi(adapter->netdev,
+				     q_vector->rx.ring->queue_index,
+				     NETDEV_QUEUE_TYPE_RX, napi);
+
+	if (q_vector->tx.ring)
+		netif_queue_set_napi(adapter->netdev,
+				     q_vector->tx.ring->queue_index,
+				     NETDEV_QUEUE_TYPE_TX, napi);
+}
+
 /**
  *  igb_up - Open the interface and prepare it to handle traffic
  *  @adapter: board private structure
@@ -2106,6 +2122,7 @@ static void igb_check_swap_media(struct igb_adapter *adapter)
 int igb_up(struct igb_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;
+	struct napi_struct *napi;
 	int i;
 
 	/* hardware has been reset, we need to reload some things */
@@ -2113,8 +2130,11 @@ int igb_up(struct igb_adapter *adapter)
 
 	clear_bit(__IGB_DOWN, &adapter->state);
 
-	for (i = 0; i < adapter->num_q_vectors; i++)
-		napi_enable(&(adapter->q_vector[i]->napi));
+	for (i = 0; i < adapter->num_q_vectors; i++) {
+		napi = &adapter->q_vector[i]->napi;
+		napi_enable(napi);
+		igb_set_queue_napi(adapter, i, napi);
+	}
 
 	if (adapter->flags & IGB_FLAG_HAS_MSIX)
 		igb_configure_msix(adapter);
@@ -2184,6 +2204,7 @@ void igb_down(struct igb_adapter *adapter)
 	for (i = 0; i < adapter->num_q_vectors; i++) {
 		if (adapter->q_vector[i]) {
 			napi_synchronize(&adapter->q_vector[i]->napi);
+			igb_set_queue_napi(adapter, i, NULL);
 			napi_disable(&adapter->q_vector[i]->napi);
 		}
 	}
@@ -4116,8 +4137,9 @@ static int igb_sw_init(struct igb_adapter *adapter)
 static int __igb_open(struct net_device *netdev, bool resuming)
 {
 	struct igb_adapter *adapter = netdev_priv(netdev);
-	struct e1000_hw *hw = &adapter->hw;
 	struct pci_dev *pdev = adapter->pdev;
+	struct e1000_hw *hw = &adapter->hw;
+	struct napi_struct *napi;
 	int err;
 	int i;
 
@@ -4169,8 +4191,11 @@ static int __igb_open(struct net_device *netdev, bool resuming)
 	/* From here on the code is the same as igb_up() */
 	clear_bit(__IGB_DOWN, &adapter->state);
 
-	for (i = 0; i < adapter->num_q_vectors; i++)
-		napi_enable(&(adapter->q_vector[i]->napi));
+	for (i = 0; i < adapter->num_q_vectors; i++) {
+		napi = &adapter->q_vector[i]->napi;
+		napi_enable(napi);
+		igb_set_queue_napi(adapter, i, napi);
+	}
 
 	/* Clear any pending interrupts. */
 	rd32(E1000_TSICR);
@@ -9677,8 +9702,11 @@ static pci_ers_result_t igb_io_error_detected(struct pci_dev *pdev,
 	if (state == pci_channel_io_perm_failure)
 		return PCI_ERS_RESULT_DISCONNECT;
 
+	rtnl_lock();
 	if (netif_running(netdev))
 		igb_down(adapter);
+	rtnl_unlock();
+
 	pci_disable_device(pdev);
 
 	/* Request a slot reset. */
@@ -9737,16 +9765,21 @@ static void igb_io_resume(struct pci_dev *pdev)
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct igb_adapter *adapter = netdev_priv(netdev);
 
+	rtnl_lock();
 	if (netif_running(netdev)) {
 		if (!test_bit(__IGB_DOWN, &adapter->state)) {
 			dev_dbg(&pdev->dev, "Resuming from non-fatal error, do nothing.\n");
+			rtnl_unlock();
 			return;
 		}
+
 		if (igb_up(adapter)) {
 			dev_err(&pdev->dev, "igb_up failed after reset\n");
+			rtnl_unlock();
 			return;
 		}
 	}
+	rtnl_unlock();
 
 	netif_device_attach(netdev);
 
diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
index 157d43787fa0b55a74714f69e9e7903b695fcf0a..a5ad090dfe94b6afc8194fe39d28cdd51c7067b0 100644
--- a/drivers/net/ethernet/intel/igb/igb_xsk.c
+++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
@@ -45,6 +45,7 @@ static void igb_txrx_ring_disable(struct igb_adapter *adapter, u16 qid)
 	synchronize_net();
 
 	/* Rx/Tx share the same napi context. */
+	igb_set_queue_napi(adapter, qid, NULL);
 	napi_disable(&rx_ring->q_vector->napi);
 
 	igb_clean_tx_ring(tx_ring);
@@ -78,6 +79,7 @@ static void igb_txrx_ring_enable(struct igb_adapter *adapter, u16 qid)
 
 	/* Rx/Tx share the same napi context. */
 	napi_enable(&rx_ring->q_vector->napi);
+	igb_set_queue_napi(adapter, qid, &rx_ring->q_vector->napi);
 }
 
 struct xsk_buff_pool *igb_xsk_pool(struct igb_adapter *adapter,

-- 
2.39.5

Re: [PATCH iwl-next v2 2/4] igb: Link queues to NAPI instances

[Joe Damato]

On Mon, Feb 17, 2025 at 12:31:22PM +0100, Kurt Kanzenbach wrote:
> Link queues to NAPI instances via netdev-genl API. This is required to use
> XDP/ZC busy polling. See commit 5ef44b3cb43b ("xsk: Bring back busy polling
> support") for details.
> 
> This also allows users to query the info with netlink:
> 
> |$ ./tools/net/ynl/pyynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
> |                               --dump queue-get --json='{"ifindex": 2}'
> |[{'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'rx'},
> | {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'rx'},
> | {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'rx'},
> | {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'rx'},
> | {'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'tx'},
> | {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'tx'},
> | {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'tx'},
> | {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'tx'}]
> 
> Add rtnl locking to PCI error handlers, because netif_queue_set_napi()
> requires the lock held.
> 
> While at __igb_open() use RCT coding style.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>  drivers/net/ethernet/intel/igb/igb.h      |  2 ++
>  drivers/net/ethernet/intel/igb/igb_main.c | 43 +++++++++++++++++++++++++++----
>  drivers/net/ethernet/intel/igb/igb_xsk.c  |  2 ++
>  3 files changed, 42 insertions(+), 5 deletions(-)

[...]

> @@ -9737,16 +9765,21 @@ static void igb_io_resume(struct pci_dev *pdev)
>  	struct net_device *netdev = pci_get_drvdata(pdev);
>  	struct igb_adapter *adapter = netdev_priv(netdev);
>  
> +	rtnl_lock();
>  	if (netif_running(netdev)) {
>  		if (!test_bit(__IGB_DOWN, &adapter->state)) {
>  			dev_dbg(&pdev->dev, "Resuming from non-fatal error, do nothing.\n");
> +			rtnl_unlock();
>  			return;
>  		}
> +
>  		if (igb_up(adapter)) {
>  			dev_err(&pdev->dev, "igb_up failed after reset\n");
> +			rtnl_unlock();
>  			return;
>  		}
>  	}
> +	rtnl_unlock();

Does RTNL need to be held when calling netif_running()? If not, you
could probably reduce the size of the section under the lock a bit?

Otherwise, the commit looks OK to me, but I am not an IGB expert and
it is possible there is an RTNL path I missed in my review of the
previous series.

Re: [PATCH iwl-next v2 2/4] igb: Link queues to NAPI instances

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 2333 bytes --]

On Tue Feb 18 2025, Joe Damato wrote:
> On Mon, Feb 17, 2025 at 12:31:22PM +0100, Kurt Kanzenbach wrote:
>> Link queues to NAPI instances via netdev-genl API. This is required to use
>> XDP/ZC busy polling. See commit 5ef44b3cb43b ("xsk: Bring back busy polling
>> support") for details.
>> 
>> This also allows users to query the info with netlink:
>> 
>> |$ ./tools/net/ynl/pyynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
>> |                               --dump queue-get --json='{"ifindex": 2}'
>> |[{'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'rx'},
>> | {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'rx'},
>> | {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'rx'},
>> | {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'rx'},
>> | {'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'tx'},
>> | {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'tx'},
>> | {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'tx'},
>> | {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'tx'}]
>> 
>> Add rtnl locking to PCI error handlers, because netif_queue_set_napi()
>> requires the lock held.
>> 
>> While at __igb_open() use RCT coding style.
>> 
>> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
>> ---
>>  drivers/net/ethernet/intel/igb/igb.h      |  2 ++
>>  drivers/net/ethernet/intel/igb/igb_main.c | 43 +++++++++++++++++++++++++++----
>>  drivers/net/ethernet/intel/igb/igb_xsk.c  |  2 ++
>>  3 files changed, 42 insertions(+), 5 deletions(-)
>
> [...]
>
>> @@ -9737,16 +9765,21 @@ static void igb_io_resume(struct pci_dev *pdev)
>>  	struct net_device *netdev = pci_get_drvdata(pdev);
>>  	struct igb_adapter *adapter = netdev_priv(netdev);
>>  
>> +	rtnl_lock();
>>  	if (netif_running(netdev)) {
>>  		if (!test_bit(__IGB_DOWN, &adapter->state)) {
>>  			dev_dbg(&pdev->dev, "Resuming from non-fatal error, do nothing.\n");
>> +			rtnl_unlock();
>>  			return;
>>  		}
>> +
>>  		if (igb_up(adapter)) {
>>  			dev_err(&pdev->dev, "igb_up failed after reset\n");
>> +			rtnl_unlock();
>>  			return;
>>  		}
>>  	}
>> +	rtnl_unlock();
>
> Does RTNL need to be held when calling netif_running()? If not, you
> could probably reduce the size of the section under the lock a bit?

All the other instances in the driver guard the netif_running(), too.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH iwl-next v2 2/4] igb: Link queues to NAPI instances

[Joe Damato]

On Wed, Feb 19, 2025 at 08:41:01AM +0100, Kurt Kanzenbach wrote:
> On Tue Feb 18 2025, Joe Damato wrote:
> > On Mon, Feb 17, 2025 at 12:31:22PM +0100, Kurt Kanzenbach wrote:
> >> Link queues to NAPI instances via netdev-genl API. This is required to use
> >> XDP/ZC busy polling. See commit 5ef44b3cb43b ("xsk: Bring back busy polling
> >> support") for details.
> >> 
> >> This also allows users to query the info with netlink:
> >> 
> >> |$ ./tools/net/ynl/pyynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
> >> |                               --dump queue-get --json='{"ifindex": 2}'
> >> |[{'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'rx'},
> >> | {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'rx'},
> >> | {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'rx'},
> >> | {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'rx'},
> >> | {'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'tx'},
> >> | {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'tx'},
> >> | {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'tx'},
> >> | {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'tx'}]
> >> 
> >> Add rtnl locking to PCI error handlers, because netif_queue_set_napi()
> >> requires the lock held.
> >> 
> >> While at __igb_open() use RCT coding style.
> >> 
> >> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> >> ---
> >>  drivers/net/ethernet/intel/igb/igb.h      |  2 ++
> >>  drivers/net/ethernet/intel/igb/igb_main.c | 43 +++++++++++++++++++++++++++----
> >>  drivers/net/ethernet/intel/igb/igb_xsk.c  |  2 ++
> >>  3 files changed, 42 insertions(+), 5 deletions(-)
> >
> > [...]
> >
> >> @@ -9737,16 +9765,21 @@ static void igb_io_resume(struct pci_dev *pdev)
> >>  	struct net_device *netdev = pci_get_drvdata(pdev);
> >>  	struct igb_adapter *adapter = netdev_priv(netdev);
> >>  
> >> +	rtnl_lock();
> >>  	if (netif_running(netdev)) {
> >>  		if (!test_bit(__IGB_DOWN, &adapter->state)) {
> >>  			dev_dbg(&pdev->dev, "Resuming from non-fatal error, do nothing.\n");
> >> +			rtnl_unlock();
> >>  			return;
> >>  		}
> >> +
> >>  		if (igb_up(adapter)) {
> >>  			dev_err(&pdev->dev, "igb_up failed after reset\n");
> >> +			rtnl_unlock();
> >>  			return;
> >>  		}
> >>  	}
> >> +	rtnl_unlock();
> >
> > Does RTNL need to be held when calling netif_running()? If not, you
> > could probably reduce the size of the section under the lock a bit?
> 
> All the other instances in the driver guard the netif_running(), too.

OK, I spent a bit of time tracing through the paths in the igb
source. I think the v1 feedback I sent identified all the RTNL
paths, but: 
  - I am not an igb expert
  - I don't have a device to test this on
  - It is certainly possible I missed a path in my v1 analysis

The above said, as far as I can tell this patch seems fine, so:

Acked-by: Joe Damato <jdamato@fastly.com>

Re: [PATCH iwl-next v2 2/4] igb: Link queues to NAPI instances

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 3162 bytes --]

On Wed Feb 19 2025, Joe Damato wrote:
> On Wed, Feb 19, 2025 at 08:41:01AM +0100, Kurt Kanzenbach wrote:
>> On Tue Feb 18 2025, Joe Damato wrote:
>> > On Mon, Feb 17, 2025 at 12:31:22PM +0100, Kurt Kanzenbach wrote:
>> >> Link queues to NAPI instances via netdev-genl API. This is required to use
>> >> XDP/ZC busy polling. See commit 5ef44b3cb43b ("xsk: Bring back busy polling
>> >> support") for details.
>> >> 
>> >> This also allows users to query the info with netlink:
>> >> 
>> >> |$ ./tools/net/ynl/pyynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
>> >> |                               --dump queue-get --json='{"ifindex": 2}'
>> >> |[{'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'rx'},
>> >> | {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'rx'},
>> >> | {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'rx'},
>> >> | {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'rx'},
>> >> | {'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'tx'},
>> >> | {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'tx'},
>> >> | {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'tx'},
>> >> | {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'tx'}]
>> >> 
>> >> Add rtnl locking to PCI error handlers, because netif_queue_set_napi()
>> >> requires the lock held.
>> >> 
>> >> While at __igb_open() use RCT coding style.
>> >> 
>> >> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
>> >> ---
>> >>  drivers/net/ethernet/intel/igb/igb.h      |  2 ++
>> >>  drivers/net/ethernet/intel/igb/igb_main.c | 43 +++++++++++++++++++++++++++----
>> >>  drivers/net/ethernet/intel/igb/igb_xsk.c  |  2 ++
>> >>  3 files changed, 42 insertions(+), 5 deletions(-)
>> >
>> > [...]
>> >
>> >> @@ -9737,16 +9765,21 @@ static void igb_io_resume(struct pci_dev *pdev)
>> >>  	struct net_device *netdev = pci_get_drvdata(pdev);
>> >>  	struct igb_adapter *adapter = netdev_priv(netdev);
>> >>  
>> >> +	rtnl_lock();
>> >>  	if (netif_running(netdev)) {
>> >>  		if (!test_bit(__IGB_DOWN, &adapter->state)) {
>> >>  			dev_dbg(&pdev->dev, "Resuming from non-fatal error, do nothing.\n");
>> >> +			rtnl_unlock();
>> >>  			return;
>> >>  		}
>> >> +
>> >>  		if (igb_up(adapter)) {
>> >>  			dev_err(&pdev->dev, "igb_up failed after reset\n");
>> >> +			rtnl_unlock();
>> >>  			return;
>> >>  		}
>> >>  	}
>> >> +	rtnl_unlock();
>> >
>> > Does RTNL need to be held when calling netif_running()? If not, you
>> > could probably reduce the size of the section under the lock a bit?
>> 
>> All the other instances in the driver guard the netif_running(), too.
>
> OK, I spent a bit of time tracing through the paths in the igb
> source. I think the v1 feedback I sent identified all the RTNL
> paths, but: 
>   - I am not an igb expert
>   - I don't have a device to test this on

Intel i210 is one of the supported cards by igb driver. These are
relatively cheap (20-30 Euro/$) and easy to get hands on.

>   - It is certainly possible I missed a path in my v1 analysis
>
> The above said, as far as I can tell this patch seems fine, so:
>
> Acked-by: Joe Damato <jdamato@fastly.com>

Thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

RE: [Intel-wired-lan] [PATCH iwl-next v2 2/4] igb: Link queues to NAPI instances

[Rinitha, SX]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Kurt Kanzenbach
> Sent: 17 February 2025 17:01
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
> Cc: Andrew Lunn <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Sebastian Andrzej Siewior <bigeasy@linutronix.de>; Damato, Joe <jdamato@fastly.com>; Gerhard Engleder <gerhard@engleder-embedded.com>; intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Kurt Kanzenbach <kurt@linutronix.de>
> Subject: [Intel-wired-lan] [PATCH iwl-next v2 2/4] igb: Link queues to NAPI instances
>
> Link queues to NAPI instances via netdev-genl API. This is required to use XDP/ZC busy polling. See commit 5ef44b3cb43b ("xsk: Bring back busy polling
support") for details.
>
> This also allows users to query the info with netlink:
>
> |$ ./tools/net/ynl/pyynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
> |                               --dump queue-get --json='{"ifindex": 2}'
> |[{'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'rx'},
> | {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'rx'},
> | {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'rx'},
> | {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'rx'},
> | {'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'tx'},
> | {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'tx'},
> | {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'tx'},
> | {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'tx'}]
>
> Add rtnl locking to PCI error handlers, because netif_queue_set_napi() requires the lock held.
>
> While at __igb_open() use RCT coding style.
>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
> drivers/net/ethernet/intel/igb/igb.h      |  2 ++
> drivers/net/ethernet/intel/igb/igb_main.c | 43 +++++++++++++++++++++++++++----  drivers/net/ethernet/intel/igb/igb_xsk.c  |  2 ++
> 3 files changed, 42 insertions(+), 5 deletions(-)
>

Tested-by: Rinitha S <sx.rinitha@intel.com> (A Contingent worker at Intel)

Re: [PATCH iwl-next v2 2/4] igb: Link queues to NAPI instances

[Tony Nguyen]

On 2/17/2025 3:31 AM, Kurt Kanzenbach wrote:

...

> diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
> index 157d43787fa0b55a74714f69e9e7903b695fcf0a..a5ad090dfe94b6afc8194fe39d28cdd51c7067b0 100644
> --- a/drivers/net/ethernet/intel/igb/igb_xsk.c
> +++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
> @@ -45,6 +45,7 @@ static void igb_txrx_ring_disable(struct igb_adapter *adapter, u16 qid)
>   	synchronize_net();
>   
>   	/* Rx/Tx share the same napi context. */
> +	igb_set_queue_napi(adapter, qid, NULL);
>   	napi_disable(&rx_ring->q_vector->napi);
>   
>   	igb_clean_tx_ring(tx_ring);
> @@ -78,6 +79,7 @@ static void igb_txrx_ring_enable(struct igb_adapter *adapter, u16 qid)
>   
>   	/* Rx/Tx share the same napi context. */
>   	napi_enable(&rx_ring->q_vector->napi);
> +	igb_set_queue_napi(adapter, qid, &rx_ring->q_vector->napi);
>   }
>   
>   struct xsk_buff_pool *igb_xsk_pool(struct igb_adapter *adapter,

I believe Joe's fix/changes [1] need to be done here as well?

Thanks,
Tony

[1] 
https://lore.kernel.org/intel-wired-lan/9ddf6293-6cb0-47ea-a0e7-cad7d33c2535@intel.com/T/#m863614df1fb3d1980ad09016b1c9ef4e2f0b074e

Re: [PATCH iwl-next v2 2/4] igb: Link queues to NAPI instances

[Joe Damato]

On Fri, Mar 07, 2025 at 02:03:44PM -0800, Tony Nguyen wrote:
> On 2/17/2025 3:31 AM, Kurt Kanzenbach wrote:
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
> > index 157d43787fa0b55a74714f69e9e7903b695fcf0a..a5ad090dfe94b6afc8194fe39d28cdd51c7067b0 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_xsk.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
> > @@ -45,6 +45,7 @@ static void igb_txrx_ring_disable(struct igb_adapter *adapter, u16 qid)
> >   	synchronize_net();
> >   	/* Rx/Tx share the same napi context. */
> > +	igb_set_queue_napi(adapter, qid, NULL);
> >   	napi_disable(&rx_ring->q_vector->napi);
> >   	igb_clean_tx_ring(tx_ring);
> > @@ -78,6 +79,7 @@ static void igb_txrx_ring_enable(struct igb_adapter *adapter, u16 qid)
> >   	/* Rx/Tx share the same napi context. */
> >   	napi_enable(&rx_ring->q_vector->napi);
> > +	igb_set_queue_napi(adapter, qid, &rx_ring->q_vector->napi);
> >   }
> >   struct xsk_buff_pool *igb_xsk_pool(struct igb_adapter *adapter,
> 
> I believe Joe's fix/changes [1] need to be done here as well?
> 
> Thanks,
> Tony
> 
> [1] https://lore.kernel.org/intel-wired-lan/9ddf6293-6cb0-47ea-a0e7-cad7d33c2535@intel.com/T/#m863614df1fb3d1980ad09016b1c9ef4e2f0b074e

Yes, the code above should be dropped. Sorry I missed that during
review - thanks for catching that, Tony.

Kurt: when you respin this to fix what Tony mentioned, can you also
run the test mentioned above?

NETIF=eth0 ./tools/testing/selftests/drivers/net/queues.py

Re: [PATCH iwl-next v2 2/4] igb: Link queues to NAPI instances

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 1740 bytes --]

On Mon Mar 10 2025, Joe Damato wrote:
> On Fri, Mar 07, 2025 at 02:03:44PM -0800, Tony Nguyen wrote:
>> On 2/17/2025 3:31 AM, Kurt Kanzenbach wrote:
>> 
>> ...
>> 
>> > diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
>> > index 157d43787fa0b55a74714f69e9e7903b695fcf0a..a5ad090dfe94b6afc8194fe39d28cdd51c7067b0 100644
>> > --- a/drivers/net/ethernet/intel/igb/igb_xsk.c
>> > +++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
>> > @@ -45,6 +45,7 @@ static void igb_txrx_ring_disable(struct igb_adapter *adapter, u16 qid)
>> >   	synchronize_net();
>> >   	/* Rx/Tx share the same napi context. */
>> > +	igb_set_queue_napi(adapter, qid, NULL);
>> >   	napi_disable(&rx_ring->q_vector->napi);
>> >   	igb_clean_tx_ring(tx_ring);
>> > @@ -78,6 +79,7 @@ static void igb_txrx_ring_enable(struct igb_adapter *adapter, u16 qid)
>> >   	/* Rx/Tx share the same napi context. */
>> >   	napi_enable(&rx_ring->q_vector->napi);
>> > +	igb_set_queue_napi(adapter, qid, &rx_ring->q_vector->napi);
>> >   }
>> >   struct xsk_buff_pool *igb_xsk_pool(struct igb_adapter *adapter,
>> 
>> I believe Joe's fix/changes [1] need to be done here as well?
>>  
>> Thanks,
>> Tony
>> 
>> [1] https://lore.kernel.org/intel-wired-lan/9ddf6293-6cb0-47ea-a0e7-cad7d33c2535@intel.com/T/#m863614df1fb3d1980ad09016b1c9ef4e2f0b074e
>
> Yes, the code above should be dropped. Sorry I missed that during
> review - thanks for catching that, Tony.
>
> Kurt: when you respin this to fix what Tony mentioned, can you also
> run the test mentioned above?

Hi Tony & Joe,

Hm. I did run the test and it succeeded. I'll take a look at it next
week when I'm back in the office.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

[PATCH iwl-next v2 3/4] igb: Add support for persistent NAPI config

[Kurt Kanzenbach]

Use netif_napi_add_config() to assign persistent per-NAPI config.

This is useful for preserving NAPI settings when changing queue counts or
for user space programs using SO_INCOMING_NAPI_ID.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 6870803a42455aa1d31f39beb027cf282064388f..054376d648da883f35d1dee5f879487b8adfd540 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -1197,7 +1197,8 @@ static int igb_alloc_q_vector(struct igb_adapter *adapter,
 		return -ENOMEM;
 
 	/* initialize NAPI */
-	netif_napi_add(adapter->netdev, &q_vector->napi, igb_poll);
+	netif_napi_add_config(adapter->netdev, &q_vector->napi, igb_poll,
+			      v_idx);
 
 	/* tie q_vector and adapter together */
 	adapter->q_vector[v_idx] = q_vector;

-- 
2.39.5

Re: [PATCH iwl-next v2 3/4] igb: Add support for persistent NAPI config

[Joe Damato]

On Mon, Feb 17, 2025 at 12:31:23PM +0100, Kurt Kanzenbach wrote:
> Use netif_napi_add_config() to assign persistent per-NAPI config.
> 
> This is useful for preserving NAPI settings when changing queue counts or
> for user space programs using SO_INCOMING_NAPI_ID.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Thanks for adding this.

Reviewed-by: Joe Damato <jdamato@fastly.com>

RE: [Intel-wired-lan] [PATCH iwl-next v2 3/4] igb: Add support for persistent NAPI config

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Joe
> Damato
> Sent: Tuesday, February 18, 2025 10:15 PM
> To: Kurt Kanzenbach <kurt@linutronix.de>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Andrew Lunn <andrew+netdev@lunn.ch>;
> David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Sebastian Andrzej Siewior <bigeasy@linutronix.de>;
> Gerhard Engleder <gerhard@engleder-embedded.com>; intel-wired-
> lan@lists.osuosl.org; netdev@vger.kernel.org
> Subject: Re: [Intel-wired-lan] [PATCH iwl-next v2 3/4] igb: Add support for
> persistent NAPI config
> 
> On Mon, Feb 17, 2025 at 12:31:23PM +0100, Kurt Kanzenbach wrote:
> > Use netif_napi_add_config() to assign persistent per-NAPI config.
> >
> > This is useful for preserving NAPI settings when changing queue counts
> > or for user space programs using SO_INCOMING_NAPI_ID.
> >
> > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> > ---
> >  drivers/net/ethernet/intel/igb/igb_main.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> 
> Thanks for adding this.
> 
> Reviewed-by: Joe Damato <jdamato@fastly.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

RE: [Intel-wired-lan] [PATCH iwl-next v2 3/4] igb: Add support for persistent NAPI config

[Rinitha, SX]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Kurt Kanzenbach
> Sent: 17 February 2025 17:01
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
> Cc: Andrew Lunn <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Sebastian Andrzej Siewior <bigeasy@linutronix.de>; Damato, Joe <jdamato@fastly.com>; Gerhard Engleder <gerhard@engleder-embedded.com>; intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Kurt Kanzenbach <kurt@linutronix.de>
> Subject: [Intel-wired-lan] [PATCH iwl-next v2 3/4] igb: Add support for persistent NAPI config
>
> Use netif_napi_add_config() to assign persistent per-NAPI config.
> 
> This is useful for preserving NAPI settings when changing queue counts or for user space programs using SO_INCOMING_NAPI_ID.
>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
> drivers/net/ethernet/intel/igb/igb_main.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>

Tested-by: Rinitha S <sx.rinitha@intel.com> (A Contingent worker at Intel)

[PATCH iwl-next v2 4/4] igb: Get rid of spurious interrupts

[Kurt Kanzenbach]

When running the igc with XDP/ZC in busy polling mode with deferral of hard
interrupts, interrupts still happen from time to time. That is caused by
the igb task watchdog which triggers Rx interrupts periodically.

That mechanism has been introduced to overcome skb/memory allocation
failures [1]. So the Rx clean functions stop processing the Rx ring in case
of such failure. The task watchdog triggers Rx interrupts periodically in
the hope that memory became available in the mean time.

The current behavior is undesirable for real time applications, because the
driver induced Rx interrupts trigger also the softirq processing. However,
all real time packets should be processed by the application which uses the
busy polling method.

Therefore, only trigger the Rx interrupts in case of real allocation
failures. Introduce a new flag for signaling that condition.

Follow the same logic as in commit 8dcf2c212078 ("igc: Get rid of spurious
interrupts").

[1] - https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=3be507547e6177e5c808544bd6a2efa2c7f1d436

Reviewed-by: Joe Damato <jdamato@fastly.com>
Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/ethernet/intel/igb/igb.h      |  3 ++-
 drivers/net/ethernet/intel/igb/igb_main.c | 29 +++++++++++++++++++++++++----
 drivers/net/ethernet/intel/igb/igb_xsk.c  |  1 +
 3 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 79eca385a751bfdafdf384928b6cc1b350b22560..f34ead8243e9f0176a068299138c5c16f7faab2e 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -391,7 +391,8 @@ enum e1000_ring_flags_t {
 	IGB_RING_FLAG_RX_LB_VLAN_BSWAP,
 	IGB_RING_FLAG_TX_CTX_IDX,
 	IGB_RING_FLAG_TX_DETECT_HANG,
-	IGB_RING_FLAG_TX_DISABLED
+	IGB_RING_FLAG_TX_DISABLED,
+	IGB_RING_FLAG_RX_ALLOC_FAILED,
 };
 
 #define ring_uses_large_buffer(ring) \
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 054376d648da883f35d1dee5f879487b8adfd540..25abe7d8ab400f63ca9b4e87c9b5f2c15316485a 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -5755,11 +5755,29 @@ static void igb_watchdog_task(struct work_struct *work)
 	if (adapter->flags & IGB_FLAG_HAS_MSIX) {
 		u32 eics = 0;
 
-		for (i = 0; i < adapter->num_q_vectors; i++)
-			eics |= adapter->q_vector[i]->eims_value;
-		wr32(E1000_EICS, eics);
+		for (i = 0; i < adapter->num_q_vectors; i++) {
+			struct igb_q_vector *q_vector = adapter->q_vector[i];
+			struct igb_ring *rx_ring;
+
+			if (!q_vector->rx.ring)
+				continue;
+
+			rx_ring = adapter->rx_ring[q_vector->rx.ring->queue_index];
+
+			if (test_bit(IGB_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags)) {
+				eics |= q_vector->eims_value;
+				clear_bit(IGB_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags);
+			}
+		}
+		if (eics)
+			wr32(E1000_EICS, eics);
 	} else {
-		wr32(E1000_ICS, E1000_ICS_RXDMT0);
+		struct igb_ring *rx_ring = adapter->rx_ring[0];
+
+		if (test_bit(IGB_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags)) {
+			clear_bit(IGB_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags);
+			wr32(E1000_ICS, E1000_ICS_RXDMT0);
+		}
 	}
 
 	igb_spoof_check(adapter);
@@ -9090,6 +9108,7 @@ static int igb_clean_rx_irq(struct igb_q_vector *q_vector, const int budget)
 		if (!xdp_res && !skb) {
 			rx_ring->rx_stats.alloc_failed++;
 			rx_buffer->pagecnt_bias++;
+			set_bit(IGB_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags);
 			break;
 		}
 
@@ -9149,6 +9168,7 @@ static bool igb_alloc_mapped_page(struct igb_ring *rx_ring,
 	page = dev_alloc_pages(igb_rx_pg_order(rx_ring));
 	if (unlikely(!page)) {
 		rx_ring->rx_stats.alloc_failed++;
+		set_bit(IGB_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags);
 		return false;
 	}
 
@@ -9165,6 +9185,7 @@ static bool igb_alloc_mapped_page(struct igb_ring *rx_ring,
 		__free_pages(page, igb_rx_pg_order(rx_ring));
 
 		rx_ring->rx_stats.alloc_failed++;
+		set_bit(IGB_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags);
 		return false;
 	}
 
diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
index a5ad090dfe94b6afc8194fe39d28cdd51c7067b0..47344ee1ed7f29bd68055485702a87df3b8922e8 100644
--- a/drivers/net/ethernet/intel/igb/igb_xsk.c
+++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
@@ -417,6 +417,7 @@ int igb_clean_rx_irq_zc(struct igb_q_vector *q_vector,
 		/* exit if we failed to retrieve a buffer */
 		if (!skb) {
 			rx_ring->rx_stats.alloc_failed++;
+			set_bit(IGB_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags);
 			break;
 		}
 

-- 
2.39.5

RE: [Intel-wired-lan] [PATCH iwl-next v2 4/4] igb: Get rid of spurious interrupts

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Kurt Kanzenbach
> Sent: Monday, February 17, 2025 12:31 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>
> Cc: Andrew Lunn <andrew+netdev@lunn.ch>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Sebastian
> Andrzej Siewior <bigeasy@linutronix.de>; Damato, Joe
> <jdamato@fastly.com>; Gerhard Engleder <gerhard@engleder-
> embedded.com>; intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org;
> Kurt Kanzenbach <kurt@linutronix.de>
> Subject: [Intel-wired-lan] [PATCH iwl-next v2 4/4] igb: Get rid of spurious
> interrupts
> 
> When running the igc with XDP/ZC in busy polling mode with deferral of hard
> interrupts, interrupts still happen from time to time. That is caused by the igb
> task watchdog which triggers Rx interrupts periodically.
> 
> That mechanism has been introduced to overcome skb/memory allocation
> failures [1]. So the Rx clean functions stop processing the Rx ring in case of
> such failure. The task watchdog triggers Rx interrupts periodically in the hope
> that memory became available in the mean time.
> 
> The current behavior is undesirable for real time applications, because the
> driver induced Rx interrupts trigger also the softirq processing. However, all
> real time packets should be processed by the application which uses the busy
> polling method.
> 
> Therefore, only trigger the Rx interrupts in case of real allocation failures.
> Introduce a new flag for signaling that condition.
> 
> Follow the same logic as in commit 8dcf2c212078 ("igc: Get rid of spurious
> interrupts").
> 
> [1] -
> https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=3
> be507547e6177e5c808544bd6a2efa2c7f1d436
> 
> Reviewed-by: Joe Damato <jdamato@fastly.com>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

> ---
>  drivers/net/ethernet/intel/igb/igb.h      |  3 ++-
>  drivers/net/ethernet/intel/igb/igb_main.c | 29
> +++++++++++++++++++++++++----  drivers/net/ethernet/intel/igb/igb_xsk.c
> |  1 +
>  3 files changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb.h
> b/drivers/net/ethernet/intel/igb/igb.h
> index
> 79eca385a751bfdafdf384928b6cc1b350b22560..f34ead8243e9f0176a068
> 299138c5c16f7faab2e 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -391,7 +391,8 @@ enum e1000_ring_flags_t {
>  	IGB_RING_FLAG_RX_LB_VLAN_BSWAP,
>  	IGB_RING_FLAG_TX_CTX_IDX,
>  	IGB_RING_FLAG_TX_DETECT_HANG,
> -	IGB_RING_FLAG_TX_DISABLED
> +	IGB_RING_FLAG_TX_DISABLED,
> +	IGB_RING_FLAG_RX_ALLOC_FAILED,
>  };
> 
>  #define ring_uses_large_buffer(ring) \
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
> b/drivers/net/ethernet/intel/igb/igb_main.c
> index
> 054376d648da883f35d1dee5f879487b8adfd540..25abe7d8ab400f63ca9b
> 4e87c9b5f2c15316485a 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -5755,11 +5755,29 @@ static void igb_watchdog_task(struct
> work_struct *work)
>  	if (adapter->flags & IGB_FLAG_HAS_MSIX) {
>  		u32 eics = 0;
> 
> -		for (i = 0; i < adapter->num_q_vectors; i++)
> -			eics |= adapter->q_vector[i]->eims_value;
> -		wr32(E1000_EICS, eics);
> +		for (i = 0; i < adapter->num_q_vectors; i++) {
> +			struct igb_q_vector *q_vector = adapter->q_vector[i];
> +			struct igb_ring *rx_ring;
> +
> +			if (!q_vector->rx.ring)
> +				continue;
> +
> +			rx_ring = adapter->rx_ring[q_vector->rx.ring-
> >queue_index];
> +
> +			if (test_bit(IGB_RING_FLAG_RX_ALLOC_FAILED,
> &rx_ring->flags)) {
> +				eics |= q_vector->eims_value;
> +				clear_bit(IGB_RING_FLAG_RX_ALLOC_FAILED,
> &rx_ring->flags);
> +			}
> +		}
> +		if (eics)
> +			wr32(E1000_EICS, eics);
>  	} else {
> -		wr32(E1000_ICS, E1000_ICS_RXDMT0);
> +		struct igb_ring *rx_ring = adapter->rx_ring[0];
> +
> +		if (test_bit(IGB_RING_FLAG_RX_ALLOC_FAILED, &rx_ring-
> >flags)) {
> +			clear_bit(IGB_RING_FLAG_RX_ALLOC_FAILED,
> &rx_ring->flags);
> +			wr32(E1000_ICS, E1000_ICS_RXDMT0);
> +		}
>  	}
> 
>  	igb_spoof_check(adapter);
> @@ -9090,6 +9108,7 @@ static int igb_clean_rx_irq(struct igb_q_vector
> *q_vector, const int budget)
>  		if (!xdp_res && !skb) {
>  			rx_ring->rx_stats.alloc_failed++;
>  			rx_buffer->pagecnt_bias++;
> +			set_bit(IGB_RING_FLAG_RX_ALLOC_FAILED, &rx_ring-
> >flags);
>  			break;
>  		}
> 
> @@ -9149,6 +9168,7 @@ static bool igb_alloc_mapped_page(struct igb_ring
> *rx_ring,
>  	page = dev_alloc_pages(igb_rx_pg_order(rx_ring));
>  	if (unlikely(!page)) {
>  		rx_ring->rx_stats.alloc_failed++;
> +		set_bit(IGB_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags);
>  		return false;
>  	}
> 
> @@ -9165,6 +9185,7 @@ static bool igb_alloc_mapped_page(struct igb_ring
> *rx_ring,
>  		__free_pages(page, igb_rx_pg_order(rx_ring));
> 
>  		rx_ring->rx_stats.alloc_failed++;
> +		set_bit(IGB_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags);
>  		return false;
>  	}
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c
> b/drivers/net/ethernet/intel/igb/igb_xsk.c
> index
> a5ad090dfe94b6afc8194fe39d28cdd51c7067b0..47344ee1ed7f29bd68055
> 485702a87df3b8922e8 100644
> --- a/drivers/net/ethernet/intel/igb/igb_xsk.c
> +++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
> @@ -417,6 +417,7 @@ int igb_clean_rx_irq_zc(struct igb_q_vector *q_vector,
>  		/* exit if we failed to retrieve a buffer */
>  		if (!skb) {
>  			rx_ring->rx_stats.alloc_failed++;
> +			set_bit(IGB_RING_FLAG_RX_ALLOC_FAILED, &rx_ring-
> >flags);
>  			break;
>  		}
> 
> 
> --
> 2.39.5

RE: [Intel-wired-lan] [PATCH iwl-next v2 4/4] igb: Get rid of spurious interrupts

[Kumari, Sweta]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Kurt Kanzenbach
> Sent: Monday, February 17, 2025 5:01 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>
> Cc: Andrew Lunn <andrew+netdev@lunn.ch>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Sebastian
> Andrzej Siewior <bigeasy@linutronix.de>; Damato, Joe
> <jdamato@fastly.com>; Gerhard Engleder <gerhard@engleder-
> embedded.com>; intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org;
> Kurt Kanzenbach <kurt@linutronix.de>
> Subject: [Intel-wired-lan] [PATCH iwl-next v2 4/4] igb: Get rid of spurious
> interrupts
> 
> When running the igc with XDP/ZC in busy polling mode with deferral of hard
> interrupts, interrupts still happen from time to time. That is caused by the igb
> task watchdog which triggers Rx interrupts periodically.
> 
> That mechanism has been introduced to overcome skb/memory allocation
> failures [1]. So the Rx clean functions stop processing the Rx ring in case of
> such failure. The task watchdog triggers Rx interrupts periodically in the hope
> that memory became available in the mean time.
> 
> The current behavior is undesirable for real time applications, because the
> driver induced Rx interrupts trigger also the softirq processing. However, all
> real time packets should be processed by the application which uses the busy
> polling method.
> 
> Therefore, only trigger the Rx interrupts in case of real allocation failures.
> Introduce a new flag for signaling that condition.
> 
> Follow the same logic as in commit 8dcf2c212078 ("igc: Get rid of spurious
> interrupts").
> 
> [1] -
> https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=3b
> e507547e6177e5c808544bd6a2efa2c7f1d436
> 
> Reviewed-by: Joe Damato <jdamato@fastly.com>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>  drivers/net/ethernet/intel/igb/igb.h      |  3 ++-
>  drivers/net/ethernet/intel/igb/igb_main.c | 29
> +++++++++++++++++++++++++----  drivers/net/ethernet/intel/igb/igb_xsk.c  |
> 1 +
>  3 files changed, 28 insertions(+), 5 deletions(-)
> 
Tested-by: Sweta Kumari <sweta.kumari@intel.com>

Re: [PATCH iwl-next v2 0/4] igb: XDP/ZC follow up

[Joe Damato]

On Mon, Feb 17, 2025 at 12:31:20PM +0100, Kurt Kanzenbach wrote:
> This is a follow up for the igb XDP/ZC implementation. The first two 
> patches link the IRQs and queues to NAPI instances. This is required to 
> bring back the XDP/ZC busy polling support. The last patch removes 
> undesired IRQs (injected via igb watchdog) while busy polling with 
> napi_defer_hard_irqs and gro_flush_timeout set.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
> Changes in v2:
> - Take RTNL lock in PCI error handlers (Joe)
> - Fix typo in commit message (Gerhard)
> - Use netif_napi_add_config() (Joe)
> - Link to v1: https://lore.kernel.org/r/20250210-igb_irq-v1-0-bde078cdb9df@linutronix.de

Thanks for sending a v2.

My comment from the previous series still stands, which simply that
I have no idea if the maintainers will accept changes using this API
or prefer to wait until Stanislav's work [1] is completed to remove
the RTNL requirement from this API altogether.

[1]: https://lore.kernel.org/netdev/20250218020948.160643-1-sdf@fomichev.me/

Re: [PATCH iwl-next v2 0/4] igb: XDP/ZC follow up

[Joe Damato]

On Tue, Feb 18, 2025 at 04:18:19PM -0500, Joe Damato wrote:
> On Mon, Feb 17, 2025 at 12:31:20PM +0100, Kurt Kanzenbach wrote:
> > This is a follow up for the igb XDP/ZC implementation. The first two 
> > patches link the IRQs and queues to NAPI instances. This is required to 
> > bring back the XDP/ZC busy polling support. The last patch removes 
> > undesired IRQs (injected via igb watchdog) while busy polling with 
> > napi_defer_hard_irqs and gro_flush_timeout set.
> > 
> > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> > ---
> > Changes in v2:
> > - Take RTNL lock in PCI error handlers (Joe)
> > - Fix typo in commit message (Gerhard)
> > - Use netif_napi_add_config() (Joe)
> > - Link to v1: https://lore.kernel.org/r/20250210-igb_irq-v1-0-bde078cdb9df@linutronix.de
> 
> Thanks for sending a v2.
> 
> My comment from the previous series still stands, which simply that
> I have no idea if the maintainers will accept changes using this API
> or prefer to wait until Stanislav's work [1] is completed to remove
> the RTNL requirement from this API altogether.

Also, may be worth running the newly added XSK test with the NETIF
env var set to the igb device? Assuming eth0 is your igb device:

  NETIF=eth0 ./tools/testing/selftests/drivers/net/queues.py

should output:

  KTAP version 1
  1..4
  ok 1 queues.get_queues
  ok 2 queues.addremove_queues
  ok 3 queues.check_down
  ok 4 queues.check_xdp
  # Totals: pass:4 fail:0 xfail:0 xpass:0 skip:0 error:0

Note the check_xdp line above.

Re: [PATCH iwl-next v2 0/4] igb: XDP/ZC follow up

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 4084 bytes --]

On Tue Feb 18 2025, Joe Damato wrote:
> On Tue, Feb 18, 2025 at 04:18:19PM -0500, Joe Damato wrote:
>> On Mon, Feb 17, 2025 at 12:31:20PM +0100, Kurt Kanzenbach wrote:
>> > This is a follow up for the igb XDP/ZC implementation. The first two 
>> > patches link the IRQs and queues to NAPI instances. This is required to 
>> > bring back the XDP/ZC busy polling support. The last patch removes 
>> > undesired IRQs (injected via igb watchdog) while busy polling with 
>> > napi_defer_hard_irqs and gro_flush_timeout set.
>> > 
>> > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
>> > ---
>> > Changes in v2:
>> > - Take RTNL lock in PCI error handlers (Joe)
>> > - Fix typo in commit message (Gerhard)
>> > - Use netif_napi_add_config() (Joe)
>> > - Link to v1: https://lore.kernel.org/r/20250210-igb_irq-v1-0-bde078cdb9df@linutronix.de
>> 
>> Thanks for sending a v2.
>> 
>> My comment from the previous series still stands, which simply that
>> I have no idea if the maintainers will accept changes using this API
>> or prefer to wait until Stanislav's work [1] is completed to remove
>> the RTNL requirement from this API altogether.
>
> Also, may be worth running the newly added XSK test with the NETIF
> env var set to the igb device? Assuming eth0 is your igb device:
>
>   NETIF=eth0 ./tools/testing/selftests/drivers/net/queues.py
>
> should output:
>
>   KTAP version 1
>   1..4
>   ok 1 queues.get_queues
>   ok 2 queues.addremove_queues
>   ok 3 queues.check_down
>   ok 4 queues.check_xdp
>   # Totals: pass:4 fail:0 xfail:0 xpass:0 skip:0 error:0
>
> Note the check_xdp line above.
>

Sure, why not. Seems to work.

|root@apl1:~/linux# uname -a
|Linux apl1 6.14.0-rc2+ #2 SMP PREEMPT_RT Wed Feb 19 14:41:23 CET 2025 x86_64 GNU/Linux
|root@apl1:~/linux# NETIF=enp2s0 ./tools/testing/selftests/drivers/net/queues.py
|KTAP version 1
|1..4
|ok 1 queues.get_queues
|ok 2 queues.addremove_queues
|ok 3 queues.check_down
|ok 4 queues.check_xdp
|# Totals: pass:4 fail:0 xfail:0 xpass:0 skip:0 error:0

Has this xsk netlink attribute been added fairly recently? The test
failed on my kernel from a few days ago (kernel from today works). I
think there's room for improvement though:

|root@apl1:~/linux# NETIF=enp2s0 ./tools/testing/selftests/drivers/net/queues.py
|KTAP version 1
|1..4
|ok 1 queues.get_queues
|ok 2 queues.addremove_queues
|ok 3 queues.check_down
|# Exception| Traceback (most recent call last):
|# Exception|   File "/root/linux/tools/testing/selftests/net/lib/py/ksft.py", line 218, in ksft_run
|# Exception|     case(*args)
|# Exception|   File "/root/linux/./tools/testing/selftests/drivers/net/queues.py", line 53, in check_xdp
|# Exception|     ksft_eq(q['xsk'], {})
|# Exception|             ~^^^^^^^
|# Exception| KeyError: 'xsk'
|not ok 4 queues.check_xdp
|# Totals: pass:3 fail:1 xfail:0 xpass:0 skip:0 error:0

I'd assume this shouldn't be a Python exception, but rather say
something like "Expected xsk attribute, but none found. Fix the driver!" :)

While at it would you mind to add a newline to the xdp_helper usage
line (and fix the one typo)?

diff --git a/tools/testing/selftests/drivers/net/xdp_helper.c b/tools/testing/selftests/drivers/net/xdp_helper.c
index cf06a88b830b..55bad307d81b 100644
--- a/tools/testing/selftests/drivers/net/xdp_helper.c
+++ b/tools/testing/selftests/drivers/net/xdp_helper.c
@@ -20,7 +20,7 @@
  * this test program is not intended to actually process packets, but could be
  * extended in the future if that is actually needed.
  *
- * it is used by queues.py to ensure the xsk netlinux attribute is set
+ * it is used by queues.py to ensure the xsk netlink attribute is set
  * correctly.
  */
 int main(int argc, char **argv)
@@ -35,7 +35,7 @@ int main(int argc, char **argv)
        char byte;
 
        if (argc != 3) {
-               fprintf(stderr, "Usage: %s ifindex queue_id", argv[0]);
+               fprintf(stderr, "Usage: %s ifindex queue_id\n", argv[0]);
                return 1;
        }

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH iwl-next v2 0/4] igb: XDP/ZC follow up

[Joe Damato]

On Wed, Feb 19, 2025 at 03:03:36PM +0100, Kurt Kanzenbach wrote:
> On Tue Feb 18 2025, Joe Damato wrote:
> > On Tue, Feb 18, 2025 at 04:18:19PM -0500, Joe Damato wrote:
> >> On Mon, Feb 17, 2025 at 12:31:20PM +0100, Kurt Kanzenbach wrote:
> >> > This is a follow up for the igb XDP/ZC implementation. The first two 
> >> > patches link the IRQs and queues to NAPI instances. This is required to 
> >> > bring back the XDP/ZC busy polling support. The last patch removes 
> >> > undesired IRQs (injected via igb watchdog) while busy polling with 
> >> > napi_defer_hard_irqs and gro_flush_timeout set.
> >> > 
> >> > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> >> > ---
> >> > Changes in v2:
> >> > - Take RTNL lock in PCI error handlers (Joe)
> >> > - Fix typo in commit message (Gerhard)
> >> > - Use netif_napi_add_config() (Joe)
> >> > - Link to v1: https://lore.kernel.org/r/20250210-igb_irq-v1-0-bde078cdb9df@linutronix.de
> >> 
> >> Thanks for sending a v2.
> >> 
> >> My comment from the previous series still stands, which simply that
> >> I have no idea if the maintainers will accept changes using this API
> >> or prefer to wait until Stanislav's work [1] is completed to remove
> >> the RTNL requirement from this API altogether.
> >
> > Also, may be worth running the newly added XSK test with the NETIF
> > env var set to the igb device? Assuming eth0 is your igb device:
> >
> >   NETIF=eth0 ./tools/testing/selftests/drivers/net/queues.py
> >
> > should output:
> >
> >   KTAP version 1
> >   1..4
> >   ok 1 queues.get_queues
> >   ok 2 queues.addremove_queues
> >   ok 3 queues.check_down
> >   ok 4 queues.check_xdp
> >   # Totals: pass:4 fail:0 xfail:0 xpass:0 skip:0 error:0
> >
> > Note the check_xdp line above.
> >
> 
> Sure, why not. Seems to work.

Thanks for testing it.
 
> |root@apl1:~/linux# uname -a
> |Linux apl1 6.14.0-rc2+ #2 SMP PREEMPT_RT Wed Feb 19 14:41:23 CET 2025 x86_64 GNU/Linux
> |root@apl1:~/linux# NETIF=enp2s0 ./tools/testing/selftests/drivers/net/queues.py
> |KTAP version 1
> |1..4
> |ok 1 queues.get_queues
> |ok 2 queues.addremove_queues
> |ok 3 queues.check_down
> |ok 4 queues.check_xdp
> |# Totals: pass:4 fail:0 xfail:0 xpass:0 skip:0 error:0
> 
> Has this xsk netlink attribute been added fairly recently? The test
> failed on my kernel from a few days ago (kernel from today works).

Yes, it was just merged, see the commit date here:

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=788e52e2b66844301fe09f3372d46d8c62f6ebe4

> I think there's room for improvement though:
> 
> |root@apl1:~/linux# NETIF=enp2s0 ./tools/testing/selftests/drivers/net/queues.py
> |KTAP version 1
> |1..4
> |ok 1 queues.get_queues
> |ok 2 queues.addremove_queues
> |ok 3 queues.check_down
> |# Exception| Traceback (most recent call last):
> |# Exception|   File "/root/linux/tools/testing/selftests/net/lib/py/ksft.py", line 218, in ksft_run
> |# Exception|     case(*args)
> |# Exception|   File "/root/linux/./tools/testing/selftests/drivers/net/queues.py", line 53, in check_xdp
> |# Exception|     ksft_eq(q['xsk'], {})
> |# Exception|             ~^^^^^^^
> |# Exception| KeyError: 'xsk'
> |not ok 4 queues.check_xdp
> |# Totals: pass:3 fail:1 xfail:0 xpass:0 skip:0 error:0
> 
> I'd assume this shouldn't be a Python exception, but rather say
> something like "Expected xsk attribute, but none found. Fix the driver!" :)
> 
> While at it would you mind to add a newline to the xdp_helper usage
> line (and fix the one typo)?

Jakub currently has a series out to change the test a bit and
improve it overall, see:

  https://lore.kernel.org/netdev/20250218195048.74692-1-kuba@kernel.org/

It looks like your concerns (the typo, newline, and better error)
may still remain. If so, I can submit a follow-up once his work has
been merged to address your concerns - unless you'd like to do
that?

Re: [PATCH iwl-next v2 0/4] igb: XDP/ZC follow up

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 3982 bytes --]

On Wed Feb 19 2025, Joe Damato wrote:
> On Wed, Feb 19, 2025 at 03:03:36PM +0100, Kurt Kanzenbach wrote:
>> On Tue Feb 18 2025, Joe Damato wrote:
>> > On Tue, Feb 18, 2025 at 04:18:19PM -0500, Joe Damato wrote:
>> >> On Mon, Feb 17, 2025 at 12:31:20PM +0100, Kurt Kanzenbach wrote:
>> >> > This is a follow up for the igb XDP/ZC implementation. The first two 
>> >> > patches link the IRQs and queues to NAPI instances. This is required to 
>> >> > bring back the XDP/ZC busy polling support. The last patch removes 
>> >> > undesired IRQs (injected via igb watchdog) while busy polling with 
>> >> > napi_defer_hard_irqs and gro_flush_timeout set.
>> >> > 
>> >> > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
>> >> > ---
>> >> > Changes in v2:
>> >> > - Take RTNL lock in PCI error handlers (Joe)
>> >> > - Fix typo in commit message (Gerhard)
>> >> > - Use netif_napi_add_config() (Joe)
>> >> > - Link to v1: https://lore.kernel.org/r/20250210-igb_irq-v1-0-bde078cdb9df@linutronix.de
>> >> 
>> >> Thanks for sending a v2.
>> >> 
>> >> My comment from the previous series still stands, which simply that
>> >> I have no idea if the maintainers will accept changes using this API
>> >> or prefer to wait until Stanislav's work [1] is completed to remove
>> >> the RTNL requirement from this API altogether.
>> >
>> > Also, may be worth running the newly added XSK test with the NETIF
>> > env var set to the igb device? Assuming eth0 is your igb device:
>> >
>> >   NETIF=eth0 ./tools/testing/selftests/drivers/net/queues.py
>> >
>> > should output:
>> >
>> >   KTAP version 1
>> >   1..4
>> >   ok 1 queues.get_queues
>> >   ok 2 queues.addremove_queues
>> >   ok 3 queues.check_down
>> >   ok 4 queues.check_xdp
>> >   # Totals: pass:4 fail:0 xfail:0 xpass:0 skip:0 error:0
>> >
>> > Note the check_xdp line above.
>> >
>> 
>> Sure, why not. Seems to work.
>
> Thanks for testing it.
>  
>> |root@apl1:~/linux# uname -a
>> |Linux apl1 6.14.0-rc2+ #2 SMP PREEMPT_RT Wed Feb 19 14:41:23 CET 2025 x86_64 GNU/Linux
>> |root@apl1:~/linux# NETIF=enp2s0 ./tools/testing/selftests/drivers/net/queues.py
>> |KTAP version 1
>> |1..4
>> |ok 1 queues.get_queues
>> |ok 2 queues.addremove_queues
>> |ok 3 queues.check_down
>> |ok 4 queues.check_xdp
>> |# Totals: pass:4 fail:0 xfail:0 xpass:0 skip:0 error:0
>> 
>> Has this xsk netlink attribute been added fairly recently? The test
>> failed on my kernel from a few days ago (kernel from today works).
>
> Yes, it was just merged, see the commit date here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=788e52e2b66844301fe09f3372d46d8c62f6ebe4
>
>> I think there's room for improvement though:
>> 
>> |root@apl1:~/linux# NETIF=enp2s0 ./tools/testing/selftests/drivers/net/queues.py
>> |KTAP version 1
>> |1..4
>> |ok 1 queues.get_queues
>> |ok 2 queues.addremove_queues
>> |ok 3 queues.check_down
>> |# Exception| Traceback (most recent call last):
>> |# Exception|   File "/root/linux/tools/testing/selftests/net/lib/py/ksft.py", line 218, in ksft_run
>> |# Exception|     case(*args)
>> |# Exception|   File "/root/linux/./tools/testing/selftests/drivers/net/queues.py", line 53, in check_xdp
>> |# Exception|     ksft_eq(q['xsk'], {})
>> |# Exception|             ~^^^^^^^
>> |# Exception| KeyError: 'xsk'
>> |not ok 4 queues.check_xdp
>> |# Totals: pass:3 fail:1 xfail:0 xpass:0 skip:0 error:0
>> 
>> I'd assume this shouldn't be a Python exception, but rather say
>> something like "Expected xsk attribute, but none found. Fix the driver!" :)
>> 
>> While at it would you mind to add a newline to the xdp_helper usage
>> line (and fix the one typo)?
>
> Jakub currently has a series out to change the test a bit and
> improve it overall, see:
>
>   https://lore.kernel.org/netdev/20250218195048.74692-1-kuba@kernel.org/
>

Great that Jakub is already on it. I've replied in that thread. Thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH iwl-next v2 0/4] igb: XDP/ZC follow up

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 1571 bytes --]

On Tue Feb 18 2025, Joe Damato wrote:
> On Mon, Feb 17, 2025 at 12:31:20PM +0100, Kurt Kanzenbach wrote:
>> This is a follow up for the igb XDP/ZC implementation. The first two 
>> patches link the IRQs and queues to NAPI instances. This is required to 
>> bring back the XDP/ZC busy polling support. The last patch removes 
>> undesired IRQs (injected via igb watchdog) while busy polling with 
>> napi_defer_hard_irqs and gro_flush_timeout set.
>> 
>> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
>> ---
>> Changes in v2:
>> - Take RTNL lock in PCI error handlers (Joe)
>> - Fix typo in commit message (Gerhard)
>> - Use netif_napi_add_config() (Joe)
>> - Link to v1: https://lore.kernel.org/r/20250210-igb_irq-v1-0-bde078cdb9df@linutronix.de
>
> Thanks for sending a v2.

Thanks for the review.

>
> My comment from the previous series still stands, which simply that
> I have no idea if the maintainers will accept changes using this API
> or prefer to wait until Stanislav's work [1] is completed to remove
> the RTNL requirement from this API altogether.

I'd rather consider patch #2 a bugfix to restore the busy polling with
XDP/ZC. After commit 5ef44b3cb43b ("xsk: Bring back busy polling
support") it is a requirement to implement this API.

The maintainers didn't speak up on v1, so i went along and sent v2.

@Jakub: What's your preference? Would you accept this series or rather
like to wait for Stanislav's work to be finished?

>
> [1]: https://lore.kernel.org/netdev/20250218020948.160643-1-sdf@fomichev.me/
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH iwl-next v2 0/4] igb: XDP/ZC follow up

[Jakub Kicinski]

On Wed, 19 Feb 2025 08:39:08 +0100 Kurt Kanzenbach wrote:
> > My comment from the previous series still stands, which simply that
> > I have no idea if the maintainers will accept changes using this API
> > or prefer to wait until Stanislav's work [1] is completed to remove
> > the RTNL requirement from this API altogether.  
> 
> I'd rather consider patch #2 a bugfix to restore the busy polling with
> XDP/ZC. After commit 5ef44b3cb43b ("xsk: Bring back busy polling
> support") it is a requirement to implement this API.
> 
> The maintainers didn't speak up on v1, so i went along and sent v2.
> 
> @Jakub: What's your preference? Would you accept this series or rather
> like to wait for Stanislav's work to be finished?

No strong preference. If rtnl_lock is not causing any issues 
in this driver, the we can merge as is. I haven't followed 
the past discussions, tho.

Re: [PATCH iwl-next v2 0/4] igb: XDP/ZC follow up

[Joe Damato]

On Wed, Feb 19, 2025 at 06:06:51PM -0800, Jakub Kicinski wrote:
> On Wed, 19 Feb 2025 08:39:08 +0100 Kurt Kanzenbach wrote:
> > > My comment from the previous series still stands, which simply that
> > > I have no idea if the maintainers will accept changes using this API
> > > or prefer to wait until Stanislav's work [1] is completed to remove
> > > the RTNL requirement from this API altogether.  
> > 
> > I'd rather consider patch #2 a bugfix to restore the busy polling with
> > XDP/ZC. After commit 5ef44b3cb43b ("xsk: Bring back busy polling
> > support") it is a requirement to implement this API.
> > 
> > The maintainers didn't speak up on v1, so i went along and sent v2.
> > 
> > @Jakub: What's your preference? Would you accept this series or rather
> > like to wait for Stanislav's work to be finished?
> 
> No strong preference. If rtnl_lock is not causing any issues 
> in this driver, the we can merge as is. I haven't followed 
> the past discussions, tho.

Don't mean to side-track this thread, but does this mean you've
changed your mind on the previous virtio_net thread [1] ?

Or maybe I'm just misreading your response there? And instead I
could re-spin the virtio_net but dropping the first patch and
dealing with RTNL in the code like this series is doing?

For some reason I was under the impression that the virtio_net
series and others like it (like this igb series) were being held
back until locking work Stanislav is doing is done.

[1]: https://lore.kernel.org/netdev/20250127133756.413efb24@kernel.org/

Re: [PATCH iwl-next v2 0/4] igb: XDP/ZC follow up

[Jakub Kicinski]

On Fri, 21 Feb 2025 15:53:26 -0500 Joe Damato wrote:
> > No strong preference. If rtnl_lock is not causing any issues 
> > in this driver, the we can merge as is. I haven't followed 
> > the past discussions, tho.  
> 
> Don't mean to side-track this thread, but does this mean you've
> changed your mind on the previous virtio_net thread [1] ?
> 
> Or maybe I'm just misreading your response there? And instead I
> could re-spin the virtio_net but dropping the first patch and
> dealing with RTNL in the code like this series is doing?
> 
> For some reason I was under the impression that the virtio_net
> series and others like it (like this igb series) were being held
> back until locking work Stanislav is doing is done.
> 
> [1]: https://lore.kernel.org/netdev/20250127133756.413efb24@kernel.org/

Yes, you can probably respin v1. Let's not block this.