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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Qingfang Deng <dqfext@gmail.com> writes:

> Hi Toke,
>
> On Tue, Nov 5, 2024 at 8:24 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Qingfang Deng <dqfext@gmail.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...

-Toke