netdev - Re: [PATCH net-next 2/3] net: sched: implement TCQ_F_CAN

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 29 May 2021 11:49:19 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Yunsheng Lin <linyunsheng@...wei.com>
Cc:     <davem@...emloft.net>, <olteanv@...il.com>, <ast@...nel.org>,
        <daniel@...earbox.net>, <andriin@...com>, <edumazet@...gle.com>,
        <weiwan@...gle.com>, <cong.wang@...edance.com>,
        <ap420073@...il.com>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linuxarm@...neuler.org>,
        <mkl@...gutronix.de>, <linux-can@...r.kernel.org>,
        <jhs@...atatu.com>, <xiyou.wangcong@...il.com>, <jiri@...nulli.us>,
        <andrii@...nel.org>, <kafai@...com>, <songliubraving@...com>,
        <yhs@...com>, <john.fastabend@...il.com>, <kpsingh@...nel.org>,
        <bpf@...r.kernel.org>, <jonas.bonn@...rounds.com>,
        <pabeni@...hat.com>, <mzhivich@...mai.com>, <johunt@...mai.com>,
        <albcamus@...il.com>, <kehuan.feng@...il.com>,
        <a.fatoum@...gutronix.de>, <atenart@...nel.org>,
        <alexander.duyck@...il.com>, <hdanton@...a.com>, <jgross@...e.com>,
        <JKosina@...e.com>, <mkubecek@...e.cz>, <bjorn@...nel.org>,
        <alobakin@...me>
Subject: Re: [PATCH net-next 2/3] net: sched: implement TCQ_F_CAN_BYPASS for
 lockless qdisc

On Sat, 29 May 2021 15:03:09 +0800 Yunsheng Lin wrote:
> On 2021/5/29 12:32, Jakub Kicinski wrote:
> > On Sat, 29 May 2021 09:44:57 +0800 Yunsheng Lin wrote:  
> >> 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.

The fact that MISSED is only cleared under q->seqlock does not matter,
because setting it and ->enqueue() are not under any lock. If the thread
gets interrupted between:

	if (q->flags & TCQ_F_CAN_BYPASS && nolock_qdisc_is_empty(q) &&
	    qdisc_run_begin(q)) {

and ->enqueue() we can't guarantee that something else won't come in,
take q->seqlock and clear MISSED.

thread1                thread2             thread3
# holds seqlock
                       qdisc_run_begin(q)
                       set(MISSED)
pfifo_fast_dequeue
  clear(MISSED)
  # recheck the queue
qdisc_run_end()
                       ->enqueue()
                                            q->flags & TCQ_F_CAN_BYPASS..
                                            qdisc_run_begin() # true
                                            sch_direct_xmit()
                       qdisc_run_begin()
                       set(MISSED)

Or am I missing something?

Re-checking nolock_qdisc_is_empty() may or may not help.
But it doesn't really matter because there is no ordering
requirement between thread2 and thread3 here.

> 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:)