From: Jakub Kicinski <kuba@kernel.org> Date: 2021-08-11 21:38:16
A lockdep warning was triggered by netpoll because napi poll
was taking the xmit lock. Fix that and a couple more issues
noticed while reading the code.
Jakub Kicinski (4):
bnxt: don't lock the tx queue from napi poll
bnxt: disable napi before canceling DIM
bnxt: make sure xmit_more + errors does not miss doorbells
bnxt: count Tx drops
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 60 ++++++++++++++---------
drivers/net/ethernet/broadcom/bnxt/bnxt.h | 1 +
2 files changed, 37 insertions(+), 24 deletions(-)
--
2.31.1
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-08-11 21:38:17
napi schedules DIM, napi has to be disabled first,
then DIM canceled.
Noticed while reading the code.
Fixes: 0bc0b97fca73 ("bnxt_en: cleanup DIM work on device shutdown")
Fixes: 6a8788f25625 ("bnxt_en: add support for software dynamic interrupt moderation")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-08-11 21:38:18
We can't take the tx lock from the napi poll routine, because
netpoll can poll napi at any moment, including with the tx lock
already held.
It seems that the tx lock is only protecting against the disable
path, appropriate barriers are already in place to make sure
cleanup can safely run concurrently with start_xmit. I don't see
any other reason why 'stopped && avail > thresh' needs to be
re-checked under the lock.
Remove the tx lock and use synchronize_net() to make sure
closing the device does not race we restarting the queues.
Annotate accesses to dev_state against data races.
Fixes: c0c050c58d84 ("bnxt_en: New Broadcom ethernet driver.")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
v2: keep the unlikely in bnxt_tx_int() [Edwin]
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
@@ -9264,9 +9259,11 @@ void bnxt_tx_disable(struct bnxt *bp)if(bp->tx_ring){for(i=0;i<bp->tx_nr_rings;i++){txr=&bp->tx_ring[i];-txr->dev_state=BNXT_DEV_STATE_CLOSING;+WRITE_ONCE(txr->dev_state,BNXT_DEV_STATE_CLOSING);}}+/* Make sure napi polls see @dev_state change */+synchronize_net();/* Drop carrier first to prevent TX timeout */netif_carrier_off(bp->dev);/* Stop all TX queues */
@@ -9280,8 +9277,10 @@ void bnxt_tx_enable(struct bnxt *bp)for(i=0;i<bp->tx_nr_rings;i++){txr=&bp->tx_ring[i];-txr->dev_state=0;+WRITE_ONCE(txr->dev_state,0);}+/* Make sure napi polls see @dev_state change */+synchronize_net();netif_tx_wake_all_queues(bp->dev);if(bp->link_info.link_up)netif_carrier_on(bp->dev);
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-08-11 21:38:20
skbs are freed on error and not put on the ring. We may, however,
be in a situation where we're freeing the last skb of a batch,
and there is a doorbell ring pending because of xmit_more() being
true earlier. Make sure we ring the door bell in such situations.
Since errors are rare don't pay attention to xmit_more() and just
always flush the pending frames.
The busy case should be safe to be left alone because it can
only happen if start_xmit races with completions and they
both enable the queue. In that case the kick can't be pending.
Noticed while reading the code.
Fixes: 4d172f21cefe ("bnxt_en: Implement xmit_more.")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2: - netdev_warn() -> netif_warn() [Edwin]
- use correct prod value [Michael]
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 36 +++++++++++++++--------
drivers/net/ethernet/broadcom/bnxt/bnxt.h | 1 +
2 files changed, 25 insertions(+), 12 deletions(-)
I changed my mind. I added the && txr->kick_pending to the condition,
if there is a race and napi starts the queue unnecessarily the kick
can't be pending.
I think we should remove the setting of tx_buf->skb to NULL in the
tx_dma_error path since we are setting it here now.
But tx_buf gets moved IIRC - if we hit tx_dma_error tx_buf will be one
of the fragment bufs at this point. It should be legal to clear the skb
pointer on those AFAICT.
Are you suggesting to do something along the lines of:
txr->tx_buf_ring[txr->tx_prod].skb = NULL;
?
quoted
+ if (txr->kick_pending)
+ bnxt_txr_db_kick(bp, txr, txr->tx_prod);
return NETDEV_TX_OK;
}
I changed my mind. I added the && txr->kick_pending to the condition,
if there is a race and napi starts the queue unnecessarily the kick
can't be pending.
I don't understand. The queue should be stopped if we have <=
MAX_SKB_FRAGS + 1 descriptors left. If there is a race and the queue
is awake, the first TX packet may slip through if
skb_shinfo(skb)->nr_frags is small and we have enough descriptors for
it. Let's say xmit_more is set for this packet and so kick is
pending. The next packet may not fit anymore and it will hit this
check here.
I think we should remove the setting of tx_buf->skb to NULL in the
tx_dma_error path since we are setting it here now.
But tx_buf gets moved IIRC - if we hit tx_dma_error tx_buf will be one
of the fragment bufs at this point. It should be legal to clear the skb
pointer on those AFAICT.
Ah, you're right.
Are you suggesting to do something along the lines of:
txr->tx_buf_ring[txr->tx_prod].skb = NULL;
Yeah, I like this the best.
?
quoted
quoted
+ if (txr->kick_pending)
+ bnxt_txr_db_kick(bp, txr, txr->tx_prod);
return NETDEV_TX_OK;
}
I changed my mind. I added the && txr->kick_pending to the condition,
if there is a race and napi starts the queue unnecessarily the kick
can't be pending.
I don't understand. The queue should be stopped if we have <=
MAX_SKB_FRAGS + 1 descriptors left. If there is a race and the queue
is awake, the first TX packet may slip through if
skb_shinfo(skb)->nr_frags is small and we have enough descriptors for
it. Let's say xmit_more is set for this packet and so kick is
pending. The next packet may not fit anymore and it will hit this
check here.
But even if we slip past this check we can only do it once, the check
at the end of start_xmit() will see we have fewer slots than MAX_FRAGS
+ 2, ring the doorbell and stop.
I changed my mind. I added the && txr->kick_pending to the condition,
if there is a race and napi starts the queue unnecessarily the kick
can't be pending.
I don't understand. The queue should be stopped if we have <=
MAX_SKB_FRAGS + 1 descriptors left. If there is a race and the queue
is awake, the first TX packet may slip through if
skb_shinfo(skb)->nr_frags is small and we have enough descriptors for
it. Let's say xmit_more is set for this packet and so kick is
pending. The next packet may not fit anymore and it will hit this
check here.
But even if we slip past this check we can only do it once, the check
at the end of start_xmit() will see we have fewer slots than MAX_FRAGS
+ 2, ring the doorbell and stop.
I think there is one more problem here. Now that it is possible to
get here, we can race with bnxt_tx_int() again here. We can call
netif_tx_stop_queue() here after bnxt_tx_int() has already cleaned the
entire TX ring. So I think we need to call bnxt_tx_wake_queue() here
again if descriptors have become available.
From: David Laight <hidden> Date: 2021-08-13 08:35:49
From: Jakub Kicinski
Sent: 11 August 2021 22:38
skbs are freed on error and not put on the ring. We may, however,
be in a situation where we're freeing the last skb of a batch,
and there is a doorbell ring pending because of xmit_more() being
true earlier. Make sure we ring the door bell in such situations.
Since errors are rare don't pay attention to xmit_more() and just
always flush the pending frames.
Is this case actually so unlikely that the 'kick' can be
done unconditionally?
Then all the conditionals can be removed from the hot path.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)