[BUG] net/sched : qlen can not really be per cpu ?

[BUG] net/sched : qlen can not really be per cpu ?

[Eric Dumazet]

HTB + pfifo_fast as a leaf qdisc hits badly the following warning in htb_activate() :

WARN_ON(cl->level || !cl->leaf.q || !cl->leaf.q->q.qlen);

This is because pfifo_fast does not update sch->q.qlen, but per cpu counters.
So cl->leaf.q->q.qlen is zero.

HFSC, CBQ, DRR, QFQ  have the same problem.

Any ideas how we can fix this ?

Thanks !

Re: [BUG] net/sched : qlen can not really be per cpu ?

[Eric Dumazet]

On 02/25/2019 10:42 PM, Eric Dumazet wrote:
> HTB + pfifo_fast as a leaf qdisc hits badly the following warning in htb_activate() :
> 
> WARN_ON(cl->level || !cl->leaf.q || !cl->leaf.q->q.qlen);
> 
> This is because pfifo_fast does not update sch->q.qlen, but per cpu counters.
> So cl->leaf.q->q.qlen is zero.
> 
> HFSC, CBQ, DRR, QFQ  have the same problem.
> 
> Any ideas how we can fix this ?

What about something simple for stable ?
( I yet have to boot/test this )

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 9481f2c142e26ee1174653d673e6134edd9851da..3a9e442fcaaf2ea48ae65bc87ee95f59cd7100c8 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -51,7 +51,10 @@ struct qdisc_size_table {
 struct qdisc_skb_head {
        struct sk_buff  *head;
        struct sk_buff  *tail;
-       __u32           qlen;
+       union {
+               __u32           qlen;
+               atomic_t        atomic_qlen;
+       };
        spinlock_t      lock;
 };
 
@@ -408,27 +411,19 @@ static inline void qdisc_cb_private_validate(const struct sk_buff *skb, int sz)
        BUILD_BUG_ON(sizeof(qcb->data) < sz);
 }
 
-static inline int qdisc_qlen_cpu(const struct Qdisc *q)
-{
-       return this_cpu_ptr(q->cpu_qstats)->qlen;
-}
-
 static inline int qdisc_qlen(const struct Qdisc *q)
 {
        return q->q.qlen;
 }
 
-static inline int qdisc_qlen_sum(const struct Qdisc *q)
+static inline u32 qdisc_qlen_sum(const struct Qdisc *q)
 {
-       __u32 qlen = q->qstats.qlen;
-       int i;
+       u32 qlen = q->qstats.qlen;
 
-       if (q->flags & TCQ_F_NOLOCK) {
-               for_each_possible_cpu(i)
-                       qlen += per_cpu_ptr(q->cpu_qstats, i)->qlen;
-       } else {
+       if (q->flags & TCQ_F_NOLOCK)
+               qlen += atomic_read(&q->q.atomic_qlen);
+       else
                qlen += q->q.qlen;
-       }
 
        return qlen;
 }
