Re: Packet gets stuck in NOLOCK pfifo_fast qdisc

[Fengkehuan Feng <kehuan.feng@gmail.com>]

Hi Hillf,

I just tried the updated version and the system can boot up now.
It does mitigate the issue a lot but still couldn't get rid of it
thoroughly. It seems to me like the effect of Cong's patch.


Hillf Danton <hdanton@sina.com> 于2020年8月25日周二 上午11:23写道：
>
>
> Hi Feng,
>
> On Tue, 25 Aug 2020 10:18:05 +0800 Fengkehuan Feng wrote:
> > 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;
>         }
>  }
>
> --
>