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

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

[Hayes Wang]

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 | 80 +++++++++++++++++------------------------
 1 file changed, 32 insertions(+), 48 deletions(-)

-- 
2.41.0

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

[Hayes Wang]

The original way would process all rx and queue the rx packets in the
driver. Now, the process would be broken if the budget is exhausted. And
the remained list would be queue back to rx_done for next schedule.

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 0c13d9950cd8..ae46e7e46e39 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,24 @@ static int rx_bottom(struct r8152 *tp, int budget)
 			urb->actual_length = 0;
 			list_add_tail(&agg->list, next);
 		}
+
+		/* Break if budget is exhausted. */
+		if (work_done >= budget)
+			break;
 	}
 
+	/* Splice the remained list back to rx_done */
 	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 +2973,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

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

[Eric Dumazet]

On Mon, Sep 18, 2023 at 9:42 AM Hayes Wang <hayeswang@realtek.com> wrote:
>
> The original way would process all rx and queue the rx packets in the
> driver. Now, the process would be broken if the budget is exhausted. And
> the remained list would be queue back to rx_done for next schedule.
>
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>

This deserves a Fixes: tag

> ---
>  drivers/net/usb/r8152.c | 52 ++++++++++++-----------------------------
>  1 file changed, 15 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 0c13d9950cd8..ae46e7e46e39 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,24 @@ static int rx_bottom(struct r8152 *tp, int budget)
>                         urb->actual_length = 0;
>                         list_add_tail(&agg->list, next);
>                 }
> +
> +               /* Break if budget is exhausted. */

[1] More conventional way to to put this condition at the beginning of
the while () loop,
because the budget could be zero.

> +               if (work_done >= budget)
> +                       break;
>         }
>
> +       /* Splice the remained list back to rx_done */
>         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)

This (work_done >budget) condition would never be true if point [1] is
addressed.

> +               return budget;
> +       else
> +               return work_done;
>  }
>
>  static void tx_bottom(struct r8152 *tp)
> @@ -2992,9 +2973,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
>

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

[Hayes Wang]

Eric Dumazet <edumazet@google.com>
> Sent: Monday, September 18, 2023 3:55 PM
[...]
> >                         urb->actual_length = 0;
> >                         list_add_tail(&agg->list, next);
> >                 }
> > +
> > +               /* Break if budget is exhausted. */
> 
> [1] More conventional way to to put this condition at the beginning of
> the while () loop,
> because the budget could be zero.

If the budget is zero, the function wouldn't be called.
a7b8d60b3723 ("r8152: check budget for r8152_poll") avoids it.

> > +               if (work_done >= budget)
> > +                       break;
> >         }
> >
> > +       /* Splice the remained list back to rx_done */
> >         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)
> 
> This (work_done >budget) condition would never be true if point [1] is
> addressed.

A bulk transfer may contain many packets, so the work_done may be more than budget.
That is why I queue the packets in the driver before this patch.
For example, if a bulk transfer contains 70 packet and budget is 64,
napi_gro_receive would be called for the first 64 packets and 6 packets would
be queued in driver for next schedule. After this patch, napi_gro_receive() would
be called for the 70 packets, even the budget is 64. And the remained bulk transfers
would be handled for next schedule.

> > +               return budget;
> > +       else
> > +               return work_done;
> >  }

Best Regards,
Hayes

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

[Eric Dumazet]

On Mon, Sep 18, 2023 at 10:39 AM Hayes Wang <hayeswang@realtek.com> wrote:
>
> Eric Dumazet <edumazet@google.com>
> > Sent: Monday, September 18, 2023 3:55 PM
> [...]
> > >                         urb->actual_length = 0;
> > >                         list_add_tail(&agg->list, next);
> > >                 }
> > > +
> > > +               /* Break if budget is exhausted. */
> >
> > [1] More conventional way to to put this condition at the beginning of
> > the while () loop,
> > because the budget could be zero.
>
> If the budget is zero, the function wouldn't be called.
> a7b8d60b3723 ("r8152: check budget for r8152_poll") avoids it.

Yes, and we could revert  this patch :/

Moving the test at the front of the loop like most other drivers would
have avoided this issue,
and avoided this discussion.

>
> > > +               if (work_done >= budget)
> > > +                       break;
> > >         }
> > >
> > > +       /* Splice the remained list back to rx_done */
> > >         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)
> >
> > This (work_done >budget) condition would never be true if point [1] is
> > addressed.
>
> A bulk transfer may contain many packets, so the work_done may be more than budget.
> That is why I queue the packets in the driver before this patch.
> For example, if a bulk transfer contains 70 packet and budget is 64,
> napi_gro_receive would be called for the first 64 packets and 6 packets would
> be queued in driver for next schedule. After this patch, napi_gro_receive() would
> be called for the 70 packets, even the budget is 64. And the remained bulk transfers
> would be handled for next schedule.

A comment would be nice. NAPI logic should look the same in all drivers.

If a driver has some peculiarities, comments would help to maintain
the code in the long run.

>
> > > +               return budget;
> > > +       else
> > > +               return work_done;
> > >  }
>
> Best Regards,
> Hayes
>

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

[Hayes Wang]

Eric Dumazet <edumazet@google.com>
> Sent: Monday, September 18, 2023 4:53 PM
[...]
> > > [1] More conventional way to to put this condition at the beginning of
> > > the while () loop,
> > > because the budget could be zero.
> >
> > If the budget is zero, the function wouldn't be called.
> > a7b8d60b3723 ("r8152: check budget for r8152_poll") avoids it.
> 
> Yes, and we could revert  this patch :/
> 
> Moving the test at the front of the loop like most other drivers would
> have avoided this issue,
> and avoided this discussion.

I don't do that because I want to avoid some checks and spin lock before and after
the loop. For example,

1. spin lock
2. move the ready lists to local
3. spin unlock
4. loop the lists
5. break the loop if budget is zero
6. spin lock
7. move the remained list back for next schedule
8. spin unlock

I could avoid the redundant behavior.

> > > > +               if (work_done >= budget)
> > > > +                       break;
> > > >         }
> > > >
> > > > +       /* Splice the remained list back to rx_done */
> > > >         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)
> > >
> > > This (work_done >budget) condition would never be true if point [1] is
> > > addressed.
> >
> > A bulk transfer may contain many packets, so the work_done may be more
> than budget.
> > That is why I queue the packets in the driver before this patch.
> > For example, if a bulk transfer contains 70 packet and budget is 64,
> > napi_gro_receive would be called for the first 64 packets and 6 packets
> would
> > be queued in driver for next schedule. After this patch, napi_gro_receive()
> would
> > be called for the 70 packets, even the budget is 64. And the remained bulk
> transfers
> > would be handled for next schedule.
> 
> A comment would be nice. NAPI logic should look the same in all drivers.
> 
> If a driver has some peculiarities, comments would help to maintain
> the code in the long run.

I would update it. Thanks.

Best Regards,
Hayes

[PATCH net-next resend 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 ae46e7e46e39..d872fbb5b6ff 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