@@ -827,12 +822,12 @@ static inline void qdisc_qstats_cpu_backlog_inc(struct Qdisc *sch,
 
 static inline void qdisc_qstats_cpu_qlen_inc(struct Qdisc *sch)
 {
-       this_cpu_inc(sch->cpu_qstats->qlen);
+       atomic_inc(&sch->q.atomic_qlen);
 }
 
 static inline void qdisc_qstats_cpu_qlen_dec(struct Qdisc *sch)
 {
-       this_cpu_dec(sch->cpu_qstats->qlen);
+       atomic_dec(&sch->q.atomic_qlen);
 }
 
 static inline void qdisc_qstats_cpu_requeues_inc(struct Qdisc *sch)

Re: [BUG] net/sched : qlen can not really be per cpu ?

[Cong Wang]

On Tue, Feb 26, 2019 at 3:19 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 02/25/2019 10:42 PM, Eric Dumazet wrote:
> > HTB + pfifo_fast as a leaf qdisc hits badly the following warning in htb_activate() :
> >
> > WARN_ON(cl->level || !cl->leaf.q || !cl->leaf.q->q.qlen);
> >
> > This is because pfifo_fast does not update sch->q.qlen, but per cpu counters.
> > So cl->leaf.q->q.qlen is zero.
> >
> > HFSC, CBQ, DRR, QFQ  have the same problem.
> >
> > Any ideas how we can fix this ?
>
> What about something simple for stable ?
> ( I yet have to boot/test this )

Is merely updating qlen sufficient for fixing it?

I thought it is because of the lack of qdisc_tree_reduce_backlog()
in pfifo_fast.

Re: [BUG] net/sched : qlen can not really be per cpu ?

[Eric Dumazet]

On 02/26/2019 03:51 PM, Cong Wang wrote:
> On Tue, Feb 26, 2019 at 3:19 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>>
>>
>> On 02/25/2019 10:42 PM, Eric Dumazet wrote:
>>> HTB + pfifo_fast as a leaf qdisc hits badly the following warning in htb_activate() :
>>>
>>> WARN_ON(cl->level || !cl->leaf.q || !cl->leaf.q->q.qlen);
>>>
>>> This is because pfifo_fast does not update sch->q.qlen, but per cpu counters.
>>> So cl->leaf.q->q.qlen is zero.
>>>
>>> HFSC, CBQ, DRR, QFQ  have the same problem.
>>>
>>> Any ideas how we can fix this ?
>>
>> What about something simple for stable ?
>> ( I yet have to boot/test this )
> 
> Is merely updating qlen sufficient for fixing it?
> 
> I thought it is because of the lack of qdisc_tree_reduce_backlog()
> in pfifo_fast.

It does not seem to be the qdisc_tree_reduce_backlog() thing.

HTB, HFSC, CBQ, DRR, QFQ only peek at their children 'qlen' to decide if there
is at least one packet in them.

The backlog is only reported for dumps, but the actual backlog value is not used in data path.

Re: [BUG] net/sched : qlen can not really be per cpu ?

[Eric Dumazet]

On 02/26/2019 04:56 PM, Eric Dumazet wrote:
> 
> 
> On 02/26/2019 03:51 PM, Cong Wang wrote:
>> On Tue, Feb 26, 2019 at 3:19 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>>
>>>
>>>
>>> On 02/25/2019 10:42 PM, Eric Dumazet wrote:
>>>> HTB + pfifo_fast as a leaf qdisc hits badly the following warning in htb_activate() :
>>>>
>>>> WARN_ON(cl->level || !cl->leaf.q || !cl->leaf.q->q.qlen);
>>>>
>>>> This is because pfifo_fast does not update sch->q.qlen, but per cpu counters.
>>>> So cl->leaf.q->q.qlen is zero.
>>>>
>>>> HFSC, CBQ, DRR, QFQ  have the same problem.
>>>>
>>>> Any ideas how we can fix this ?
>>>
>>> What about something simple for stable ?
>>> ( I yet have to boot/test this )
>>
>> Is merely updating qlen sufficient for fixing it?
>>
>> I thought it is because of the lack of qdisc_tree_reduce_backlog()
>> in pfifo_fast.
> 
> It does not seem to be the qdisc_tree_reduce_backlog() thing.
> 
> HTB, HFSC, CBQ, DRR, QFQ only peek at their children 'qlen' to decide if there
> is at least one packet in them.
> 
> The backlog is only reported for dumps, but the actual backlog value is not used in data path.
> 
> 

Another way to fix this would be to have a shadow version of pfifo_fast, which
basically would be the old version of it, that would be automatically selected
when used as a child of another qdisc (except mq/mqprio of course)

This seems not a stable candidate though.

Re: [BUG] net/sched : qlen can not really be per cpu ?

[Cong Wang]

On Tue, Feb 26, 2019 at 4:56 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 02/26/2019 03:51 PM, Cong Wang wrote:
> > On Tue, Feb 26, 2019 at 3:19 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >>
> >>
> >>
> >> On 02/25/2019 10:42 PM, Eric Dumazet wrote:
> >>> HTB + pfifo_fast as a leaf qdisc hits badly the following warning in htb_activate() :
> >>>
> >>> WARN_ON(cl->level || !cl->leaf.q || !cl->leaf.q->q.qlen);
> >>>
> >>> This is because pfifo_fast does not update sch->q.qlen, but per cpu counters.
> >>> So cl->leaf.q->q.qlen is zero.
> >>>
> >>> HFSC, CBQ, DRR, QFQ  have the same problem.
> >>>
> >>> Any ideas how we can fix this ?
> >>
> >> What about something simple for stable ?
> >> ( I yet have to boot/test this )
> >
> > Is merely updating qlen sufficient for fixing it?
> >
> > I thought it is because of the lack of qdisc_tree_reduce_backlog()
> > in pfifo_fast.
>
> It does not seem to be the qdisc_tree_reduce_backlog() thing.
>
> HTB, HFSC, CBQ, DRR, QFQ only peek at their children 'qlen' to decide if there
> is at least one packet in them.
>
> The backlog is only reported for dumps, but the actual backlog value is not used in data path.
>

Hmm, looking into this, do we really need to check cl->leaf.q->q.qlen
in htb_activate() for pfifo_fast? htb_activate() is only called when
qdisc_enqueue() returns NET_XMIT_SUCCESS, so for pfifo_fast
that is always qlen!=0, right?

So something like below? It is ugly but should be sufficient to shut
up the warning.

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 30f9da7e1076..6d0182750f8b 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -555,7 +555,8 @@ htb_change_class_mode(struct htb_sched *q, struct
htb_class *cl, s64 *diff)
  */
 static inline void htb_activate(struct htb_sched *q, struct htb_class *cl)
 {
-       WARN_ON(cl->level || !cl->leaf.q || !cl->leaf.q->q.qlen);
+       WARN_ON(cl->level || !cl->leaf.q ||
+               (!(cl->leaf.q->flags & TCQ_F_NOLOCK) && !cl->leaf.q->q.qlen));

        if (!cl->prio_activity) {
                cl->prio_activity = 1 << cl->prio;

Re: [BUG] net/sched : qlen can not really be per cpu ?

[Eric Dumazet]

On 02/27/2019 06:46 PM, Cong Wang wrote:

> Hmm, looking into this, do we really need to check cl->leaf.q->q.qlen
> in htb_activate() for pfifo_fast? htb_activate() is only called when
> qdisc_enqueue() returns NET_XMIT_SUCCESS, so for pfifo_fast
> that is always qlen!=0, right?
> 
> So something like below? It is ugly but should be sufficient to shut
> up the warning.
> 
> diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
> index 30f9da7e1076..6d0182750f8b 100644
> --- a/net/sched/sch_htb.c
> +++ b/net/sched/sch_htb.c
> @@ -555,7 +555,8 @@ htb_change_class_mode(struct htb_sched *q, struct
> htb_class *cl, s64 *diff)
>   */
>  static inline void htb_activate(struct htb_sched *q, struct htb_class *cl)
>  {
> -       WARN_ON(cl->level || !cl->leaf.q || !cl->leaf.q->q.qlen);
> +       WARN_ON(cl->level || !cl->leaf.q ||
> +               (!(cl->leaf.q->flags & TCQ_F_NOLOCK) && !cl->leaf.q->q.qlen));
> 
>         if (!cl->prio_activity) {
>                 cl->prio_activity = 1 << cl->prio;
> 

Well, this is the tip of the iceberg.

Look at lines 845 & 883

How are we going to fix them ?

(Then there are all the other qdisc I mentioned)