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: [PATCH net v3] net: sched: fix packet stuck problem for lockless
 qdisc
To:     Hillf Danton <hdanton@sina.com>, Juergen Gross <jgross@suse.com>
CC:     <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Jiri Kosina <JKosina@suse.com>
References: <1616641991-14847-1-git-send-email-linyunsheng@huawei.com>
 <20210409090909.1767-1-hdanton@sina.com>
From:   Yunsheng Lin <linyunsheng@huawei.com>
Message-ID: <9cb8d8e3-89e5-2b00-ad16-43f34d6b1340@huawei.com>
Date:   Mon, 12 Apr 2021 09:24:30 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.0
MIME-Version: 1.0
In-Reply-To: <20210409090909.1767-1-hdanton@sina.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2021/4/9 17:09, Hillf Danton wrote:
> On Fri, 9 Apr 2021 07:31:03  Juergen Gross wrote:
>> On 25.03.21 04:13, Yunsheng Lin wrote:
>> I have a setup which is able to reproduce the issue quite reliably:
>>
>> In a Xen guest I'm mounting 8 NFS shares and run sysbench fileio on
>> each of them. The average latency reported by sysbench is well below
>> 1 msec, but at least once per hour I get latencies in the minute
>> range.
>>
>> With this patch I don't see these high latencies any longer (test
>> is running for more than 20 hours now).
>>
>> So you can add my:
>>
>> Tested-by: Juergen Gross <jgross@suse.com>
>>
> 
> If retry is allowed in the dequeue method then a simple seqcount can do the
> work of serializing enqueuer and dequeuer. IIUC it was not attempted last year.

At the first glance, I do not think the below patch fix the data race
described in the commit log, as it does not handle the time window
between dequeuing and q->seqlock releasing, as below:

The cpu1 may not see the qdisc->pad changed after pfifo_fast_dequeue(),
and cpu2 is not able to take the q->seqlock yet because cpu1 do not
release the q->seqlock.

> 
> --- x/net/sched/sch_generic.c
> +++ y/net/sched/sch_generic.c
> @@ -632,6 +632,9 @@ static int pfifo_fast_enqueue(struct sk_
>  			return qdisc_drop(skb, qdisc, to_free);
>  	}
>  
> +	qdisc->pad++;
> +	smp_wmb();
> +
>  	qdisc_update_stats_at_enqueue(qdisc, pkt_len);
>  	return NET_XMIT_SUCCESS;
>  }
> @@ -641,6 +644,11 @@ static struct sk_buff *pfifo_fast_dequeu
>  	struct pfifo_fast_priv *priv = qdisc_priv(qdisc);
>  	struct sk_buff *skb = NULL;
>  	int band;
> +	int seq;
> +
> +again:
> +	seq = READ_ONCE(qdisc->pad);
> +	smp_rmb();
>  
>  	for (band = 0; band < PFIFO_FAST_BANDS && !skb; band++) {
>  		struct skb_array *q = band2list(priv, band);
> @@ -652,10 +660,15 @@ static struct sk_buff *pfifo_fast_dequeu
>  	}
>  	if (likely(skb)) {
>  		qdisc_update_stats_at_dequeue(qdisc, skb);
> -	} else {
> -		WRITE_ONCE(qdisc->empty, true);
> +		return skb;
>  	}
>  
> +	smp_rmb();
> +	if (seq != READ_ONCE(qdisc->pad))
> +		goto again;
> +
> +	WRITE_ONCE(qdisc->empty, true);
> +
>  	return skb;
>  }
>  
> 
> .
>