Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
MIME-Version: 1.0
References: <20230918074202.2461-426-nic_swsd@realtek.com> <20230918074202.2461-427-nic_swsd@realtek.com>
 <CANn89iJmdkyn8_hU4esycRG-XvPa_Djsp6PyaOX5cYP1Obdr4g@mail.gmail.com> <7235821eb09242adaa651172729f76aa@realtek.com>
In-Reply-To: <7235821eb09242adaa651172729f76aa@realtek.com>
From:   Eric Dumazet <edumazet@google.com>
Date:   Mon, 18 Sep 2023 10:52:54 +0200
Message-ID: <CANn89i+Tou=YwteEd5ceaHP54sZpkRotwcV6YWAs4jAUq=ocJg@mail.gmail.com>
Subject: Re: [PATCH net-next resend 1/2] r8152: remove queuing rx packets in driver
To:     Hayes Wang <hayeswang@realtek.com>
Cc:     "kuba@kernel.org" <kuba@kernel.org>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        nic_swsd <nic_swsd@realtek.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "bjorn@mork.no" <bjorn@mork.no>,
        "pabeni@redhat.com" <pabeni@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Sep 18, 2023 at 10:39=E2=80=AFAM Hayes Wang <hayeswang@realtek.com>=
 wrote:
>
> Eric Dumazet <edumazet@google.com>
> > Sent: Monday, September 18, 2023 3:55 PM
> [...]
> > >                         urb->actual_length =3D 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 >=3D 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 th=
an 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 w=
ould
> 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 bul=
k 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
>