[PATCH net-next v2 0/2] r8152: modify rx_bottom

[PATCH net-next v2 0/2] r8152: modify rx_bottom

[Hayes Wang]

v2:
For patch #1, add comment, update commit message, and add Fixes tag.

v1:
These patches are used to improve rx_bottom().

Hayes Wang (2):
  r8152: remove queuing rx packets in driver
  r8152: use napi_gro_frags

 drivers/net/usb/r8152.c | 84 ++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 48 deletions(-)

-- 
2.41.0

[PATCH net-next v2 1/2] r8152: remove queuing rx packets in driver

[Hayes Wang]

A bulk transfer of the USB may contain many packets. And, the total
number of the packets in the bulk transfer may be more than budget.

Originally, only budget packets would be handled by napi_gro_receive(),
and the other packets would be queued in the driver for next schedule.

Now, deal with all packets in the current bulk transfer through
napi_gro_receive(), even the number is more than the budget. And, stop
to handle the next bulk transfer until next schedule, if the budget is
exhausted. Then, we don't need to queue the packets in the driver.

Fixes: cf74eb5a5bc8 ("eth: r8152: try to use a normal budget")
Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 56 ++++++++++++++---------------------------
 1 file changed, 19 insertions(+), 37 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 0c13d9950cd8..03bb3a58f2a6 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -871,7 +871,7 @@ struct r8152 {
 	struct tx_agg tx_info[RTL8152_MAX_TX];
 	struct list_head rx_info, rx_used;
 	struct list_head rx_done, tx_free;
-	struct sk_buff_head tx_queue, rx_queue;
+	struct sk_buff_head tx_queue;
 	spinlock_t rx_lock, tx_lock;
 	struct delayed_work schedule, hw_phy_work;
 	struct mii_if_info mii;
@@ -2031,7 +2031,6 @@ static int alloc_all_mem(struct r8152 *tp)
 	INIT_LIST_HEAD(&tp->tx_free);
 	INIT_LIST_HEAD(&tp->rx_done);
 	skb_queue_head_init(&tp->tx_queue);
-	skb_queue_head_init(&tp->rx_queue);
 	atomic_set(&tp->rx_count, 0);
 
 	for (i = 0; i < RTL8152_MAX_RX; i++) {
@@ -2431,24 +2430,6 @@ static int rx_bottom(struct r8152 *tp, int budget)
 	int ret = 0, work_done = 0;
 	struct napi_struct *napi = &tp->napi;
 
-	if (!skb_queue_empty(&tp->rx_queue)) {
-		while (work_done < budget) {
-			struct sk_buff *skb = __skb_dequeue(&tp->rx_queue);
-			struct net_device *netdev = tp->netdev;
-			struct net_device_stats *stats = &netdev->stats;
-			unsigned int pkt_len;
-
-			if (!skb)
-				break;
-
-			pkt_len = skb->len;
-			napi_gro_receive(napi, skb);
-			work_done++;
-			stats->rx_packets++;
-			stats->rx_bytes += pkt_len;
-		}
-	}
-
 	if (list_empty(&tp->rx_done))
 		goto out1;
 
@@ -2484,10 +2465,6 @@ static int rx_bottom(struct r8152 *tp, int budget)
 			unsigned int pkt_len, rx_frag_head_sz;
 			struct sk_buff *skb;
 
-			/* limit the skb numbers for rx_queue */
-			if (unlikely(skb_queue_len(&tp->rx_queue) >= 1000))
-				break;
-
 			pkt_len = le32_to_cpu(rx_desc->opts1) & RX_LEN_MASK;
 			if (pkt_len < ETH_ZLEN)
 				break;
@@ -2525,14 +2502,10 @@ static int rx_bottom(struct r8152 *tp, int budget)
 
 			skb->protocol = eth_type_trans(skb, netdev);
 			rtl_rx_vlan_tag(rx_desc, skb);
-			if (work_done < budget) {
-				work_done++;
-				stats->rx_packets++;
-				stats->rx_bytes += skb->len;
-				napi_gro_receive(napi, skb);
-			} else {
-				__skb_queue_tail(&tp->rx_queue, skb);
-			}
+			work_done++;
+			stats->rx_packets++;
+			stats->rx_bytes += skb->len;
+			napi_gro_receive(napi, skb);
 
 find_next_rx:
 			rx_data = rx_agg_align(rx_data + pkt_len + ETH_FCS_LEN);
@@ -2562,16 +2535,28 @@ static int rx_bottom(struct r8152 *tp, int budget)
 			urb->actual_length = 0;
 			list_add_tail(&agg->list, next);
 		}
+
+		/* A bulk transfer of USB may contain may packets, so the
+		 * work_done may more than the budget. Deal with all packets
+		 * in current bulk transfer, and stop to handle the next
+		 * bulk transfer until next schedule, if budget is exhausted.
+		 */
+		if (work_done >= budget)
+			break;
 	}
 
