[PATCH 0/3] igb: XDP/ZC follow up

[PATCH 0/3] 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>
---
Kurt Kanzenbach (3):
      igb: Link IRQs to NAPI instances
      igb: Link queues to NAPI instances
      igb: Get rid of spurious interrupts

 drivers/net/ethernet/intel/igb/igb.h      |  5 ++-
 drivers/net/ethernet/intel/igb/igb_main.c | 67 ++++++++++++++++++++++++++-----
 drivers/net/ethernet/intel/igb/igb_xsk.c  |  3 ++
 3 files changed, 65 insertions(+), 10 deletions(-)
---
base-commit: acdefab0dcbc3833b5a734ab80d792bb778517a0
change-id: 20250206-igb_irq-f5a4d4deb207

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

[PATCH 1/3] 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

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: [PATCH 1/3] igb: Link IRQs to NAPI instances

[Joe Damato]

On Mon, Feb 10, 2025 at 10:19:35AM +0100, Kurt Kanzenbach wrote:
> 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
> 
> 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);
>  	}

As far as I can tell, all paths that lead here hold RTNL:
  - power management (__igb_resume)
  - ethtool set_channels (igb_reinit_queues)
  - and regular ndo_open

So:

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

Re: [PATCH 1/3] igb: Link IRQs to NAPI instances

[Joe Damato]

On Mon, Feb 10, 2025 at 10:25:47AM -0800, Joe Damato wrote:
> On Mon, Feb 10, 2025 at 10:19:35AM +0100, Kurt Kanzenbach wrote:
> > 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
> > 
> > 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);
> >  	}
> 
> As far as I can tell, all paths that lead here hold RTNL:

A nit on my own comment, netif_napi_set_irq doesn't ASSERT_RTNL (but
does hold net_device->lock); its the other functions in patch 2 that
ASSERT_RTNL.

My reviewed-by stands; just wanted to correct myself.

[PATCH 2/3] 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'}]

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 | 35 ++++++++++++++++++++++++++-----
 drivers/net/ethernet/intel/igb/igb_xsk.c  |  2 ++
 3 files changed, 34 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..8e964484f4c9854e4e3e0b4f3e8785fe93bd1207 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);
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 2/3] igb: Link queues to NAPI instances

[Joe Damato]

On Mon, Feb 10, 2025 at 10:19:36AM +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'}]
> 
> 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 | 35 ++++++++++++++++++++++++++-----
>  drivers/net/ethernet/intel/igb/igb_xsk.c  |  2 ++
>  3 files changed, 34 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..8e964484f4c9854e4e3e0b4f3e8785fe93bd1207 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);
> +	}

It looks like igb_ub is called from igb_io_resume (struct
pci_error_handlers). I don't know if RTNL is held in that path. If
its not, this could trip the ASSERT_RTNL in netif_queue_set_napi.

Can you check and see if this is an issue for that path?

igb_reinit_locked looks OK (as the name implies).

>  
>  	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);

Same question as above. It looks like igb_down is called from
igb_io_error_detected. I don't know if RTNL is held in that path. If
its not, it'll trip the ASSERT_RTNL in netif_queue_set_napi.

Can you check if that's an issue for this path, as well?

>  		}
>  	}
> @@ -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);
> +	}

The above looks fine. __igb_open is called from __igb_resume which
takes care of RTNL. So, I think this part is fine.


>  	rd32(E1000_TSICR);
> 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);
>  }

These seem fine to me.

Re: [PATCH 2/3] igb: Link queues to NAPI instances

[Kurt Kanzenbach]

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

On Mon Feb 10 2025, Joe Damato wrote:
> On Mon, Feb 10, 2025 at 10:19:36AM +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'}]
>> 
>> 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 | 35 ++++++++++++++++++++++++++-----
>>  drivers/net/ethernet/intel/igb/igb_xsk.c  |  2 ++
>>  3 files changed, 34 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..8e964484f4c9854e4e3e0b4f3e8785fe93bd1207 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);
>> +	}
>
> It looks like igb_ub is called from igb_io_resume (struct
> pci_error_handlers). I don't know if RTNL is held in that path. If
> its not, this could trip the ASSERT_RTNL in netif_queue_set_napi.
>
> Can you check and see if this is an issue for that path?

