| Message ID | 1620610956-56306-4-git-send-email-linyunsheng@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | fix packet stuck problem for lockless qdisc | expand |
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
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? > > . >
On 2021/5/11 17:04, Yunsheng Lin wrote: > 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? 3. Or check the netif_xmit_frozen_or_stopped() again after clearing STATE_MISSED, like below: if (netif_xmit_frozen_or_stopped(txq)) { clear_bit(__QDISC_STATE_MISSED, &q->state); /* Make sure the below netif_xmit_frozen_or_stopped() * checking happens after clearing STATE_MISSED. */ smp_mb__after_atomic(); /* Checking netif_xmit_frozen_or_stopped() again to * make sure __QDISC_STATE_MISSED is set if the * __QDISC_STATE_MISSED set by netif_tx_wake_queue()'s * rescheduling of net_tx_action() is cleared by the * above clear_bit(). */ if (!netif_xmit_frozen_or_stopped(txq)) set_bit(__QDISC_STATE_MISSED, &q->state); } It is kind of ugly, but it does seem to fix the above data race too. And it seems like a common pattern to deal with the concurrency between xmit and NAPI polling, as below: https://elixir.bootlin.com/linux/v5.12-rc2/source/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c#L1409 > >> >> . >> > _______________________________________________ > Linuxarm mailing list -- linuxarm@openeuler.org > To unsubscribe send an email to linuxarm-leave@openeuler.org >
On Tue, 11 May 2021 20:13:56 +0800 Yunsheng Lin wrote: > On 2021/5/11 17:04, Yunsheng Lin wrote: > > On 2021/5/11 12:22, Jakub Kicinski wrote: > >> 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() > [...] > > > > 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. Isn't the case where we have a NOLOCK qdisc without TCQ_F_ONETXQUEUE rather unexpected? It'd have to be a single pfifo on multi-queue netdev, right? Sounds not worth optimizing for. How about: static inline void qdisc_run_end(struct Qdisc *qdisc) { write_seqcount_end(&qdisc->running); if (qdisc->flags & TCQ_F_NOLOCK) { spin_unlock(&qdisc->seqlock); if (unlikely(test_bit(__QDISC_STATE_MISSED, &qdisc->state))) { clear_bit(__QDISC_STATE_MISSED, &qdisc->state); if (!(q->flags & TCQ_F_ONETXQUEUE) || !netif_xmit_frozen_or_stopped(q->dev_queue)) __netif_schedule(qdisc); } } } For the strange non-ONETXQUEUE case we'd have an occasional unnecessary net_tx_action, but no infinite loop possible. > > 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? > > 3. Or check the netif_xmit_frozen_or_stopped() again after clearing > STATE_MISSED, like below: > > if (netif_xmit_frozen_or_stopped(txq)) { > clear_bit(__QDISC_STATE_MISSED, &q->state); > > /* Make sure the below netif_xmit_frozen_or_stopped() > * checking happens after clearing STATE_MISSED. > */ > smp_mb__after_atomic(); > > /* Checking netif_xmit_frozen_or_stopped() again to > * make sure __QDISC_STATE_MISSED is set if the > * __QDISC_STATE_MISSED set by netif_tx_wake_queue()'s > * rescheduling of net_tx_action() is cleared by the > * above clear_bit(). > */ > if (!netif_xmit_frozen_or_stopped(txq)) > set_bit(__QDISC_STATE_MISSED, &q->state); > } > > It is kind of ugly, but it does seem to fix the above data race too. > And it seems like a common pattern to deal with the concurrency between > xmit and NAPI polling, as below: > > https://elixir.bootlin.com/linux/v5.12-rc2/source/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c#L1409 This is indeed the idiomatic way of dealing with Tx queue stopping race, but it's a bit of code to sprinkle around. My vote would be option 1.
On 2021/5/12 7:30, Jakub Kicinski wrote: > On Tue, 11 May 2021 20:13:56 +0800 Yunsheng Lin wrote: >> On 2021/5/11 17:04, Yunsheng Lin wrote: >>> On 2021/5/11 12:22, Jakub Kicinski wrote: >>>> 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() >> [...] >>> >>> 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. > > Isn't the case where we have a NOLOCK qdisc without TCQ_F_ONETXQUEUE > rather unexpected? It'd have to be a single pfifo on multi-queue > netdev, right? Sounds not worth optimizing for. How about: > > static inline void qdisc_run_end(struct Qdisc *qdisc) > { > write_seqcount_end(&qdisc->running); > if (qdisc->flags & TCQ_F_NOLOCK) { > spin_unlock(&qdisc->seqlock); > > if (unlikely(test_bit(__QDISC_STATE_MISSED, > &qdisc->state))) { > clear_bit(__QDISC_STATE_MISSED, &qdisc->state); > if (!(q->flags & TCQ_F_ONETXQUEUE) || > !netif_xmit_frozen_or_stopped(q->dev_queue)) > __netif_schedule(qdisc); > } > } > } > > For the strange non-ONETXQUEUE case we'd have an occasional unnecessary > net_tx_action, but no infinite loop possible. > >>> 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? >> >> 3. Or check the netif_xmit_frozen_or_stopped() again after clearing >> STATE_MISSED, like below: >> >> if (netif_xmit_frozen_or_stopped(txq)) { >> clear_bit(__QDISC_STATE_MISSED, &q->state); >> >> /* Make sure the below netif_xmit_frozen_or_stopped() >> * checking happens after clearing STATE_MISSED. >> */ >> smp_mb__after_atomic(); >> >> /* Checking netif_xmit_frozen_or_stopped() again to >> * make sure __QDISC_STATE_MISSED is set if the >> * __QDISC_STATE_MISSED set by netif_tx_wake_queue()'s >> * rescheduling of net_tx_action() is cleared by the >> * above clear_bit(). >> */ >> if (!netif_xmit_frozen_or_stopped(txq)) >> set_bit(__QDISC_STATE_MISSED, &q->state); >> } >> >> It is kind of ugly, but it does seem to fix the above data race too. >> And it seems like a common pattern to deal with the concurrency between >> xmit and NAPI polling, as below: >> >> https://elixir.bootlin.com/linux/v5.12-rc2/source/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c#L1409 > > This is indeed the idiomatic way of dealing with Tx queue stopping race, > but it's a bit of code to sprinkle around. My vote would be option 1. I had done some performance testing to see which is better, tested using pktgen and dummy netdev with pfifo_fast qdisc attached: unit: Mpps threads V6 V6 + option 1 V6 + option 3 1 2.60 2.54 2.60 2 3.86 3.84 3.84 4 5.56 5.50 5.51 8 2.79 2.77 2.77 16 2.23 2.24 2.22 So it seems the netif_xmit_frozen_or_stopped checking overhead for non-stopped queue is noticable for 1 pktgen thread. And the performance increase for V6 + option 1 with 16 pktgen threads is because of "clear_bit(__QDISC_STATE_MISSED, &qdisc->state)" at the end of qdisc_run_end(), which may avoid the another round of dequeuing in the pfifo_fast_dequeue(). And adding the "clear_bit(__QDISC_STATE_MISSED, &qdisc->state)" for V6 + option 3, the data for 16 pktgen thread also go up to 2.24Mpps. So it seems V6 + option 3 with "clear_bit(__QDISC_STATE_MISSED, &qdisc->state)" at the end of qdisc_run_end() is better? > _______________________________________________ > Linuxarm mailing list -- linuxarm@openeuler.org > To unsubscribe send an email to linuxarm-leave@openeuler.org >
On Wed, 12 May 2021 11:34:55 +0800 Yunsheng Lin wrote: > > This is indeed the idiomatic way of dealing with Tx queue stopping race, > > but it's a bit of code to sprinkle around. My vote would be option 1. > > I had done some performance testing to see which is better, tested using > pktgen and dummy netdev with pfifo_fast qdisc attached: > > unit: Mpps > > threads V6 V6 + option 1 V6 + option 3 > 1 2.60 2.54 2.60 > 2 3.86 3.84 3.84 > 4 5.56 5.50 5.51 > 8 2.79 2.77 2.77 > 16 2.23 2.24 2.22 > > So it seems the netif_xmit_frozen_or_stopped checking overhead for non-stopped queue > is noticable for 1 pktgen thread. > > And the performance increase for V6 + option 1 with 16 pktgen threads is because of > "clear_bit(__QDISC_STATE_MISSED, &qdisc->state)" at the end of qdisc_run_end(), which > may avoid the another round of dequeuing in the pfifo_fast_dequeue(). And adding the > "clear_bit(__QDISC_STATE_MISSED, &qdisc->state)" for V6 + option 3, the data for > 16 pktgen thread also go up to 2.24Mpps. > > > So it seems V6 + option 3 with "clear_bit(__QDISC_STATE_MISSED, &qdisc->state)" at > the end of qdisc_run_end() is better? Alright, sounds good.
diff --git a/net/core/dev.c b/net/core/dev.c index d596cd7..ef8cf76 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -3853,7 +3853,8 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q, if (q->flags & TCQ_F_NOLOCK) { rc = q->enqueue(skb, q, &to_free) & NET_XMIT_MASK; - qdisc_run(q); + if (likely(!netif_xmit_frozen_or_stopped(txq))) + qdisc_run(q); if (unlikely(to_free)) kfree_skb_list(to_free); diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index d86c4cc..37403c1 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -74,6 +74,7 @@ static inline struct sk_buff *__skb_dequeue_bad_txq(struct Qdisc *q) } } else { skb = SKB_XOFF_MAGIC; + clear_bit(__QDISC_STATE_MISSED, &q->state); } } @@ -242,6 +243,7 @@ static struct sk_buff *dequeue_skb(struct Qdisc *q, bool *validate, } } else { skb = NULL; + clear_bit(__QDISC_STATE_MISSED, &q->state); } if (lock) spin_unlock(lock); @@ -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; + } skb = qdisc_dequeue_skb_bad_txq(q); if (unlikely(skb)) { @@ -311,6 +315,8 @@ bool sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q, HARD_TX_LOCK(dev, txq, smp_processor_id()); if (!netif_xmit_frozen_or_stopped(txq)) skb = dev_hard_start_xmit(skb, dev, txq, &ret); + else + clear_bit(__QDISC_STATE_MISSED, &q->state); HARD_TX_UNLOCK(dev, txq); } else {
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> --- V6: Drop NET_XMIT_DROP checking for it is not really relevant to this patch, and it may cause performance performance regression with multi pktgen threads on dummy netdev with pfifo_fast qdisc case. --- net/core/dev.c | 3 ++- net/sched/sch_generic.c | 8 +++++++- 2 files changed, 9 insertions(+), 2 deletions(-)