The original value (100) is not sufficient for our use case.
For example, we have 4 NVMe-mi devices on the same i2c bus.
When sending namespace create Admin command concurrently, they
will send 4x4KB data to device concurrently, which may be
split into 4x(4KB/64B)=256 packets.
Tested:
Before the fix, we will see below message in kernel log when
concurrently sending namespace create commands to the 4 NVMe-MI
devices on the same i2c bus:
kernel: i2c i2c-6 mctpi2c6: BUG! Tx Ring full when queue awake!
After the fix, the error message is gone.
Signed-off-by: Jinliang Wang <[email protected]>
---
drivers/net/mctp/mctp-i2c.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/mctp/mctp-i2c.c b/drivers/net/mctp/mctp-i2c.c
index b37a9e4bade4..b658aa040620 100644
--- a/drivers/net/mctp/mctp-i2c.c
+++ b/drivers/net/mctp/mctp-i2c.c
@@ -34,7 +34,7 @@
#define MCTP_I2C_BUFSZ (3 + MCTP_I2C_MAXBLOCK + 1)
#define MCTP_I2C_MINLEN 8
#define MCTP_I2C_COMMANDCODE 0x0f
-#define MCTP_I2C_TX_WORK_LEN 100
+#define MCTP_I2C_TX_WORK_LEN 500
/* Sufficient for 64kB at min mtu */
#define MCTP_I2C_TX_QUEUE_LEN 1100
--
2.43.0.rc0.421.g78406f8d94-goog
Hi Jinliang,
> Tested:
> Before the fix, we will see below message in kernel log when
> concurrently sending namespace create commands to the 4 NVMe-MI
> devices on the same i2c bus:
> kernel: i2c i2c-6 mctpi2c6: BUG! Tx Ring full when queue awake!
>
> After the fix, the error message is gone.
Thanks for the report, but I don't think this is the correct fix: you
should not hit that error even if > TX_WORK_LEN packets need to be sent.
The net core should not be attempting to queue more skbs after
netif_stop_queue(), which we do in the conditional below the warning:
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)
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`
Matt, any preferences?
Cheers,
Jeremy
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