| Message ID | 20250210-igb_irq-v1-2-bde078cdb9df@linutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | igb: XDP/ZC follow up | expand |
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.
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.
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,
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(-)