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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <e3ad16c0e8414af6be25f4bf9ab5e1e3@realtek.com>
Date: Mon, 18 Sep 2023 09:28:03 +0000
From: Hayes Wang <hayeswang@...ltek.com>
To: Eric Dumazet <edumazet@...gle.com>
CC: "kuba@...nel.org" <kuba@...nel.org>,
        "davem@...emloft.net"
	<davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        nic_swsd <nic_swsd@...ltek.com>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org"
	<linux-usb@...r.kernel.org>,
        "bjorn@...k.no" <bjorn@...k.no>,
        "pabeni@...hat.com" <pabeni@...hat.com>
Subject: RE: [PATCH net-next resend 1/2] r8152: remove queuing rx packets in driver

Eric Dumazet <edumazet@...gle.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