netdev - Re: [PATCH v3 14/16] can: m_can: Use tx_fifo_in_flight for netif

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZBSPfmWEvl1eSWBV@corigine.com>
Date:   Fri, 17 Mar 2023 17:04:14 +0100
From:   Simon Horman <simon.horman@...igine.com>
To:     Markus Schneider-Pargmann <msp@...libre.com>
Cc:     Marc Kleine-Budde <mkl@...gutronix.de>,
        Chandrasekar Ramakrishnan <rcsekar@...sung.com>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        Vincent MAILHOL <mailhol.vincent@...adoo.fr>,
        linux-can@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 14/16] can: m_can: Use tx_fifo_in_flight for
 netif_queue control

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@...libre.com>
> ---
>  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;

...