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

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

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
>
  

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
>
  

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