Re: [PATCH net v3] net: sched: fix packet stuck problem for lockless qdisc
From: Yunsheng Lin <hidden>
Date: 2021-04-12 01:24:36
Also in:
lkml
On 2021/4/9 17:09, Hillf Danton wrote:
On Fri, 9 Apr 2021 07:31:03 Juergen Gross wrote:quoted
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.
quoted hunk
--- 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; }.