Re: [BUG] pfifo_fast may cause out-of-order CAN frame transmission
From: Paolo Abeni <pabeni@redhat.com>
Date: 2020-01-10 16:31:12
Also in:
netdev
Subsystem:
networking [general], tc subsystem, the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Jamal Hadi Salim, Jiri Pirko, Linus Torvalds
On Thu, 2020-01-09 at 18:39 +0100, Ahmad Fatoum wrote:
Hello Paolo, On 1/9/20 1:51 PM, Paolo Abeni wrote:quoted
On Wed, 2020-01-08 at 15:55 +0100, Ahmad Fatoum wrote:quoted
I've run into an issue of CAN frames being sent out-of-order on an i.MX6 Dual with Linux v5.5-rc5. Bisecting has lead me down to this commit:Thank you for the report.Thanks for the prompt patch. :-)quoted
The code is only build-tested, could you please try it in your setup?Issue still persists, albeit appears to have become much less frequent. Took 2 million frames till first two were swapped. What I usually saw was a swap every few thousand frames at least and quite often more frequent than that. Might just be noise though.
Thank you for testing. Even with the proposed patch there is still a possible race condition: the CPU holding the seqlock can clear the 'empty' flag after that the CPU xmitting the packet enqueue it and set the 'empty' flag. The only option I can think of - beyond plain revert - is updating the 'empty' flag in a even a more coarse way, as in the following patch. Again, the code only build tested and very rough, but it would be helpful if you could give it a spin. Thank you! Paolo ---
diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index 6a70845bd9ab..fb365fbf65f8 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h@@ -113,7 +113,7 @@ bool sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q, struct net_device *dev, struct netdev_queue *txq, spinlock_t *root_lock, bool validate); -void __qdisc_run(struct Qdisc *q); +int __qdisc_run(struct Qdisc *q); static inline void qdisc_run(struct Qdisc *q) {
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index fceddf89592a..df460fe0773a 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h@@ -158,7 +158,6 @@ static inline bool qdisc_run_begin(struct Qdisc *qdisc) if (qdisc->flags & TCQ_F_NOLOCK) { if (!spin_trylock(&qdisc->seqlock)) return false; - WRITE_ONCE(qdisc->empty, false); } else if (qdisc_is_running(qdisc)) { return false; }
diff --git a/net/core/dev.c b/net/core/dev.c
index 0ad39c87b7fd..b6378bb7b64a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c@@ -3624,10 +3624,22 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q, end_run: qdisc_run_end(q); } else { + int quota = 0; + rc = q->enqueue(skb, q, &to_free) & NET_XMIT_MASK; - qdisc_run(q); + if (!qdisc_run_begin(q)) + goto out; + + WRITE_ONCE(q->empty, false); + if (likely(!test_bit(__QDISC_STATE_DEACTIVATED, + &q->state))) + quota = __qdisc_run(q); + if (quota > 0) + WRITE_ONCE(q->empty, true); + qdisc_run_end(q); } +out: if (unlikely(to_free)) kfree_skb_list(to_free); return rc;
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 5ab696efca95..1bd2c4e9c4c2 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c@@ -376,7 +376,7 @@ static inline bool qdisc_restart(struct Qdisc *q, int *packets) return sch_direct_xmit(skb, q, dev, txq, root_lock, validate); } -void __qdisc_run(struct Qdisc *q) +int __qdisc_run(struct Qdisc *q) { int quota = dev_tx_weight; int packets;
@@ -388,6 +388,7 @@ void __qdisc_run(struct Qdisc *q) break; } } + return quota; } unsigned long dev_trans_start(struct net_device *dev)
@@ -649,12 +650,9 @@ static struct sk_buff *pfifo_fast_dequeue(struct Qdisc *qdisc) skb = __skb_array_consume(q); } - if (likely(skb)) { - qdisc_update_stats_at_dequeue(qdisc, skb); - } else { - WRITE_ONCE(qdisc->empty, true); - } + if (likely(skb)) + qdisc_update_stats_at_dequeue(qdisc, skb); return skb; }