Messages in this thread | | | Subject | Re: [PATCH] mctp-i2c: increase the MCTP_I2C_TX_WORK_LEN to 500 | From | Matt Johnston <> | Date | Fri, 17 Nov 2023 08:50:18 -0600 |
| |
Hi,
On Fri, 2023-11-17 at 15:29 +0800, Jeremy Kerr wrote: > spin_lock_irqsave(&midev->tx_queue.lock, flags); > if (skb_queue_len(&midev->tx_queue) >= MCTP_I2C_TX_WORK_LEN) { > netif_stop_queue(dev); > spin_unlock_irqrestore(&midev->tx_queue.lock, flags); > netdev_err(dev, "BUG! Tx Ring full when queue awake!\n"); > return NETDEV_TX_BUSY; > } > > __skb_queue_tail(&midev->tx_queue, skb); > if (skb_queue_len(&midev->tx_queue) == MCTP_I2C_TX_WORK_LEN) // normal stop > netif_stop_queue(dev); > spin_unlock_irqrestore(&midev->tx_queue.lock, flags); > > What looks like has happened here: > > 1) we have TX_WORK_LEN-1 packets queued > 2) we release a flow, which queues the "marker" skb. the tx_queue now > has TX_WORK_LEN items > 3) we queue another packet, ending up with TX_WORK_LEN+1 in the queue > 4) the == TX_WORK_LEN test fails, so we dont do a netif_stop_queue() > > A couple of potential fixes: > > * We do the check and conditional netif_stop_queue() in (2) > * We change the check there to be `>= MCTP_I2C_TX_WORK_LEN`
My inclination would be to change the second comparison (the normal stop condition) to
/* -1 to allow space for an additional unlock_marker skb */ if (skb_queue_len(&midev->tx_queue) >= MCTP_I2C_TX_WORK_LEN-1)
Cheers, Matt
| |