Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: Packet gets stuck in NOLOCK pfifo_fast qdisc
To:     Paolo Abeni <pabeni@redhat.com>,
        Jonas Bonn <jonas.bonn@netrounds.com>,
        Cong Wang <xiyou.wangcong@gmail.com>
Cc:     Michael Zhivich <mzhivich@akamai.com>,
        David Miller <davem@davemloft.net>,
        John Fastabend <john.fastabend@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Linux Kernel Network Developers <netdev@vger.kernel.org>
References: <465a540e-5296-32e7-f6a6-79942dfe2618@netrounds.com>
 <20200623134259.8197-1-mzhivich@akamai.com>
 <1849b74f-163c-8cfa-baa5-f653159fefd4@akamai.com>
 <CAM_iQpX1+dHB0kJF8gRfuDeAb9TsA9mB9H_Og8n8Hr19+EMLJA@mail.gmail.com>
 <CAM_iQpWjQiG-zVs+e-V=8LvTFbRwgC4y4eoGERjezfAT0Fmm8g@mail.gmail.com>
 <7fd86d97-6785-0b5f-1e95-92bc1da9df35@netrounds.com>
 <500b4843cb7c425ea5449fe199095edd5f7feb0c.camel@redhat.com>
From:   Josh Hunt <johunt@akamai.com>
Message-ID: <25ca46e4-a8c1-1c88-d6a9-603289ff44c3@akamai.com>
Date:   Thu, 2 Jul 2020 11:08:43 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.8.0
MIME-Version: 1.0
In-Reply-To: <500b4843cb7c425ea5449fe199095edd5f7feb0c.camel@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.wangcong@gmail.com> wrote:
>>>> On Tue, Jun 30, 2020 at 2:08 PM Josh Hunt <johunt@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.

Josh