netdev - Re: [RFC PATCH net-next] net: ppp: convert to IFF_NO

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CALW65jZtdy8xkNnMD2pCFKyf6JM4uwTHgt8v49M46GpCfS8cCw@mail.gmail.com>
Date: Tue, 5 Nov 2024 23:04:44 +0800
From: Qingfang Deng <dqfext@...il.com>
To: Toke Høiland-Jørgensen <toke@...hat.com>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-ppp@...r.kernel.org
Subject: Re: [RFC PATCH net-next] net: ppp: convert to IFF_NO_QUEUE

On Tue, Nov 5, 2024 at 10:35 PM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>
> Qingfang Deng <dqfext@...il.com> writes:
>
> > Hi Toke,
> >
> > On Tue, Nov 5, 2024 at 8:24 PM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
> >>
> >> Qingfang Deng <dqfext@...il.com> writes:
> >>
> >> > When testing the parallel TX performance of a single PPPoE interface
> >> > over a 2.5GbE link with multiple hardware queues, the throughput could
> >> > not exceed 1.9Gbps, even with low CPU usage.
> >> >
> >> > This issue arises because the PPP interface is registered with a single
> >> > queue and a tx_queue_len of 3. This default behavior dates back to Linux
> >> > 2.3.13, which was suitable for slower serial ports. However, in modern
> >> > devices with multiple processors and hardware queues, this configuration
> >> > can lead to congestion.
> >> >
> >> > For PPPoE/PPTP, the lower interface should handle qdisc, so we need to
> >> > set IFF_NO_QUEUE.
> >>
> >> This bit makes sense - the PPPoE and PPTP channel types call through to
> >> the underlying network stack, and their start_xmit() ops never return
> >> anything other than 1 (so there's no pushback against the upper PPP
> >> device anyway). The same goes for the L2TP PPP channel driver.
> >>
> >> > For PPP over a serial port, we don't benefit from a qdisc with such a
> >> > short TX queue, so handling TX queueing in the driver and setting
> >> > IFF_NO_QUEUE is more effective.
> >>
> >> However, this bit is certainly not true. For the channel drivers that
> >> do push back (which is everything apart from the three mentioned above,
> >> AFAICT), we absolutely do want a qdisc to store the packets, instead of
> >> this arbitrary 32-packet FIFO inside the driver. Your comment about the
> >> short TX queue only holds for the pfifo_fast qdisc (that's the only one
> >> that uses the tx_queue_len for anything), anything else will do its own
> >> thing.
> >>
> >> (Side note: don't use pfifo_fast!)
> >>
> >> I suppose one option here could be to set the IFF_NO_QUEUE flag
> >> conditionally depending on whether the underlying channel driver does
> >> pushback against the PPP device or not (add a channel flag to indicate
> >> this, or something), and then call the netif_{wake,stop}_queue()
> >> functions conditionally depending on this. But setting the noqueue flag
> >> unconditionally like this patch does, is definitely not a good idea!
> >
> > I agree. Then the problem becomes how to test if a PPP device is a PPPoE.
> > It seems like PPPoE is the only one that implements
> > ops->fill_forward_path, should I use that? Or is there a better way?
>
> Just add a new field to struct ppp_channel and have the PPoE channel
> driver set that? Could be a flags field, or even just a 'bool
> direct_xmit' field...

Okay, I'll send a patch later.

>
> -Toke
>