From: Jakub Kicinski <kuba@kernel.org>
To: Yunsheng Lin <linyunsheng@huawei.com>
Cc: <davem@davemloft.net>, <olteanv@gmail.com>, <ast@kernel.org>,
<daniel@iogearbox.net>, <andriin@fb.com>, <edumazet@google.com>,
<weiwan@google.com>, <cong.wang@bytedance.com>,
<ap420073@gmail.com>, <netdev@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <linuxarm@openeuler.org>,
<mkl@pengutronix.de>, <linux-can@vger.kernel.org>,
<jhs@mojatatu.com>, <xiyou.wangcong@gmail.com>,
<jiri@resnulli.us>, <andrii@kernel.org>, <kafai@fb.com>,
<songliubraving@fb.com>, <yhs@fb.com>, <john.fastabend@gmail.com>,
<kpsingh@kernel.org>, <bpf@vger.kernel.org>,
<jonas.bonn@netrounds.com>, <pabeni@redhat.com>,
<mzhivich@akamai.com>, <johunt@akamai.com>, <albcamus@gmail.com>,
<kehuan.feng@gmail.com>, <a.fatoum@pengutronix.de>,
<atenart@kernel.org>, <alexander.duyck@gmail.com>,
<hdanton@sina.com>, <jgross@suse.com>, <JKosina@suse.com>,
<mkubecek@suse.cz>, <bjorn@kernel.org>, <alobakin@pm.me>
Subject: Re: [PATCH net-next 2/3] net: sched: implement TCQ_F_CAN_BYPASS for lockless qdisc
Date: Fri, 28 May 2021 21:32:18 -0700 [thread overview]
Message-ID: <20210528213218.2b90864c@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net> (raw)
In-Reply-To: <a6a965ee-7368-d37b-9c70-bba50c67eec9@huawei.com>
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.
IOW this check is not required for correctness, right?
> 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?
> > 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.
>
> >
> >> + rc = q->enqueue(skb, q, &to_free) & NET_XMIT_MASK;
> >> + __qdisc_run(q);
> >> + qdisc_run_end(q);
> >> +
> >> + goto no_lock_out;
> >> + }
> >> +
> >> + qdisc_bstats_cpu_update(q, skb);
> >> + if (sch_direct_xmit(skb, q, dev, txq, NULL, true) &&
> >> + !nolock_qdisc_is_empty(q))
> >> + __qdisc_run(q);
> >> +
> >> + qdisc_run_end(q);
> >> + return NET_XMIT_SUCCESS;
> >> + }
> >> +
> >> rc = q->enqueue(skb, q, &to_free) & NET_XMIT_MASK;
> >> - if (likely(!netif_xmit_frozen_or_stopped(txq)))
> >> - qdisc_run(q);
> >> + qdisc_run(q);
> >>
> >> +no_lock_out:
> >> if (unlikely(to_free))
> >> kfree_skb_list(to_free);
> >> return rc;
> >> @@ -164,9 +166,13 @@ static inline void dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
> >>
> >> skb = next;
> >> }
> >> - if (lock)
> >> +
> >> + if (lock) {
> >> spin_unlock(lock);
> >> - __netif_schedule(q);
> >> + set_bit(__QDISC_STATE_MISSED, &q->state);
> >> + } else {
> >> + __netif_schedule(q);
> >
> > Could we reorder qdisc_run_begin() with clear_bit(SCHED)
> > in net_tx_action() and add SCHED to the NON_EMPTY mask?
>
> Did you mean clearing the SCHED after the q->seqlock is
> taken?
>
> The problem is that the SCHED is also used to indicate
> a qdisc is in sd->output_queue or not, and the
> qdisc_run_begin() called by net_tx_action() can not
> guarantee it will take the q->seqlock(we are using trylock
> for lockless qdisc)
Ah, right. We'd need to do some more flag juggling in net_tx_action()
to get it right.
> >> + }
> >> }
> >>
> >> static void try_bulk_dequeue_skb(struct Qdisc *q,
> >> @@ -409,7 +415,11 @@ void __qdisc_run(struct Qdisc *q)
> >> while (qdisc_restart(q, &packets)) {
> >> quota -= packets;
> >> if (quota <= 0) {
> >> - __netif_schedule(q);
> >> + if (q->flags & TCQ_F_NOLOCK)
> >> + set_bit(__QDISC_STATE_MISSED, &q->state);
> >> + else
> >> + __netif_schedule(q);
> >> +
> >> break;
> >> }
> >> }
> >> @@ -680,13 +690,14 @@ static struct sk_buff *pfifo_fast_dequeue(struct Qdisc *qdisc)
> >> if (likely(skb)) {
> >> qdisc_update_stats_at_dequeue(qdisc, skb);
> >> } else if (need_retry &&
> >> - test_bit(__QDISC_STATE_MISSED, &qdisc->state)) {
> >> + READ_ONCE(qdisc->state) & QDISC_STATE_NON_EMPTY) {
> >
> > Do we really need to retry based on DRAINING being set?
> > Or is it just a convenient way of coding things up?
>
> Yes, it is just a convenient way of coding things up.
> Only MISSED need retrying.
next prev parent reply other threads:[~2021-05-29 4:32 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-28 2:49 [PATCH net-next 0/3] Some optimization for lockless qdisc Yunsheng Lin
2021-05-28 2:49 ` [PATCH net-next 1/3] net: sched: avoid unnecessary seqcount operation " Yunsheng Lin
2021-05-28 2:49 ` [PATCH net-next 2/3] net: sched: implement TCQ_F_CAN_BYPASS " Yunsheng Lin
2021-05-29 1:00 ` Jakub Kicinski
2021-05-29 1:44 ` Yunsheng Lin
2021-05-29 4:32 ` Jakub Kicinski [this message]
2021-05-29 7:03 ` Yunsheng Lin
2021-05-29 18:49 ` Jakub Kicinski
2021-05-30 1:37 ` Yunsheng Lin
2021-05-30 20:21 ` Jakub Kicinski
2021-05-31 0:40 ` Yunsheng Lin
2021-05-31 1:10 ` [Linuxarm] " Yunsheng Lin
2021-05-31 12:40 ` Yunsheng Lin
2021-06-01 4:51 ` Jakub Kicinski
2021-06-01 8:18 ` Yunsheng Lin
2021-06-01 20:48 ` Jakub Kicinski
2021-06-02 1:21 ` Yunsheng Lin
2021-06-02 16:28 ` Jakub Kicinski
2021-05-28 2:49 ` [PATCH net-next 3/3] net: sched: remove qdisc->empty " Yunsheng Lin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210528213218.2b90864c@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net \
--to=kuba@kernel.org \
--cc=JKosina@suse.com \
--cc=a.fatoum@pengutronix.de \
--cc=albcamus@gmail.com \
--cc=alexander.duyck@gmail.com \
--cc=alobakin@pm.me \
--cc=andrii@kernel.org \
--cc=andriin@fb.com \
--cc=ap420073@gmail.com \
--cc=ast@kernel.org \
--cc=atenart@kernel.org \
--cc=bjorn@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=cong.wang@bytedance.com \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=hdanton@sina.com \
--cc=jgross@suse.com \
--cc=jhs@mojatatu.com \
--cc=jiri@resnulli.us \
--cc=john.fastabend@gmail.com \
--cc=johunt@akamai.com \
--cc=jonas.bonn@netrounds.com \
--cc=kafai@fb.com \
--cc=kehuan.feng@gmail.com \
--cc=kpsingh@kernel.org \
--cc=linux-can@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxarm@openeuler.org \
--cc=linyunsheng@huawei.com \
--cc=mkl@pengutronix.de \
--cc=mkubecek@suse.cz \
--cc=mzhivich@akamai.com \
--cc=netdev@vger.kernel.org \
--cc=olteanv@gmail.com \
--cc=pabeni@redhat.com \
--cc=songliubraving@fb.com \
--cc=weiwan@google.com \
--cc=xiyou.wangcong@gmail.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).