Re: [PATCH] mctp-i2c: increase the MCTP_I2C_TX_WORK_LEN to 500

[Matt Johnston <matt@codeconstruct.com.au>]

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