AFAICS the PCI error handlers are called in drivers/pci/pcie/err.c
without RTNL lock held. These function take only the device_lock().

I'll add the missing rtnl_lock()/unlock() calls to igb_io_resume() and
igb_io_error_detected(). Thanks.

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

[PATCH 3/3] 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 igc 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

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 8e964484f4c9854e4e3e0b4f3e8785fe93bd1207..96da3e2ddc9a67f799ee55d9621d98f80a6b449c 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -5754,11 +5754,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);
@@ -9089,6 +9107,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;
 		}
 
@@ -9148,6 +9167,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;
 	}
 
@@ -9164,6 +9184,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: [PATCH 3/3] igb: Get rid of spurious interrupts

[Joe Damato]

On Mon, Feb 10, 2025 at 10:19:37AM +0100, Kurt Kanzenbach wrote:
> 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 igc 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
> 
> 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(-)

I am not an igb expert (nor do I have such a device), but after
reading the source a bit this seems reasonable.

I suppose perhaps a better direction in the future would be to
convert the driver to the page pool, but in the meantime the
proposed change seems reasonable.

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

Re: [PATCH 3/3] igb: Get rid of spurious interrupts

[Gerhard Engleder]

On 10.02.25 10:19, Kurt Kanzenbach wrote:
> 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 igc task watchdog which triggers Rx interrupts periodically.

igc or igb?

Re: [PATCH 3/3] igb: Get rid of spurious interrupts

[Kurt Kanzenbach]

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

On Mon Feb 10 2025, Gerhard Engleder wrote:
> On 10.02.25 10:19, Kurt Kanzenbach wrote:
>> 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 igc task watchdog which triggers Rx interrupts periodically.
>
> igc or igb?

igb of course. Thanks.

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

Re: [PATCH 0/3] igb: XDP/ZC follow up

[Joe Damato]

On Mon, Feb 10, 2025 at 10:19:34AM +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>
> ---
> Kurt Kanzenbach (3):
>       igb: Link IRQs to NAPI instances
>       igb: Link queues to NAPI instances
>       igb: Get rid of spurious interrupts
> 
>  drivers/net/ethernet/intel/igb/igb.h      |  5 ++-
>  drivers/net/ethernet/intel/igb/igb_main.c | 67 ++++++++++++++++++++++++++-----
>  drivers/net/ethernet/intel/igb/igb_xsk.c  |  3 ++
>  3 files changed, 65 insertions(+), 10 deletions(-)
> ---
> base-commit: acdefab0dcbc3833b5a734ab80d792bb778517a0
> change-id: 20250206-igb_irq-f5a4d4deb207

Overall wanted to note that Stanislav is working on some locking
changes to remove the RTNL dependency [1].

My previous attempt at adding this API to virtio_net is on hold
until the locking stuff Stanislav is doing is done. I am not sure if
the maintainers will also ask to hold your series back, as well.

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

Re: [PATCH 0/3] igb: XDP/ZC follow up

[Joe Damato]

On Mon, Feb 10, 2025 at 10:19:34AM +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.

You may want to use netif_napi_add_config to enable persistent NAPI
config, btw. This makes writing userland programs based on
SO_INCOMING_NAPI_ID much easier.

See also:

https://lore.kernel.org/netdev/20250208012822.34327-1-jdamato@fastly.com/

Re: [PATCH 0/3] igb: XDP/ZC follow up

[Kurt Kanzenbach]

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

On Mon Feb 10 2025, Joe Damato wrote:
> On Mon, Feb 10, 2025 at 10:19:34AM +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.
>
> You may want to use netif_napi_add_config to enable persistent NAPI
> config, btw. This makes writing userland programs based on
> SO_INCOMING_NAPI_ID much easier.

Thanks, that looks useful too. I'll add another patch to this series to
use netif_napi_add_config().

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