On 7/2/20 2:45 AM, Paolo Abeni wrote:
Hi all,

On Thu, 2020-07-02 at 08:14 +0200, Jonas Bonn wrote:
Hi Cong,

On 01/07/2020 21:58, Cong Wang wrote:
On Wed, Jul 1, 2020 at 9:05 AM Cong Wang <xiyou.wangc...@gmail.com> wrote:
On Tue, Jun 30, 2020 at 2:08 PM Josh Hunt <joh...@akamai.com> wrote:
Do either of you know if there's been any development on a fix for this
issue? If not we can propose something.

If you have a reproducer, I can look into this.

Does the attached patch fix this bug completely?

It's easier to comment if you inline the patch, but after taking a quick
look it seems too simplistic.

i)  Are you sure you haven't got the return values on qdisc_run reversed?

qdisc_run() returns true if it was able to acquire the seq lock. We
need to take special action in the opposite case, so Cong's patch LGTM
from a functional PoV.

ii) There's a "bypass" path that skips the enqueue/dequeue operation if
the queue is empty; that needs a similar treatment:  after releasing
seqlock it needs to ensure that another packet hasn't been enqueued
since it last checked.

That has been reverted with
commit 379349e9bc3b42b8b2f8f7a03f64a97623fff323

diff --git a/net/core/dev.c b/net/core/dev.c
index 90b59fc50dc9..c7e48356132a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3744,7 +3744,8 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, 
struct Qdisc *q,

        if (q->flags & TCQ_F_NOLOCK) {
                rc = q->enqueue(skb, q, &to_free) & NET_XMIT_MASK;
-               qdisc_run(q);
+               if (!qdisc_run(q) && rc == NET_XMIT_SUCCESS)
+                       __netif_schedule(q);

I fear the __netif_schedule() call may cause performance regression to
the point of making a revert of TCQ_F_NOLOCK preferable. I'll try to
collect some data.

Initial results with Cong's patch look promising, so far no stalls. We will let it run over the long weekend and report back on Tuesday.

Paolo - I have concerns about possible performance regression with the change as well. If you can gather some data that would be great. If things look good with our low throughput test over the weekend we can also try assessing performance next week.


Reply via email to