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

From: Jakub Kicinski
Date: Thu Aug 11 2022 - 01:35:34 EST


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