Date:   Fri, 17 Mar 2023 17:04:14 +0100
From:   Simon Horman <simon.horman@corigine.com>
To:     Markus Schneider-Pargmann <msp@baylibre.com>
Cc:     Marc Kleine-Budde <mkl@pengutronix.de>,
        Chandrasekar Ramakrishnan <rcsekar@samsung.com>,
        Wolfgang Grandegger <wg@grandegger.com>,
        Vincent MAILHOL <mailhol.vincent@wanadoo.fr>,
        linux-can@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 14/16] can: m_can: Use tx_fifo_in_flight for
 netif_queue control
Message-ID: <ZBSPfmWEvl1eSWBV@corigine.com>
References: <20230315110546.2518305-1-msp@baylibre.com>
 <20230315110546.2518305-15-msp@baylibre.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230315110546.2518305-15-msp@baylibre.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?wVnkiC6y5lrfn02PkS0Z6yHXXEGxsUg2JEgKpPivuZwsfFc5iNJ86ScVNs6W?=
 =?us-ascii?Q?6xlKWXw0mL197vvJESKMUJU0Q+uQNEkS+hKUFGgWp2KoepwbdOS5toBVW28c?=
 =?us-ascii?Q?rVSIAk5p6FJ6mVplR1vF+F3X8k4Xf50D3NkUXj2KIGWx2fjsYioVQg7ewUyH?=
 =?us-ascii?Q?9cKmQmEdwxRh6XNKovuFrAFyc57yFsJHBTGvYCdZtJYD6pL07t8l2JN9CJj8?=
 =?us-ascii?Q?mkU20s1luG0S+cfeFqIIW4SuTNSOviHWuw5cmxFcuKJOSlL567rlT0JHb+Mi?=
 =?us-ascii?Q?oQR0XHOuskpH44yLrCPFSOX8n0shOXj1cvOrOxcNhAnDNGhoyMy2aLF53dVu?=
 =?us-ascii?Q?P/Uh0+nODamW9OlG6XXrdPa5AY6Jl/NjqseacPOYmGuv4E4jkWBmaCdQWbSI?=
 =?us-ascii?Q?d0XaXNIP5hNMJPiIdBp21SGFD4/AdfxQY9aF2qoekJRFDdGflGvvLa7cNah4?=
 =?us-ascii?Q?bAaNXwq3FHsuW/5mXDnvf2YjmfQSTz6Hxib99HGGc5nM+Ywd8VF6jeSkGaUa?=
 =?us-ascii?Q?EePGNMYYaGrbJHaC41CeaLTpbQyHBzF9MNmEvq8GMjGoikUJRrX8CCyqXl2x?=
 =?us-ascii?Q?VH3oT1ZEPnUc4Y9NbK8yaUIv0Ku0H6lY5CFz7yUJyNFpBLlu6l/JbWa7G8ev?=
 =?us-ascii?Q?uXGHKTZTA3+czcLbXz7nvCSj3ZyMwET2ENMudN1v/N875f1iXBJcMLxpDxm8?=
 =?us-ascii?Q?xnkv2co4RJlWTY8KGuqhkLbhsOuOF0ZUjWkmyilTgrDxL3hPXsQ3w8cnma+t?=
 =?us-ascii?Q?6DKrZJRmjpsl4RjeMw+hNXb27xI81m7rJ2TmNTNSX8HufLMWhg68tiSYYsjp?=
 =?us-ascii?Q?QK5/Y0HmJVxmw7gYHHEXBYBbWCHfm6kJBasX3B1TGiSWCQ1TOiJoDu2LENW+?=
 =?us-ascii?Q?GQlgPKW3wsWQ6arQZ1gZNw72AGgt138w2jTAbqZtcA4g8wyU80Xj/cGMAnVK?=
 =?us-ascii?Q?1ogezayq8GdheNWAI+j1YSzm50asdqK0Th14NEQ9B0qluVtoDWZgutLsN6K6?=
 =?us-ascii?Q?2Ua4nataod0Z+HSgn+D8GSvzrTC2jFP7Vug9P6rylY3X1bA0iClkzPFqPx8v?=
 =?us-ascii?Q?4CTVDCR4QrCZ4lxxQgrsXKhQVNd+qsq98hMbXULbideRazOOmLH2EEzD4N84?=
 =?us-ascii?Q?dsp2BiQlVSbkCsH6SEHjyRkGFpnZQXbeRKw55h1DlOw2zccsYzuCqYBxr25i?=
 =?us-ascii?Q?X8+ClOHlK3UfAl6yi3tuvY4QFxmINuQ7VzNGHRKrdqNL7/YqOFHRSE404zXM?=
 =?us-ascii?Q?yHZKL0TWffsldy9V0HCoKR0eJXlVk4MIIjkHME5QIUrhnDIBFvT0yPjCJkXa?=
 =?us-ascii?Q?C+me/GWriIy8XTn/CeWsZKAEbv4RR3P1SNs4JNfkey7+71RDZmr/mgL3Im9q?=
 =?us-ascii?Q?hbno3+F1sxNa4UXYDg9EpNM0rM40z+mhv7o4dKzwr6UFJesDE++dF3KNfb4i?=
 =?us-ascii?Q?I45QCwpQJ9WrK0OYd9JYO/Ycp1c7mFVsy7xEXmSX3aYk41c7ignD+6Bw73iA?=
 =?us-ascii?Q?oyxVe81tMtX5mbeQSp/OogsE7fPYQ2dYutjciRQK2RvsP6GMTara0IlP1RuI?=
 =?us-ascii?Q?NunfaT2D1j48L7TYkXpkl5uF/STMsIROF7IAfzcOC7EXoCbqBxu5f69ozcQb?=
 =?us-ascii?Q?cefrsHk4xCkQw6r3Rw2yhgejEScMlnOxIn+dadaW1KQgRseFG0RsmuRaE+a2?=
 =?us-ascii?Q?Spr1Jw=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d0161423-2504-4178-9782-08db27014467
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Mar 2023 16:04:20.1022
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 8jS2mWd939Lb8mKco+mP6bk6MizLFvnh1oz1D18pXzfASFf+3aSWgf7CukhKXEB1g+Wvfr2Z1TZH/SJBhjhh9aCVFy52+xHkiibrDmL4Mxg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR13MB3979
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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@baylibre.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;

...