+	/* Splice the remained list back to rx_done for next schedule */
 	if (!list_empty(&rx_queue)) {
 		spin_lock_irqsave(&tp->rx_lock, flags);
-		list_splice_tail(&rx_queue, &tp->rx_done);
+		list_splice(&rx_queue, &tp->rx_done);
 		spin_unlock_irqrestore(&tp->rx_lock, flags);
 	}
 
 out1:
-	return work_done;
+	if (work_done > budget)
+		return budget;
+	else
+		return work_done;
 }
 
 static void tx_bottom(struct r8152 *tp)
@@ -2992,9 +2977,6 @@ static int rtl_stop_rx(struct r8152 *tp)
 	list_splice(&tmp_list, &tp->rx_info);
 	spin_unlock_irqrestore(&tp->rx_lock, flags);
 
-	while (!skb_queue_empty(&tp->rx_queue))
-		dev_kfree_skb(__skb_dequeue(&tp->rx_queue));
-
 	return 0;
 }
 
-- 
2.41.0

[PATCH net-next v2 2/2] r8152: use napi_gro_frags

[Hayes Wang]

Use napi_gro_frags() for the skb of fragments.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 03bb3a58f2a6..00dac52dea44 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -2462,8 +2462,9 @@ static int rx_bottom(struct r8152 *tp, int budget)
 		while (urb->actual_length > len_used) {
 			struct net_device *netdev = tp->netdev;
 			struct net_device_stats *stats = &netdev->stats;
-			unsigned int pkt_len, rx_frag_head_sz;
+			unsigned int pkt_len;
 			struct sk_buff *skb;
+			bool use_frags;
 
 			pkt_len = le32_to_cpu(rx_desc->opts1) & RX_LEN_MASK;
 			if (pkt_len < ETH_ZLEN)
@@ -2477,35 +2478,40 @@ static int rx_bottom(struct r8152 *tp, int budget)
 			rx_data += sizeof(struct rx_desc);
 
 			if (!agg_free || tp->rx_copybreak > pkt_len)
-				rx_frag_head_sz = pkt_len;
+				use_frags = false;
 			else
-				rx_frag_head_sz = tp->rx_copybreak;
+				use_frags = true;
+
+			if (use_frags)
+				skb = napi_get_frags(napi);
+			else
+				skb = napi_alloc_skb(napi, pkt_len);
 
-			skb = napi_alloc_skb(napi, rx_frag_head_sz);
 			if (!skb) {
 				stats->rx_dropped++;
 				goto find_next_rx;
 			}
 
 			skb->ip_summed = r8152_rx_csum(tp, rx_desc);
-			memcpy(skb->data, rx_data, rx_frag_head_sz);
-			skb_put(skb, rx_frag_head_sz);
-			pkt_len -= rx_frag_head_sz;
-			rx_data += rx_frag_head_sz;
-			if (pkt_len) {
+			rtl_rx_vlan_tag(rx_desc, skb);
+
+			if (use_frags) {
 				skb_add_rx_frag(skb, 0, agg->page,
 						agg_offset(agg, rx_data),
 						pkt_len,
 						SKB_DATA_ALIGN(pkt_len));
 				get_page(agg->page);
+				napi_gro_frags(napi);
+			} else {
+				memcpy(skb->data, rx_data, pkt_len);
+				skb_put(skb, pkt_len);
+				skb->protocol = eth_type_trans(skb, netdev);
+				napi_gro_receive(napi, skb);
 			}
 
-			skb->protocol = eth_type_trans(skb, netdev);
-			rtl_rx_vlan_tag(rx_desc, skb);
 			work_done++;
 			stats->rx_packets++;
-			stats->rx_bytes += skb->len;
-			napi_gro_receive(napi, skb);
+			stats->rx_bytes += pkt_len;
 
 find_next_rx:
 			rx_data = rx_agg_align(rx_data + pkt_len + ETH_FCS_LEN);
-- 
2.41.0

Re: [PATCH net-next v2 1/2] r8152: remove queuing rx packets in driver

[Andrew Lunn]

> -	return work_done;
> +	if (work_done > budget)
> +		return budget;
> +	else
> +		return work_done;
>  }

I don't know NAPI too well. Are there implications of not telling it
the truth?

    Andrew

RE: [PATCH net-next v2 1/2] r8152: remove queuing rx packets in driver

[Hayes Wang]

Andrew Lunn <andrew@lunn.ch>
> Sent: Tuesday, September 19, 2023 8:08 PM
[...]
> > -     return work_done;
> > +     if (work_done > budget)
> > +             return budget;
> > +     else
> > +             return work_done;
> >  }
> 
> I don't know NAPI too well. Are there implications of not telling it
> the truth?

