HTB updates class's bstats in one place
Subsystems (2)
PATCH 1/1 HTB updates class's bstats in one place
This patch make HTB update class's bstats in one place when dequeuing packets instead of updating the leaf's when enqueuing and updating the other's when dequeuing. It also fixes a statistics bug, the statistics data of leaf class will exceeds its parent classes's due to packets are dropped by its inner qdisc.
---sch_htb.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
sched/sch_htb.c renamed from sched/sch_htb.c.orig +4 -9
@@ -579,9 +579,6 @@ }return ret; }else { - cl->bstats.packets += -skb_is_gso (skb)?skb_shinfo (skb)->gso_segs:1 ; - cl->bstats.bytes +=qdisc_pkt_len (skb);htb_activate (q, cl); }
@@ -679,12 +676,10 @@htb_add_to_wait_tree (q, cl, diff); } -/* update byte stats except for leaves which are already updated */ -if (cl->level) { - cl->bstats.bytes += bytes; - cl->bstats.packets +=skb_is_gso (skb)? -skb_shinfo (skb)->gso_segs:1 ; - } + cl->bstats.bytes += bytes; + cl->bstats.packets +=skb_is_gso (skb)? +skb_shinfo (skb)->gso_segs:1 ; + cl = cl->parent; } }
Quoted 5 lines · Changli Gao
This patch make HTB update class's bstats in one place when dequeuing packets instead of updating the leaf's when enqueuing and updating the other's when dequeuing. It also fixes a statistics bug, the statistics data of leaf class will exceeds its parent classes's due to packets are dropped by its inner qdisc.
They should actually all be updated in enqueue. The estimators needs this since they're estimating the arrival rate.
Quoted 4 lines · Patrick McHardy
The estimators needs
this since they're estimating the arrival rate.
I do not agree with you. TC is only for sending packets. Why does its estimator estimating the arrival rate?
Signature · 2 lines
Regards， Changli Gao([email protected])
Quoted 7 lines · Changli Gao
Quoted 4 lines · Patrick McHardy
The estimators needs
this since they're estimating the arrival rate.I do not agree with you. TC is only for sending packets. Why does its estimator estimating the arrival rate?
I suppose because they were intended for policing.
Quoted 10 lines · Patrick McHardy
Quoted 7 lines · Changli Gao
Quoted 4 lines · Patrick McHardy
The estimators needs
this since they're estimating the arrival rate.I do not agree with you. TC is only for sending packets. Why does its estimator estimating the arrival rate?
I suppose because they were intended for policing.
On the other hand, it seems with shaping qdiscs they are more useful to query dequeue rates. BTW, after this patch there is still "a statistics bug": the statistics data of a qdisc can differ from its classes.
Jarek P.
Quoted 7 lines · Jarek Poplawski
Quoted 6 lines · Patrick McHardy
Quoted 5 lines · Changli Gao
Quoted 4 lines · Patrick McHardy
The estimators needs
this since they're estimating the arrival rate.
Quoted 6 lines
Quoted 3 lines
Quoted 2 lines
I do not agree with you. TC is only for sending packets. Why does its estimator estimating the arrival rate?
I suppose because they were intended for policing.
On the other hand, it seems with shaping qdiscs they are more useful to query dequeue rates.
Only for purely informational purposes, for policing you'd want to use arrival rates. Since there are no users of estimators in the kernel besides policers (which use seperate ones), the point is not terribly important. But I would prefer to not start introducing (or extending) inconsistencies among qdisc implementations here, that will only make it harder in case someone finally implements something useful with estimators.
David Miller
maintainerFrom: Patrick McHardy <[email protected]>
Date: Wed, 29 Oct 2008 11:33:25 +0100
Quoted 11 lines
Quoted 2 lines · Jarek Poplawski
On the other hand, it seems with shaping qdiscs they are more useful to query dequeue rates.
Only for purely informational purposes, for policing you'd want to use arrival rates. Since there are no users of estimators in the kernel besides policers (which use seperate ones), the point is not terribly important. But I would prefer to not start introducing (or extending) inconsistencies among qdisc implementations here, that will only make it harder in case someone finally implements something useful with estimators.
I agree with Patrick %100
Quoted 4 lines · Patrick McHardy
Only for purely informational purposes, for policing you'd want to use arrival rates. Since there are no users of estimators in the kernel besides policers (which use seperate ones), the point is not terribly important.
Where can I find sth. about this?
In fact, this bug is encountered when I am trying to implement a statistic daemon in user space. It fetches the classes's statistics data periodicity, then calculates the rate based on the data fetched. In some cases, it reports the leaf classes's rate is over theirs limit, but its ascent's isn't.
I think there must be someone does the same thing as me. So the feature is important for them too. If we don't fix it, how can I archive my requirement? Any comment?
Signature · 2 lines
Regards， Changli Gao([email protected])
Quoted 14 lines · Changli Gao
Quoted 4 lines · Patrick McHardy
Only for purely informational purposes, for policing you'd want to use arrival rates. Since there are no users of estimators in the kernel besides policers (which use seperate ones), the point is not terribly important.
Where can I find sth. about this?
In fact, this bug is encountered when I am trying to implement a statistic daemon in user space. It fetches the classes's statistics data periodicity, then calculates the rate based on the data fetched. In some cases, it reports the leaf classes's rate is over theirs limit, but its ascent's isn't.
Quoted 3 lines
I think there must be someone does the same thing as me. So the feature is important for them too. If we don't fix it, how can I archive my requirement? Any comment?
Independant of this statistics bug, according to your description you want to measure departure rates (otherwise the measured rates can't really be compared to the limit) and the current counters are not really suitable for this since they count arriving packets. So the only way to properly do this is to add seperate counters that account for departing packets. I think this would be a good addition since we've always been advocating to do this stuff in userspace, but it would need some thought on how to minimize the impact for people not needing it.
Quoted 12 lines · Patrick McHardy
Independant of this statistics bug, according to your description you want to measure departure rates (otherwise the measured rates can't really be compared to the limit) and the current counters are not really suitable for this since they count arriving packets. So the only way to properly do this is to add seperate counters that account for departing packets. I think this would be a good addition since we've always been advocating to do this stuff in userspace, but it would need some thought on how to minimize the impact for people not needing it.
In fact, only the counters of the leaf classes represent arriving rates, but the others represent departing rates. Is it right? Or does it need to be fixed?
Signature · 2 lines
Regards， Changli Gao([email protected])
Quoted 12 lines · Changli Gao
Quoted 11 lines · Patrick McHardy
Independant of this statistics bug, according to your description you want to measure departure rates (otherwise the measured rates can't really be compared to the limit) and the current counters are not really suitable for this since they count arriving packets. So the only way to properly do this is to add seperate counters that account for departing packets. I think this would be a good addition since we've always been advocating to do this stuff in userspace, but it would need some thought on how to minimize the impact for people not needing it.
Quoted 3 lines
In fact, only the counters of the leaf classes represent arriving rates, but the others represent departing rates. Is it right? Or does it need to be fixed?
Its not correct, but I wouldn't fix it at this time unless you can suggest a good way that doesn't require to walk the entire hierarchy during ->enqueue() just for the class counters.