Re: [PATCH net-next] net: lan743x: Fix the multi queue overflow issue

[Jakub Kicinski]

On Tue, 9 Aug 2022 14:06:28 +0530 Raju Lakkaraju wrote:
> Fix the Tx multi-queue overflow issue
> 
> Tx ring size of 128 (for TCP) provides ability to handle way more data than
> what Rx can (Rx buffers are constrained to one frame or even less during a
> dynamic mtu size change)
> 
> TX napi weight dependent of the ring size like it is now (ring size -1)
> because there is an express warning in the kernel about not registering weight
> values > NAPI_POLL_WEIGHT (currently 64)

I've read this message 3 times, I don't understand what you're saying.
Could you please rewrite it and add necessary details?

> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index a9a1dea6d731..d7c14ee7e413 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -2064,8 +2064,10 @@ static void lan743x_tx_frame_end(struct lan743x_tx *tx,
>  static netdev_tx_t lan743x_tx_xmit_frame(struct lan743x_tx *tx,
>  					 struct sk_buff *skb)
>  {
> +	struct lan743x_adapter *adapter = tx->adapter;
>  	int required_number_of_descriptors = 0;
>  	unsigned int start_frame_length = 0;
> +	netdev_tx_t retval = NETDEV_TX_OK;
>  	unsigned int frame_length = 0;
>  	unsigned int head_length = 0;
>  	unsigned long irq_flags = 0;
> @@ -2083,9 +2085,13 @@ static netdev_tx_t lan743x_tx_xmit_frame(struct lan743x_tx *tx,
>  		if (required_number_of_descriptors > (tx->ring_size - 1)) {
>  			dev_kfree_skb_irq(skb);
>  		} else {
> -			/* save to overflow buffer */
> -			tx->overflow_skb = skb;
> -			netif_stop_queue(tx->adapter->netdev);
> +			/* save how many descriptors we needed to restart the queue */
> +			tx->rqd_descriptors = required_number_of_descriptors;
> +			retval = NETDEV_TX_BUSY;
> +			if (is_pci11x1x_chip(adapter))
> +				netif_stop_subqueue(adapter->netdev, tx->channel_number);

Is tx->channel_number not 0 for devices other than pci11x1x ?
netif_stop_queue() is just an alias for queue 0 IIRC so
you can save all the ifs, most likely?

> +			else
> +				netif_stop_queue(adapter->netdev);
>  		}
>  		goto unlock;
>  	}
> @@ -2093,7 +2099,7 @@ static netdev_tx_t lan743x_tx_xmit_frame(struct lan743x_tx *tx,
>  	/* space available, transmit skb  */
>  	if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
>  	    (tx->ts_flags & TX_TS_FLAG_TIMESTAMPING_ENABLED) &&
> -	    (lan743x_ptp_request_tx_timestamp(tx->adapter))) {
> +	    (lan743x_ptp_request_tx_timestamp(adapter))) {

If this is a fix you should hold off on refactoring like adding the
local variable for adapter to make backports easier.

>  		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
>  		do_timestamp = true;
>  		if (tx->ts_flags & TX_TS_FLAG_ONE_STEP_SYNC)

> @@ -1110,7 +1109,7 @@ struct lan743x_tx_buffer_info {
>  	unsigned int    buffer_length;
>  };
>  
> -#define LAN743X_TX_RING_SIZE    (50)
> +#define LAN743X_TX_RING_SIZE    (128)

So the ring size is getting increased? I did not get that from the
commit message at all :S