You could check the reply from Jakub Kicinski, as following link.
https://www.spinics.net/lists/netdev/msg933846.html
https://www.spinics.net/lists/netdev/msg933923.html

If the work_done is more than budget, I should return budget.

Best Regards,
Hayes

Re: [PATCH net-next v2 1/2] r8152: remove queuing rx packets in driver

[Paolo Abeni]

On Tue, 2023-09-19 at 12:26 +0000, Hayes Wang wrote:
> Andrew Lunn <andrew@lunn.ch>
> > Sent: Tuesday, September 19, 2023 8:08 PM
> [...]
> > > -     return work_done;
> > > +     if (work_done > budget)
> > > +             return budget;
> > > +     else
> > > +             return work_done;
> > >  }
> > 
> > I don't know NAPI too well. Are there implications of not telling it
> > the truth?
> 
> You could check the reply from Jakub Kicinski, as following link.
> https://www.spinics.net/lists/netdev/msg933846.html
> https://www.spinics.net/lists/netdev/msg933923.html
> 
> If the work_done is more than budget, I should return budget.

One of the key points in Jakub's reply is that the burst must exceed
the budget by a limited number of packets:

"Nothing will explode if we process a few more packets than budget
(assuming budget > 0)"

How many packets can contain at most a single URB?

If that number can be significant greater then the napi budget, I think
we are better off keeping the packet in the queue - otherwise the whole
system latency/stability would be affected.

Cheers,

Paolo

RE: [PATCH net-next v2 1/2] r8152: remove queuing rx packets in driver

[Hayes Wang]

Paolo Abeni <pabeni@redhat.com>
> Sent: Thursday, September 21, 2023 9:39 PM
[...]
> One of the key points in Jakub's reply is that the burst must exceed
> the budget by a limited number of packets:
> 
> "Nothing will explode if we process a few more packets than budget
> (assuming budget > 0)"
> 
> How many packets can contain at most a single URB?

I think it depends on
1. if the packets coming continually
2. the size of the packet

The URB would be completed when
1. the device doesn't receive any packet during several us.
2. the desired size is reached.

For 2.5G NIC with the packets of 1514 bytes, a urb at most contains about 21 packets.
If all packet are 60 bytes, the number would be about 390.
If the average size of the packets is 488 bytes, the number is about 64.

> If that number can be significant greater then the napi budget, I think
> we are better off keeping the packet in the queue - otherwise the whole
> system latency/stability would be affected.

Should I go back the patch with queuing the rx packet in driver?

Best Regards,
Hayes