Re: [PATCH net v3] net: sched: fix packet stuck problem for lockless qdisc

[Yunsheng Lin <linyunsheng@huawei.com>]

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