On Wed, Mar 15, 2023 at 12:05:44PM +0100, Markus Schneider-Pargmann wrote:
> The network queue is currently always stopped in start_xmit and
> continued in the interrupt handler. This is not possible anymore if we
> want to keep multiple transmits in flight in parallel.
>
> Use the previously introduced tx_fifo_in_flight counter to control the
> network queue instead. This has the benefit of not needing to ask the
> hardware about fifo status.
>
> This patch stops the network queue in start_xmit if the number of
> transmits in flight reaches the size of the fifo and wakes up the queue
> from the interrupt handler once the transmits in flight drops below the
> fifo size. This means any skbs over the limit will be rejected
> immediately in start_xmit (it shouldn't be possible at all to reach that
> state anyways).
>
> The maximum number of transmits in flight is the size of the fifo.
>
> Signed-off-by: Markus Schneider-Pargmann <msp@xxxxxxxxxxxx>
> ---
> drivers/net/can/m_can/m_can.c | 71 +++++++++++++----------------------
> 1 file changed, 26 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 4ad8f08f8284..3cb3d01e1a61 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
...
> @@ -1033,17 +1023,31 @@ static void m_can_finish_tx(struct m_can_classdev *cdev, int transmitted)
> unsigned long irqflags;
>
> spin_lock_irqsave(&cdev->tx_handling_spinlock, irqflags);
> + if (cdev->tx_fifo_in_flight >= cdev->tx_fifo_size && transmitted > 0)
> + netif_wake_queue(cdev->net);
> cdev->tx_fifo_in_flight -= transmitted;
> spin_unlock_irqrestore(&cdev->tx_handling_spinlock, irqflags);
> }
>
> -static void m_can_start_tx(struct m_can_classdev *cdev)
> +static netdev_tx_t m_can_start_tx(struct m_can_classdev *cdev)
> {
> unsigned long irqflags;
> + int tx_fifo_in_flight;
>
> spin_lock_irqsave(&cdev->tx_handling_spinlock, irqflags);
> - ++cdev->tx_fifo_in_flight;
> + tx_fifo_in_flight = cdev->tx_fifo_in_flight + 1;
> + if (tx_fifo_in_flight >= cdev->tx_fifo_size) {
> + netif_stop_queue(cdev->net);
> + if (tx_fifo_in_flight > cdev->tx_fifo_size) {
> + netdev_err(cdev->net, "hard_xmit called while TX FIFO full\n");
Perhaps I misunderstand things.
But it seems that this message is triggered in the datapath.
Thus I think it should be rate limited, or perhaps only logged once.
> + spin_unlock_irqrestore(&cdev->tx_handling_spinlock, irqflags);
> + return NETDEV_TX_BUSY;
> + }
> + }
> + cdev->tx_fifo_in_flight = tx_fifo_in_flight;
> spin_unlock_irqrestore(&cdev->tx_handling_spinlock, irqflags);
> +
> + return NETDEV_TX_OK;
> }
>
> static int m_can_echo_tx_event(struct net_device *dev)
...
> @@ -1830,11 +1810,6 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev,
> m_can_write(cdev, M_CAN_TXBAR, (1 << putidx));
> cdev->tx_fifo_putidx = (++cdev->tx_fifo_putidx >= cdev->can.echo_skb_max ?
> 0 : cdev->tx_fifo_putidx);
> -
> - /* stop network queue if fifo full */
> - if (m_can_tx_fifo_full(cdev) ||
> - m_can_next_echo_skb_occupied(dev, putidx))
> - netif_stop_queue(dev);
> }
smatch tells me that m_can_next_echo_skb_occupied is now defined but unused.
>
> return NETDEV_TX_OK;
...
