| Message ID | 1444675083-9825-4-git-send-email-xiyou.wangcong@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 10/12/15 14:38, Cong Wang wrote: > It is odd to see qlen!=0 but backlog==0, for a real example: > Backlog is a transient stat so a lot of times it should be 0. Only when the CPU is sending faster than the link can handle should you see the backlog grow (and eventually drain to 0). Even though your explanation above is inaccurate I think the spirit of the patch looks reasonable. i.e keeping track of all additions to the queue and removals from the queue in the backlog stats is useful. However, you need to be extremely careful: This should only be done at exactly the spot the packet is enqueued (and not by a parent's enqueue asking for hierarchical enques). I think some more work is needed Cong for this general patchset. cheers, jamal > qdisc htb 1: dev eth0 root refcnt 2 r2q 10 default 1 direct_packets_stat 0 ver 3.17 > Sent 172680457356 bytes 222469449 pkt (dropped 0, overlimits 123575834 requeues 0) > backlog 0b 72p requeues 0 > > So we need to update backlog too when we update qlen. > > Cc: Jamal Hadi Salim <jhs@mojatatu.com> > Signed-off-by: Cong Wang <cwang@twopensource.com> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 14, 2015 at 5:25 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > On 10/12/15 14:38, Cong Wang wrote: >> >> It is odd to see qlen!=0 but backlog==0, for a real example: >> > > Backlog is a transient stat so a lot of times it should be 0. Only when > the CPU is sending faster than the link can handle should you see > the backlog grow (and eventually drain to 0). Of course. But in my case, we were sending a burst of traffic while with a lower HTB bw limit, so we can consistently see backlog!=0 for many seconds. > > Even though your explanation above is inaccurate I think the spirit > of the patch looks reasonable. i.e keeping track of all additions to > the queue and removals from the queue in the backlog stats is useful. > However, you need to be extremely careful: This should only be done > at exactly the spot the packet is enqueued (and not by a parent's > enqueue asking for hierarchical enques). The reason why I care about backlog and qlen is I want to know the average length of each packet in backlog, to check if it is a GSO packet at least. > > I think some more work is needed Cong for this general patchset. > Sure, I could miss something somewhere, just point it out. :) Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/15/15 00:21, Cong Wang wrote: > On Wed, Oct 14, 2015 at 5:25 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: >> On 10/12/15 14:38, Cong Wang wrote: >>> >>> It is odd to see qlen!=0 but backlog==0, for a real example: >>> >> >> Backlog is a transient stat so a lot of times it should be 0. Only when >> the CPU is sending faster than the link can handle should you see >> the backlog grow (and eventually drain to 0). > > Of course. But in my case, we were sending a burst of traffic while > with a lower HTB bw limit, so we can consistently see backlog!=0 > for many seconds. > Depending on how much bandwidth you are allowing to be shaped, this observation is expected. HTB (and any non-work conserving qdisc) would keep packets backlogged for as long as you you are instructing it to keep them. If you are sending to the link faster than the shaping rate, qdisc backlog will grow and take a while to drain. [I have seen cases where actually the way the parameters are set is the problem (unfortunately userspace doesnt do a lot of checking for sanity).] Like i said earlier, the idea of keeping track of backlog is useful. I think the commit log threw me off. "It is odd to see qlen!=0 but backlog==0" If you change that commit log then: Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 16, 2015 at 5:19 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > Like i said earlier, the idea of keeping track of backlog is useful. > I think the commit log threw me off. > "It is odd to see qlen!=0 but backlog==0" > If you change that commit log then: Sure, I think DaveM probably already discards this version, I will update the changelog as you pointed out and resend. > > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> > Thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c index 846a7f9..87b02ed3 100644 --- a/net/sched/sch_htb.c +++ b/net/sched/sch_htb.c @@ -600,6 +600,7 @@ static int htb_enqueue(struct sk_buff *skb, struct Qdisc *sch) htb_activate(q, cl); } + qdisc_qstats_backlog_inc(sch, skb); sch->q.qlen++; return NET_XMIT_SUCCESS; } @@ -889,6 +890,7 @@ static struct sk_buff *htb_dequeue(struct Qdisc *sch) ok: qdisc_bstats_update(sch, skb); qdisc_unthrottled(sch); + qdisc_qstats_backlog_dec(sch, skb); sch->q.qlen--; return skb; } @@ -955,6 +957,7 @@ static unsigned int htb_drop(struct Qdisc *sch) unsigned int len; if (cl->un.leaf.q->ops->drop && (len = cl->un.leaf.q->ops->drop(cl->un.leaf.q))) { + sch->qstats.backlog -= len; sch->q.qlen--; if (!cl->un.leaf.q->q.qlen) htb_deactivate(q, cl); @@ -984,12 +987,12 @@ static void htb_reset(struct Qdisc *sch) } cl->prio_activity = 0; cl->cmode = HTB_CAN_SEND; - } } qdisc_watchdog_cancel(&q->watchdog); __skb_queue_purge(&q->direct_queue); sch->q.qlen = 0; + sch->qstats.backlog = 0; memset(q->hlevel, 0, sizeof(q->hlevel)); memset(q->row_mask, 0, sizeof(q->row_mask)); for (i = 0; i < TC_HTB_NUMPRIO; i++)