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

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

[Raju Lakkaraju]

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)

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
---
 drivers/net/ethernet/microchip/lan743x_main.c | 64 +++++++++++--------
 drivers/net/ethernet/microchip/lan743x_main.h |  5 +-
 2 files changed, 41 insertions(+), 28 deletions(-)

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);
+			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))) {
 		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 		do_timestamp = true;
 		if (tx->ts_flags & TX_TS_FLAG_ONE_STEP_SYNC)
@@ -2144,14 +2150,13 @@ static netdev_tx_t lan743x_tx_xmit_frame(struct lan743x_tx *tx,
 
 unlock:
 	spin_unlock_irqrestore(&tx->ring_lock, irq_flags);
-	return NETDEV_TX_OK;
+	return retval;
 }
 
 static int lan743x_tx_napi_poll(struct napi_struct *napi, int weight)
 {
 	struct lan743x_tx *tx = container_of(napi, struct lan743x_tx, napi);
 	struct lan743x_adapter *adapter = tx->adapter;
-	bool start_transmitter = false;
 	unsigned long irq_flags = 0;
 	u32 ioc_bit = 0;
 
@@ -2163,24 +2168,36 @@ static int lan743x_tx_napi_poll(struct napi_struct *napi, int weight)
 
 	/* clean up tx ring */
 	lan743x_tx_release_completed_descriptors(tx);
-	if (netif_queue_stopped(adapter->netdev)) {
-		if (tx->overflow_skb) {
-			if (lan743x_tx_get_desc_cnt(tx, tx->overflow_skb) <=
-				lan743x_tx_get_avail_desc(tx))
-				start_transmitter = true;
-		} else {
-			netif_wake_queue(adapter->netdev);
+	if (is_pci11x1x_chip(adapter)) {
+		if (__netif_subqueue_stopped(adapter->netdev,
+		    tx->channel_number)) {
+			if (tx->rqd_descriptors) {
+				if (tx->rqd_descriptors <=
+					lan743x_tx_get_avail_desc(tx)) {
+					tx->rqd_descriptors = 0;
+					netif_wake_subqueue(adapter->netdev,
+							    tx->channel_number);
+				}
+			} else {
+				netif_wake_subqueue(adapter->netdev,
+						    tx->channel_number);
+			}
+		}
+	} else {
+		if (netif_queue_stopped(adapter->netdev)) {
+			if (tx->rqd_descriptors) {
+				if (tx->rqd_descriptors <=
+				    lan743x_tx_get_avail_desc(tx)) {
+					tx->rqd_descriptors = 0;
+					netif_wake_queue(adapter->netdev);
+				}
+			} else {
+				netif_wake_queue(adapter->netdev);
+			}
 		}
 	}
 	spin_unlock_irqrestore(&tx->ring_lock, irq_flags);
 
-	if (start_transmitter) {
-		/* space is now available, transmit overflow skb */
-		lan743x_tx_xmit_frame(tx, tx->overflow_skb);
-		tx->overflow_skb = NULL;
-		netif_wake_queue(adapter->netdev);
-	}
-
 	if (!napi_complete(napi))
 		goto done;
 
@@ -2304,10 +2321,7 @@ static void lan743x_tx_close(struct lan743x_tx *tx)
 
 	lan743x_tx_release_all_descriptors(tx);
 
-	if (tx->overflow_skb) {
-		dev_kfree_skb(tx->overflow_skb);
-		tx->overflow_skb = NULL;
-	}
+	tx->rqd_descriptors = 0;
 
 	lan743x_tx_ring_cleanup(tx);
 }
@@ -2387,7 +2401,7 @@ static int lan743x_tx_open(struct lan743x_tx *tx)
 							 (tx->channel_number));
 	netif_napi_add_tx_weight(adapter->netdev,
 				 &tx->napi, lan743x_tx_napi_poll,
-				 tx->ring_size - 1);
+				 NAPI_POLL_WEIGHT);
 	napi_enable(&tx->napi);
 
 	data = 0;
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index 72adae4f2aa0..58eb7abf976b 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -954,8 +954,7 @@ struct lan743x_tx {
 
 	struct napi_struct napi;
 	u32 frame_count;
-
-	struct sk_buff *overflow_skb;
+	u32 rqd_descriptors;
 };
 
 void lan743x_tx_set_timestamping_mode(struct lan743x_tx *tx,
@@ -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)
 
 /* OWN bit is set. ie, Descs are owned by RX DMAC */
 #define RX_DESC_DATA0_OWN_                (0x00008000)
-- 
2.25.1

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

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

[Raju Lakkaraju]

Hi Jakub,

Thank you for review comments.
I accepted all your comments.
I will fix in V1 patch series and also change the subject line to 
"net: lan743x: Fix to use multiqueue start/stop APIs"

Thanks,
Raju
The 08/10/2022 22:35, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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?
> 
Yes.
I will fix.

> > +                     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.
> 

Accepted.
I will fix.

> >               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

-- 
--------
Thanks,
Raju