On Tue, Feb 26, 2019 at 4:56 PM Eric Dumazet <eric.duma...@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.duma...@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;
