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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <87msid98dc.fsf@toke.dk>
Date: Tue, 05 Nov 2024 13:23:59 +0100
From: Toke Høiland-Jørgensen <toke@...hat.com>
To: Qingfang Deng <dqfext@...il.com>, "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

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!

-Toke