[PATCH] HTB updates class's bstats in one place

[PATCH] HTB updates class's bstats in one place

[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.

Signed-off-by: Changli Gao <xiaosuo@gmail.com <mailto:xiaosuo@gmail.com>>
---
sch_htb.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)

--- net/sched/sch_htb.c.orig	2008-10-29 16:18:13.000000000 +0800
+++ net/sched/sch_htb.c	2008-10-29 16:19:54.000000000 +0800
@@ -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;
 	}
 }

Re: [PATCH] HTB updates class's bstats in one place

[Patrick McHardy]

Changli Gao wrote:
> 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.

Re: [PATCH] HTB updates class's bstats in one place

[Changli Gao]

On Wed, Oct 29, 2008 at 5:01 PM, Patrick McHardy <kaber@trash.net> wrote:
> 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?
-- 
Regards，
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH] HTB updates class's bstats in one place

[Patrick McHardy]

Changli Gao wrote:
> On Wed, Oct 29, 2008 at 5:01 PM, Patrick McHardy <kaber@trash.net> wrote:
>> 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.

Re: [PATCH] HTB updates class's bstats in one place

[Jarek Poplawski]

On 29-10-2008 10:19, Patrick McHardy wrote:
> Changli Gao wrote:
>> On Wed, Oct 29, 2008 at 5:01 PM, Patrick McHardy <kaber@trash.net> wrote:
>>> 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.

Re: [PATCH] HTB updates class's bstats in one place

[Patrick McHardy]

Jarek Poplawski wrote:
> On 29-10-2008 10:19, Patrick McHardy wrote:
>> Changli Gao wrote:
>>> On Wed, Oct 29, 2008 at 5:01 PM, Patrick McHardy <kaber@trash.net> wrote:
>>>> 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.

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.

Re: [PATCH] HTB updates class's bstats in one place

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Wed, 29 Oct 2008 11:33:25 +0100

> Jarek Poplawski wrote:
> > 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

Re: [PATCH] HTB updates class's bstats in one place

[Changli Gao]

On Wed, Oct 29, 2008 at 6:33 PM, Patrick McHardy <kaber@trash.net> wrote:
> 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?

-- 
Regards，
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH] HTB updates class's bstats in one place

[Patrick McHardy]

Changli Gao wrote:
> On Wed, Oct 29, 2008 at 6:33 PM, Patrick McHardy <kaber@trash.net> wrote:
>> 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?

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.

Re: [PATCH] HTB updates class's bstats in one place

[Changli Gao]

On Thu, Oct 30, 2008 at 3:11 PM, Patrick McHardy <kaber@trash.net> wrote:
>
> 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?


-- 
Regards，
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH] HTB updates class's bstats in one place

[Patrick McHardy]

Changli Gao wrote:
> On Thu, Oct 30, 2008 at 3:11 PM, Patrick McHardy <kaber@trash.net> wrote:
>> 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?

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.