Re: [PATCH net-next 2/3] net: sched: implement TCQ_F_CAN_BYPASS for lockless qdisc

[Yunsheng Lin]

On 2021/5/29 12:32, Jakub Kicinski wrote:
> On Sat, 29 May 2021 09:44:57 +0800 Yunsheng Lin wrote:
>>>> @@ -3852,10 +3852,32 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
>>>>  	qdisc_calculate_pkt_len(skb, q);
>>>>  
>>>>  	if (q->flags & TCQ_F_NOLOCK) {
>>>> +		if (q->flags & TCQ_F_CAN_BYPASS && nolock_qdisc_is_empty(q) &&
>>>> +		    qdisc_run_begin(q)) {
>>>> +			/* Retest nolock_qdisc_is_empty() within the protection
>>>> +			 * of q->seqlock to ensure qdisc is indeed empty.
>>>> +			 */
>>>> +			if (unlikely(!nolock_qdisc_is_empty(q))) {  
>>>
>>> This is just for the DRAINING case right? 
>>>
>>> MISSED can be set at any moment, testing MISSED seems confusing.  
>>
>> MISSED is only set when there is lock contention, which means it
>> is better not to do the qdisc bypass to avoid out of order packet
>> problem, 
> 
> Avoid as in make less likely? Nothing guarantees other thread is not
> interrupted after ->enqueue and before qdisc_run_begin().
> 
> TBH I'm not sure what out-of-order situation you're referring to,
> there is no ordering guarantee between separate threads trying to
> transmit AFAIU.
A thread need to do the bypass checking before doing enqueuing, so
it means MISSED is set or the trylock fails for the bypass transmiting(
which will set the MISSED after the first trylock), so the MISSED will
always be set before a thread doing a enqueuing, and we ensure MISSED
only be cleared during the protection of q->seqlock, after clearing
MISSED, we do anther round of dequeuing within the protection of
q->seqlock.

So if a thread has taken the q->seqlock and the MISSED is not set yet,
it is allowed to send the packet directly without going through the
qdisc enqueuing and dequeuing process.


> 
> IOW this check is not required for correctness, right?

if a thread has taken the q->seqlock and the MISSED is not set, it means
other thread has not set MISSED after the first trylock and before the
second trylock, which means the enqueuing is not done yet.
So I assume the this check is required for correctness if I understand
your question correctly.

> 
>> another good thing is that we could also do the batch
>> dequeuing and transmiting of packets when there is lock contention.
> 
> No doubt, but did you see the flag get set significantly often here 
> to warrant the double-checking?

No, that is just my guess:)

> 
>>> Is it really worth the extra code?  
>>
>> Currently DRAINING is only set for the netdev queue stopped.
>> We could only use DRAINING to indicate the non-empty of a qdisc,
>> then we need to set the DRAINING evrywhere MISSED is set, that is
>> why I use both DRAINING and MISSED to indicate a non-empty qdisc.