Re: [PATCH RFC] net: sched: implement TCQ_F_CAN_BYPASS for lockless qdisc

From: Vladimir Oltean <olteanv@gmail.com>
To: Yunsheng Lin <linyunsheng@huawei.com>
Cc: davem@davemloft.net, kuba@kernel.org, ast@kernel.org,
	daniel@iogearbox.net, andriin@fb.com, edumazet@google.com,
	weiwan@google.com, cong.wang@bytedance.com, ap420073@gmail.com,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	linuxarm@openeuler.org, pabeni@redhat.com
Subject: Re: [PATCH RFC] net: sched: implement TCQ_F_CAN_BYPASS for lockless qdisc
Date: Sun, 14 Mar 2021 02:03:50 +0200	[thread overview]
Message-ID: <20210314000350.2mrhvprsi77qwqdi@skbuf> (raw)
In-Reply-To: <1615603667-22568-1-git-send-email-linyunsheng@huawei.com>

Hi Yunsheng,

On Sat, Mar 13, 2021 at 10:47:47AM +0800, Yunsheng Lin wrote:
> Currently pfifo_fast has both TCQ_F_CAN_BYPASS and TCQ_F_NOLOCK
> flag set, but queue discipline by-pass does not work for lockless
> qdisc because skb is always enqueued to qdisc even when the qdisc
> is empty, see __dev_xmit_skb().
> 
> This patch calles sch_direct_xmit() to transmit the skb directly
> to the driver for empty lockless qdisc too, which aviod enqueuing
> and dequeuing operation. qdisc->empty is set to false whenever a
> skb is enqueued, and is set to true when skb dequeuing return NULL,
> see pfifo_fast_dequeue().
> 
> Also, qdisc is scheduled at the end of qdisc_run_end() when q->empty
> is false to avoid packet stuck problem.
> 
> The performance for ip_forward test increases about 10% with this
> patch.
> 
> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> ---

I can confirm the ~10% IP forwarding throughput improvement brought by
this patch, but as you might be aware, there was a previous attempt to
add qdisc bypass to pfifo_fast by Paolo Abeni:
https://lore.kernel.org/netdev/661cc33a-5f65-2769-cc1a-65791cb4b131@pengutronix.de/
It was reverted because TX reordering was observed with SocketCAN
(although, presumably it should also be seen with Ethernet and such).

In fact I have a setup with two NXP LS1028A-RDB boards (which use the
drivers/net/can/flexcan.c driver and the pfifo_fast qdisc):

 +-----------+                      +-----------+
 |           |                      |           |
 | Generator |                      |     DUT   |
 |           |--------------------->|           |
 | canfdtest |           reflector  | canfdtest |
 |           |<---------------------|           |
 |    can1   |                      |    can0   |
 |           |                      |           |
 +-----------+                      +-----------+

where reordering happens in the TX side of the DUT and is noticed in the
RX side of the generator. The test frames are classic CAN, not CAN FD.

I was able to run the canfdtest described above successfully for several
hours (10 million CAN frames) on the current net-next (HEAD at commit
34bb97512641 ("net: fddi: skfp: Mundane typo fixes throughout the file
smt.h")) with no reordering.

Then, after applying your patch, I am seeing TX reordering within a few
minutes (less than 100K frames sent), therefore this reintroduces the
bug due to which Paolo's patch was reverted.

Sadly I am not knowledgeable enough to give you any hints as to what is
going wrong, but in case you have any ideas for debug, I would be glad
to test them out on my boards.