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 <jgr...@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;
> }
>
>
> .
>
