Re: Packet gets stuck in NOLOCK pfifo_fast qdisc

Hi Feng,

On Tue, 25 Aug 2020 10:18:05 +0800 Fengkehuan Feng wrote:

quoted

Hillf,

With the latest version (attached what I have changed on my tree), the
system failed to start up with cpu stalled.

My fault.

There is a missing break while running qdisc and it's fixed
in the diff below for Linux-5.x.

If it is Linux-4.x in your testing, running qdisc looks a bit
different based on your diff(better if it's in the message body):

 static inline void qdisc_run(struct Qdisc *q)
 {
-       if (qdisc_run_begin(q)) {
+       while (qdisc_run_begin(q)) {
+               int seq = q->pkt_seq;
                __qdisc_run(q);
                qdisc_run_end(q);
+
+               /* go another round if there are pkts enqueued after
+                * taking seq_lock
+                */
+               if (seq != q->pkt_seq)
+                       continue;
+               else
+                       return;
        }
 }


Hillf

--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h

@@ -79,6 +79,7 @@ struct Qdisc {
 #define TCQ_F_INVISIBLE                0x80 /* invisible by default in dump */
 #define TCQ_F_NOLOCK           0x100 /* qdisc does not require locking */
 #define TCQ_F_OFFLOADED                0x200 /* qdisc is offloaded to HW */
+       int                     pkt_seq;
        u32                     limit;
        const struct Qdisc_ops  *ops;
        struct qdisc_size_table __rcu *stab;
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c

@@ -631,6 +631,7 @@ static int pfifo_fast_enqueue(struct sk_
                        return qdisc_drop(skb, qdisc, to_free);
        }

+       qdisc->pkt_seq++;
        qdisc_update_stats_at_enqueue(qdisc, pkt_len);
        return NET_XMIT_SUCCESS;
 }
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h

@@ -117,14 +117,27 @@ void __qdisc_run(struct Qdisc *q);

 static inline void qdisc_run(struct Qdisc *q)
 {
-       if (qdisc_run_begin(q)) {
+       while (qdisc_run_begin(q)) {
+               int seq = q->pkt_seq;
+               bool check_seq = false;
+
                /* NOLOCK qdisc must check 'state' under the qdisc seqlock
                 * to avoid racing with dev_qdisc_reset()
                 */
                if (!(q->flags & TCQ_F_NOLOCK) ||
-                   likely(!test_bit(__QDISC_STATE_DEACTIVATED, &q->state)))
+                   likely(!test_bit(__QDISC_STATE_DEACTIVATED, &q->state))) {
                        __qdisc_run(q);
+                       check_seq = true;
+               }
                qdisc_run_end(q);
+
+               /* go another round if there are pkts enqueued after
+                * taking seq_lock
+                */
+               if (check_seq && seq != q->pkt_seq)
+                       continue;
+               else
+                       return;
        }
 }

--