From: Yunsheng Lin <linyunsheng@huawei.com>
To: Jakub Kicinski <kuba@kernel.org>
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 v6 3/3] net: sched: fix tx action reschedule issue with stopped queue
Date: Tue, 11 May 2021 17:04:59 +0800 [thread overview]
Message-ID: <c676404c-f210-b0cb-ced3-5449676055a8@huawei.com> (raw)
In-Reply-To: <20210510212232.3386c5b4@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On 2021/5/11 12:22, Jakub Kicinski wrote:
> On Mon, 10 May 2021 09:42:36 +0800 Yunsheng Lin wrote:
>> The netdev qeueue might be stopped when byte queue limit has
>> reached or tx hw ring is full, net_tx_action() may still be
>> rescheduled endlessly if STATE_MISSED is set, which consumes
>> a lot of cpu without dequeuing and transmiting any skb because
>> the netdev queue is stopped, see qdisc_run_end().
>>
>> This patch fixes it by checking the netdev queue state before
>> calling qdisc_run() and clearing STATE_MISSED if netdev queue is
>> stopped during qdisc_run(), the net_tx_action() is recheduled
>> again when netdev qeueue is restarted, see netif_tx_wake_queue().
>>
>> Fixes: 6b3ba9146fe6 ("net: sched: allow qdiscs to handle locking")
>> Reported-by: Michal Kubecek <mkubecek@suse.cz>
>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
>
> Patches 1 and 2 look good to me but this one I'm not 100% sure.
>
>> @@ -251,8 +253,10 @@ static struct sk_buff *dequeue_skb(struct Qdisc *q, bool *validate,
>> *validate = true;
>>
>> if ((q->flags & TCQ_F_ONETXQUEUE) &&
>> - netif_xmit_frozen_or_stopped(txq))
>> + netif_xmit_frozen_or_stopped(txq)) {
>> + clear_bit(__QDISC_STATE_MISSED, &q->state);
>> return skb;
>> + }
>
> The queues are woken asynchronously without holding any locks via
> netif_tx_wake_queue(). Theoretically we can have a situation where:
>
> CPU 0 CPU 1
> . .
> dequeue_skb() .
> netif_xmit_frozen..() # true .
> . [IRQ]
> . netif_tx_wake_queue()
> . <end of IRQ>
> . netif_tx_action()
> . set MISSED
> clear MISSED
> return NULL
> ret from qdisc_restart()
> ret from __qdisc_run()
> qdisc_run_end()
> -> MISSED not set
Yes, the above does seems to have the above data race.
As my understanding, there is two ways to fix the above data race:
1. do not clear the STATE_MISSED for netif_xmit_frozen_or_stopped()
case, just check the netif_xmit_frozen_or_stopped() before
calling __netif_schedule() at the end of qdisc_run_end(). This seems
to only work with qdisc with TCQ_F_ONETXQUEUE flag because it seems
we can only check the netif_xmit_frozen_or_stopped() with q->dev_queue,
I am not sure q->dev_queue is pointint to which netdev queue when qdisc
is not set with TCQ_F_ONETXQUEUE flag.
2. clearing the STATE_MISSED for netif_xmit_frozen_or_stopped() case
as this patch does, and protect the __netif_schedule() with q->seqlock
for netif_tx_wake_queue(), which might bring unnecessary overhead for
non-stopped queue case
Any better idea?
>
> .
>
next prev parent reply other threads:[~2021-05-11 9:05 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-10 1:42 [PATCH net v6 0/3] fix packet stuck problem for lockless qdisc Yunsheng Lin
2021-05-10 1:42 ` [PATCH net v6 1/3] net: sched: " Yunsheng Lin
2021-05-10 1:42 ` [PATCH net v6 2/3] net: sched: fix endless tx action reschedule during deactivation Yunsheng Lin
2021-05-10 1:42 ` [PATCH net v6 3/3] net: sched: fix tx action reschedule issue with stopped queue Yunsheng Lin
2021-05-11 4:22 ` Jakub Kicinski
2021-05-11 9:04 ` Yunsheng Lin [this message]
2021-05-11 12:13 ` Yunsheng Lin
2021-05-11 23:30 ` Jakub Kicinski
2021-05-12 3:34 ` [Linuxarm] " Yunsheng Lin
2021-05-12 19:47 ` Jakub Kicinski
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=c676404c-f210-b0cb-ced3-5449676055a8@huawei.com \
--to=linyunsheng@huawei.com \
--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=kuba@kernel.org \
--cc=linux-can@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxarm@openeuler.org \
--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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.