[net-next] net: enetc: fix TX ring interrupt storm

Commit Message

Vladimir Oltean April 9, 2021, 7:27 p.m. UTC

From: Vladimir Oltean <[e-mail]>

The blamed commit introduced a bit in the TX software buffer descriptor
structure for determining whether a BD is final or not; we rearm the TX
interrupt vector for every frame (hence final BD) transmitted.

But there is a problem with the patch: it replaced a condition whose
expression is a bool which was evaluated at the beginning of the "while"
loop with a bool expression that is evaluated on the spot: tx_swbd->is_eof.

The problem with the latter expression is that the tx_swbd has already
been incremented at that stage, so the tx_swbd->is_eof check is in fact
with the _next_ software BD. Which is _not_ final.

The effect is that the CPU is in 100% load with ksoftirqd because it
does not acknowledge the TX interrupt, so the handler keeps getting
called again and again.

The fix is to restore the code structure, and keep the local bool is_eof
variable, just to assign it the tx_swbd->is_eof value instead of
!!tx_swbd->skb.

Fixes: d504498d2eb3 ("net: enetc: add a dedicated is_eof bit in the TX software BD")
Reported-by: Alex Marginean <[e-mail]>
Signed-off-by: Vladimir Oltean <[e-mail]>
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Claudiu Manoil April 9, 2021, 9:29 p.m. UTC | #1

On 09.04.2021 22:27, Vladimir Oltean wrote:
> From: Vladimir Oltean <[e-mail]>
> 
> The blamed commit introduced a bit in the TX software buffer descriptor
> structure for determining whether a BD is final or not; we rearm the TX
> interrupt vector for every frame (hence final BD) transmitted.
> 
> But there is a problem with the patch: it replaced a condition whose
> expression is a bool which was evaluated at the beginning of the "while"
> loop with a bool expression that is evaluated on the spot: tx_swbd->is_eof.
> 
> The problem with the latter expression is that the tx_swbd has already
> been incremented at that stage, so the tx_swbd->is_eof check is in fact
> with the _next_ software BD. Which is _not_ final.
> 
> The effect is that the CPU is in 100% load with ksoftirqd because it
> does not acknowledge the TX interrupt, so the handler keeps getting
> called again and again.
> 
> The fix is to restore the code structure, and keep the local bool is_eof
> variable, just to assign it the tx_swbd->is_eof value instead of
> !!tx_swbd->skb.
> 
> Fixes: d504498d2eb3 ("net: enetc: add a dedicated is_eof bit in the TX software BD")
> Reported-by: Alex Marginean <[e-mail]>
> Signed-off-by: Vladimir Oltean <[e-mail]>

Reviewed-by: Claudiu Manoil <[e-mail]>

Jakub Kicinski April 10, 2021, 1:20 a.m. UTC | #2

On Sat, 10 Apr 2021 00:29:23 +0300 Claudiu Manoil wrote:
> On 09.04.2021 22:27, Vladimir Oltean wrote:
> > From: Vladimir Oltean <[e-mail]>
> > 
> > The blamed commit introduced a bit in the TX software buffer descriptor
> > structure for determining whether a BD is final or not; we rearm the TX
> > interrupt vector for every frame (hence final BD) transmitted.
> > 
> > But there is a problem with the patch: it replaced a condition whose
> > expression is a bool which was evaluated at the beginning of the "while"
> > loop with a bool expression that is evaluated on the spot: tx_swbd->is_eof.
> > 
> > The problem with the latter expression is that the tx_swbd has already
> > been incremented at that stage, so the tx_swbd->is_eof check is in fact
> > with the _next_ software BD. Which is _not_ final.
> > 
> > The effect is that the CPU is in 100% load with ksoftirqd because it
> > does not acknowledge the TX interrupt, so the handler keeps getting
> > called again and again.
> > 
> > The fix is to restore the code structure, and keep the local bool is_eof
> > variable, just to assign it the tx_swbd->is_eof value instead of
> > !!tx_swbd->skb.
> > 
> > Fixes: d504498d2eb3 ("net: enetc: add a dedicated is_eof bit in the TX software BD")
> > Reported-by: Alex Marginean <[e-mail]>
> > Signed-off-by: Vladimir Oltean <[e-mail]>  
> 
> Reviewed-by: Claudiu Manoil <[e-mail]>

Applied, thanks!

Patch

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 57049ae97201..65f7f63c9bad 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -406,6 +406,7 @@  static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)
 	while (bds_to_clean && tx_frm_cnt < ENETC_DEFAULT_TX_WORK) {
 		struct xdp_frame *xdp_frame = enetc_tx_swbd_get_xdp_frame(tx_swbd);
 		struct sk_buff *skb = enetc_tx_swbd_get_skb(tx_swbd);
+		bool is_eof = tx_swbd->is_eof;
 
 		if (unlikely(tx_swbd->check_wb)) {
 			struct enetc_ndev_priv *priv = netdev_priv(ndev);
@@ -453,7 +454,7 @@  static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)
 		}
 
 		/* BD iteration loop end */
-		if (tx_swbd->is_eof) {
+		if (is_eof) {
 			tx_frm_cnt++;
 			/* re-arm interrupt source */
 			enetc_wr_reg_hot(tx_ring->idr, BIT(tx_ring->index) |