* [BUG] net/sched : qlen can not really be per cpu ? @ 2019-02-26 6:42 Eric Dumazet 2019-02-26 23:19 ` Eric Dumazet 0 siblings, 1 reply; 7+ messages in thread From: Eric Dumazet @ 2019-02-26 6:42 UTC (permalink / raw) To: John Fastabend, Networking, Jamal Hadi Salim, Cong Wang, Jiri Pirko 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 ! ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG] net/sched : qlen can not really be per cpu ? 2019-02-26 6:42 [BUG] net/sched : qlen can not really be per cpu ? Eric Dumazet @ 2019-02-26 23:19 ` Eric Dumazet 2019-02-26 23:51 ` Cong Wang 0 siblings, 1 reply; 7+ messages in thread From: Eric Dumazet @ 2019-02-26 23:19 UTC (permalink / raw) To: Eric Dumazet, John Fastabend, Networking, Jamal Hadi Salim, Cong Wang, Jiri Pirko 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) ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [BUG] net/sched : qlen can not really be per cpu ? 2019-02-26 23:19 ` Eric Dumazet @ 2019-02-26 23:51 ` Cong Wang 2019-02-27 0:56 ` Eric Dumazet 0 siblings, 1 reply; 7+ messages in thread From: Cong Wang @ 2019-02-26 23:51 UTC (permalink / raw) To: Eric Dumazet; +Cc: John Fastabend, Networking, Jamal Hadi Salim, Jiri Pirko 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG] net/sched : qlen can not really be per cpu ? 2019-02-26 23:51 ` Cong Wang @ 2019-02-27 0:56 ` Eric Dumazet 2019-02-27 16:59 ` Eric Dumazet 2019-02-28 2:46 ` Cong Wang 0 siblings, 2 replies; 7+ messages in thread From: Eric Dumazet @ 2019-02-27 0:56 UTC (permalink / raw) To: Cong Wang; +Cc: John Fastabend, Networking, Jamal Hadi Salim, Jiri Pirko 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG] net/sched : qlen can not really be per cpu ? 2019-02-27 0:56 ` Eric Dumazet @ 2019-02-27 16:59 ` Eric Dumazet 2019-02-28 2:46 ` Cong Wang 1 sibling, 0 replies; 7+ messages in thread From: Eric Dumazet @ 2019-02-27 16:59 UTC (permalink / raw) To: Cong Wang; +Cc: John Fastabend, Networking, Jamal Hadi Salim, Jiri Pirko 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG] net/sched : qlen can not really be per cpu ? 2019-02-27 0:56 ` Eric Dumazet 2019-02-27 16:59 ` Eric Dumazet @ 2019-02-28 2:46 ` Cong Wang 2019-02-28 5:15 ` Eric Dumazet 1 sibling, 1 reply; 7+ messages in thread From: Cong Wang @ 2019-02-28 2:46 UTC (permalink / raw) To: Eric Dumazet; +Cc: John Fastabend, Networking, Jamal Hadi Salim, Jiri Pirko 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; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [BUG] net/sched : qlen can not really be per cpu ? 2019-02-28 2:46 ` Cong Wang @ 2019-02-28 5:15 ` Eric Dumazet 0 siblings, 0 replies; 7+ messages in thread From: Eric Dumazet @ 2019-02-28 5:15 UTC (permalink / raw) To: Cong Wang; +Cc: John Fastabend, Networking, Jamal Hadi Salim, Jiri Pirko 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) ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-02-28 5:15 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-26 6:42 [BUG] net/sched : qlen can not really be per cpu ? Eric Dumazet 2019-02-26 23:19 ` Eric Dumazet 2019-02-26 23:51 ` Cong Wang 2019-02-27 0:56 ` Eric Dumazet 2019-02-27 16:59 ` Eric Dumazet 2019-02-28 2:46 ` Cong Wang 2019-02-28 5:15 ` Eric Dumazet
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).