bnxt: don't handle XDP in netpoll
Jakub Kicinski
maintainerPATCH v2 1/1 bnxt: don't handle XDP in netpoll
Similarly to other recently fixed drivers make sure we don't try to access XDP or page pool APIs when NAPI budget is 0. NAPI budget of 0 may mean that we are in netpoll.
This may result in running software IRQs in hard IRQ context, leading to deadlocks or crashes.
To make sure bnapi->tx_pkts don't get wiped without handling the events, move clearing the field into the handler itself. Remember to clear tx_pkts after reset (bnxt_enable_napi()) as it's technically possible that netpoll will accumulate some tx_pkts and then a reset will happen, leaving tx_pkts out of sync with reality.
---
CC: [email protected]
v2:
- handle storing tx_pkts more carefully
- now that we touch tx_pkts in the handlers don't pass nr_pkts as an arg, makes lines shorter
v1: https://lore.kernel.org/all/[email protected]/
--- drivers/net/ethernet/broadcom/bnxt/bnxt.c | 26 +++++++++++-------- drivers/net/ethernet/broadcom/bnxt/bnxt.h | 2 +- drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 8 +++++- drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h | 2 +- 4 files changed, 24 insertions(+), 14 deletions(-)
drivers/net/ethernet/broadcom/bnxt/bnxt.c +15 -11
@@ -633,12 +633,13 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)return NETDEV_TX_OK ; } -static void bnxt_tx_int (struct bnxt *bp,struct bnxt_napi *bnapi,int nr_pkts) +static void bnxt_tx_int (struct bnxt *bp,struct bnxt_napi *bnapi,int budget) {struct bnxt_tx_ring_info *txr = bnapi->tx_ring;struct netdev_queue *txq =netdev_get_tx_queue (bp->dev, txr->txq_index);u16 cons = txr->tx_cons;struct pci_dev *pdev = bp->pdev; +int nr_pkts = bnapi->tx_pkts;int i;unsigned int tx_bytes =0 ;
@@ -688,6 +689,7 @@ static void bnxt_tx_int(struct bnxt *bp, struct bnxt_napi *bnapi, int nr_pkts)dev_kfree_skb_any (skb); } + bnapi->tx_pkts =0 ;WRITE_ONCE (txr->tx_cons, cons); __netif_txq_completed_wake(txq, nr_pkts, tx_bytes,
@@ -2569,12 +2571,11 @@ static int __bnxt_poll_work(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,return rx_pkts; } -static void __bnxt_poll_work_done (struct bnxt *bp,struct bnxt_napi *bnapi) +static void __bnxt_poll_work_done (struct bnxt *bp,struct bnxt_napi *bnapi, +int budget) { -if (bnapi->tx_pkts) { - bnapi->tx_int (bp, bnapi, bnapi->tx_pkts); - bnapi->tx_pkts =0 ; - } +if (bnapi->tx_pkts) + bnapi->tx_int (bp, bnapi, budget);if ((bnapi->events &BNXT_RX_EVENT ) && !(bnapi->in_reset)) {struct bnxt_rx_ring_info *rxr = bnapi->rx_ring;
@@ -2603,7 +2604,7 @@ static int bnxt_poll_work(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,*/ bnxt_db_cq (bp, &cpr->cp_db, cpr->cp_raw_cons); -__bnxt_poll_work_done (bp, bnapi); +__bnxt_poll_work_done (bp, bnapi, budget);return rx_pkts; }
@@ -2734,7 +2735,7 @@ static int __bnxt_poll_cqs(struct bnxt *bp, struct bnxt_napi *bnapi, int budget) }static void __bnxt_poll_cqs_done (struct bnxt *bp,struct bnxt_napi *bnapi, -u64 dbr_type) +u64 dbr_type,int budget) {struct bnxt_cp_ring_info *cpr = &bnapi->cp_ring;int i;
@@ -2750,7 +2751,7 @@ static void __bnxt_poll_cqs_done(struct bnxt *bp, struct bnxt_napi *bnapi, cpr2->had_work_done =0 ; } } -__bnxt_poll_work_done (bp, bnapi); +__bnxt_poll_work_done (bp, bnapi, budget); }static int bnxt_poll_p5 (struct napi_struct *napi,int budget)
@@ -2780,7 +2781,8 @@ static int bnxt_poll_p5(struct napi_struct *napi, int budget)if (cpr->has_more_work)break ; -__bnxt_poll_cqs_done (bp, bnapi,DBR_TYPE_CQ_ARMALL ); +__bnxt_poll_cqs_done (bp, bnapi,DBR_TYPE_CQ_ARMALL , + budget); cpr->cp_raw_cons = raw_cons;if (napi_complete_done (napi, work_done)) BNXT_DB_NQ_ARM_P5(&cpr->cp_db,
@@ -2810,7 +2812,7 @@ static int bnxt_poll_p5(struct napi_struct *napi, int budget) } raw_cons =NEXT_RAW_CMP (raw_cons); } -__bnxt_poll_cqs_done (bp, bnapi,DBR_TYPE_CQ ); +__bnxt_poll_cqs_done (bp, bnapi,DBR_TYPE_CQ , budget);if (raw_cons != cpr->cp_raw_cons) { cpr->cp_raw_cons = raw_cons;BNXT_DB_NQ_P5 (&cpr->cp_db, raw_cons);
@@ -9429,6 +9431,8 @@ static void bnxt_enable_napi(struct bnxt *bp) cpr->sw_stats.rx.rx_resets++; bnapi->in_reset = false; + bnapi->tx_pkts =0 ; +if (bnapi->rx_ring) {INIT_WORK (&cpr->dim.work, bnxt_dim_work); cpr->dim.mode =DIM_CQ_PERIOD_MODE_START_FROM_EQE ;
drivers/net/ethernet/broadcom/bnxt/bnxt.h +1 -1
@@ -1005,7 +1005,7 @@ struct bnxt_napi {struct bnxt_tx_ring_info *tx_ring;void (*tx_int)(struct bnxt *,struct bnxt_napi *, -int ); +int budget);int tx_pkts;u8 events;
drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c +7 -1
@@ -125,16 +125,20 @@ static void __bnxt_xmit_xdp_redirect(struct bnxt *bp,dma_unmap_len_set (tx_buf, len,0 ); } -void bnxt_tx_int_xdp (struct bnxt *bp,struct bnxt_napi *bnapi,int nr_pkts) +void bnxt_tx_int_xdp (struct bnxt *bp,struct bnxt_napi *bnapi,int budget) {struct bnxt_tx_ring_info *txr = bnapi->tx_ring;struct bnxt_rx_ring_info *rxr = bnapi->rx_ring;bool rx_doorbell_needed = false; +int nr_pkts = bnapi->tx_pkts;struct bnxt_sw_tx_bd *tx_buf;u16 tx_cons = txr->tx_cons;u16 last_tx_cons = tx_cons;int i, j, frags; +if (!budget) +return ; +for (i =0 ; i < nr_pkts; i++) { tx_buf = &txr->tx_buf_ring[tx_cons];
@@ -161,6 +165,8 @@ void bnxt_tx_int_xdp(struct bnxt *bp, struct bnxt_napi *bnapi, int nr_pkts) } tx_cons =NEXT_TX (tx_cons); } + + bnapi->tx_pkts =0 ;WRITE_ONCE (txr->tx_cons, tx_cons);if (rx_doorbell_needed) { tx_buf = &txr->tx_buf_ring[last_tx_cons];
drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h +1 -1
@@ -16,7 +16,7 @@ struct bnxt_sw_tx_bd *bnxt_xmit_bd(struct bnxt *bp,struct bnxt_tx_ring_info *txr, dma_addr_t mapping, u32 len, struct xdp_buff *xdp); -void bnxt_tx_int_xdp (struct bnxt *bp,struct bnxt_napi *bnapi,int nr_pkts); +void bnxt_tx_int_xdp (struct bnxt *bp,struct bnxt_napi *bnapi,int budget);bool bnxt_rx_xdp (struct bnxt *bp,struct bnxt_rx_ring_info *rxr,u16 cons,struct xdp_buff xdp,struct page *page,u8 **data_ptr,unsigned int *len,u8 *event);
Signature · 1 line
2.41.0
Quoted 8 lines · Jakub Kicinski
Similarly to other recently fixed drivers make sure we don't try to access XDP or page pool APIs when NAPI budget is 0. NAPI budget of 0 may mean that we are in netpoll.
This may result in running software IRQs in hard IRQ context, leading to deadlocks or crashes.
Quoted 6 lines
To make sure
bnapi->tx_pktsdon't get wiped without handling the events, move clearing the field into the handler itself. Remember to cleartx_pktsafter reset (bnxt_enable_napi()) as it's technically possible that netpoll will accumulate sometx_pktsand then a reset will happen, leavingtx_pktsout of sync with reality.
Yes, with your recently added TX reset when the SKB is NULL, this is definitely a possibility.
Quoted 4 lines
Fixes 322b87ca55f2 ("bnxt_en: add page_pool support")Reviewed-by Andy Gospodarek <[email protected]>Signed-off-by Jakub Kicinski <[email protected]>
Thanks.
This patch was applied to netdev/net.git (main) by Jakub Kicinski <[email protected]>:
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:
Quoted 8 lines · On Fri, 28 Jul 2023 13:50:20 -0700 you wrote:
Similarly to other recently fixed drivers make sure we don't try to access XDP or page pool APIs when NAPI budget is 0. NAPI budget of 0 may mean that we are in netpoll.
This may result in running software IRQs in hard IRQ context, leading to deadlocks or crashes.
[...]
Here is the summary with links:
- [net,v2] bnxt: don't handle XDP in netpoll
https://git.kernel.org/netdev/net/c/37b61cda9c16
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html