tc linklayer ADSL calc broken after commit 56b765b79 (htb: improved accuracy at high rates)

tc linklayer ADSL calc broken after commit 56b765b79 (htb: improved accuracy at high rates)

[Jesper Dangaard Brouer]

I recently discovered that the (traffic control) tc linklayer
calculations for ATM/ADSL have been broken by:
 commit 56b765b79 (htb: improved accuracy at high rates).

Thus, people shaping on ADSL links, using e.g.:
 tc class add ... htb rate X ceil Y linklayer atm overhead 10

Will no-longer get ATM cell tax/overhead adjusted.

How can we solve/fix this?
Perhaps we can change to use the "stab" system instead (as it does
not seem to be broken by the commit).

But how do we facilitate a change to use "stab" system (for all the
scripts using the old option)?

Can we change the iproute2/tc command to handle this transparently, or
should we give an error/warning if someone uses "tc" and "linklayer" on
a kernel above v.3.8. ?


History:
 - My linklayer ATM changes appeared in kernel 2.6.24 (and iproute2 2.6.25)
 - The STAB changes appeared in kernel 2.6.27

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Re: tc linklayer ADSL calc broken after commit 56b765b79 (htb: improved accuracy at high rates)

[Eric Dumazet]

On Wed, 2013-05-29 at 15:13 +0200, Jesper Dangaard Brouer wrote:
> I recently discovered that the (traffic control) tc linklayer
> calculations for ATM/ADSL have been broken by:
>  commit 56b765b79 (htb: improved accuracy at high rates).
> 
> Thus, people shaping on ADSL links, using e.g.:
>  tc class add ... htb rate X ceil Y linklayer atm overhead 10
> 
> Will no-longer get ATM cell tax/overhead adjusted.
> 
> How can we solve/fix this?
> Perhaps we can change to use the "stab" system instead (as it does
> not seem to be broken by the commit).
> 
> But how do we facilitate a change to use "stab" system (for all the
> scripts using the old option)?
> 
> Can we change the iproute2/tc command to handle this transparently, or
> should we give an error/warning if someone uses "tc" and "linklayer" on
> a kernel above v.3.8. ?
> 
> 
> History:
>  - My linklayer ATM changes appeared in kernel 2.6.24 (and iproute2 2.6.25)
>  - The STAB changes appeared in kernel 2.6.27
> 

Hi Jesper

stab suffers from the same problem : its table driven, so works only for
packet smaller than a given size.

I am not sure it will solve the ATM logic (with the 5 bytes overhead per
48 bytes cell)

btw, even on old kernels :

Re: tc linklayer ADSL calc broken after commit 56b765b79 (htb: improved accuracy at high rates)

[Stephen Hemminger]

On Wed, 29 May 2013 08:52:04 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Wed, 2013-05-29 at 15:13 +0200, Jesper Dangaard Brouer wrote:
> > I recently discovered that the (traffic control) tc linklayer
> > calculations for ATM/ADSL have been broken by:
> >  commit 56b765b79 (htb: improved accuracy at high rates).
> > 
> > Thus, people shaping on ADSL links, using e.g.:
> >  tc class add ... htb rate X ceil Y linklayer atm overhead 10
> > 
> > Will no-longer get ATM cell tax/overhead adjusted.
> > 
> > How can we solve/fix this?
> > Perhaps we can change to use the "stab" system instead (as it does
> > not seem to be broken by the commit).
> > 
> > But how do we facilitate a change to use "stab" system (for all the
> > scripts using the old option)?
> > 
> > Can we change the iproute2/tc command to handle this transparently, or
> > should we give an error/warning if someone uses "tc" and "linklayer" on
> > a kernel above v.3.8. ?
> > 
> > 
> > History:
> >  - My linklayer ATM changes appeared in kernel 2.6.24 (and iproute2 2.6.25)
> >  - The STAB changes appeared in kernel 2.6.27
> > 
> 
> Hi Jesper
> 
> stab suffers from the same problem : its table driven, so works only for
> packet smaller than a given size.
> 
> I am not sure it will solve the ATM logic (with the 5 bytes overhead per
> 48 bytes cell)
> 
> btw, even on old kernels :


How bad is the failure? If it is fixed, will it break existing installations?

Which probably means, is anyone but the original developers ever using it
and therefore likely to notice?

Re: tc linklayer ADSL calc broken after commit 56b765b79 (htb: improved accuracy at high rates)

[Eric Dumazet]

On Wed, 2013-05-29 at 15:50 -0700, Stephen Hemminger wrote:
> On Wed, 29 May 2013 08:52:04 -0700
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> > On Wed, 2013-05-29 at 15:13 +0200, Jesper Dangaard Brouer wrote:
> > > I recently discovered that the (traffic control) tc linklayer
> > > calculations for ATM/ADSL have been broken by:
> > >  commit 56b765b79 (htb: improved accuracy at high rates).
> > > 
> > > Thus, people shaping on ADSL links, using e.g.:
> > >  tc class add ... htb rate X ceil Y linklayer atm overhead 10
> > > 
> > > Will no-longer get ATM cell tax/overhead adjusted.
> > > 
> > > How can we solve/fix this?
> > > Perhaps we can change to use the "stab" system instead (as it does
> > > not seem to be broken by the commit).
> > > 
> > > But how do we facilitate a change to use "stab" system (for all the
> > > scripts using the old option)?
> > > 
> > > Can we change the iproute2/tc command to handle this transparently, or
> > > should we give an error/warning if someone uses "tc" and "linklayer" on
> > > a kernel above v.3.8. ?
> > > 
> > > 
> > > History:
> > >  - My linklayer ATM changes appeared in kernel 2.6.24 (and iproute2 2.6.25)
> > >  - The STAB changes appeared in kernel 2.6.27
> > > 
> > 
> > Hi Jesper
> > 
> > stab suffers from the same problem : its table driven, so works only for
> > packet smaller than a given size.
> > 
> > I am not sure it will solve the ATM logic (with the 5 bytes overhead per
> > 48 bytes cell)
> > 
> > btw, even on old kernels :
> 
> 
> How bad is the failure? If it is fixed, will it break existing installations?
> 
> Which probably means, is anyone but the original developers ever using it
> and therefore likely to notice?

Adding the logic on the kernel is doable, by adding some clean
attributes so that tc can setup the feature, and report the attributes
back.

cpus are fast today and can perform the atm cell/overhead faster than a
table lookup.

Re: tc linklayer ADSL calc broken after commit 56b765b79 (htb: improved accuracy at high rates)

[Dave Taht]

[-- Attachment #1.1: Type: text/plain, Size: 2049 bytes --]

On May 29, 2013 6:50 PM, "Stephen Hemminger" <stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org>
wrote:
>
> On Wed, 29 May 2013 08:52:04 -0700
> Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> > On Wed, 2013-05-29 at 15:13 +0200, Jesper Dangaard Brouer wrote:
> > > I recently discovered that the (traffic control) tc linklayer
> > > calculations for ATM/ADSL have been broken by:
> > >  commit 56b765b79 (htb: improved accuracy at high rates).
> > >
> > > Thus, people shaping on ADSL links, using e.g.:
> > >  tc class add ... htb rate X ceil Y linklayer atm overhead 10
> > >
> > > Will no-longer get ATM cell tax/overhead adjusted.
> > >
> > > How can we solve/fix this?
> > > Perhaps we can change to use the "stab" system instead (as it does
> > > not seem to be broken by the commit).
> > >
> > > But how do we facilitate a change to use "stab" system (for all the
> > > scripts using the old option)?
> > >
> > > Can we change the iproute2/tc command to handle this transparently, or
> > > should we give an error/warning if someone uses "tc" and "linklayer"
on
> > > a kernel above v.3.8. ?
> > >
> > >
> > > History:
> > >  - My linklayer ATM changes appeared in kernel 2.6.24 (and iproute2
2.6.25)
> > >  - The STAB changes appeared in kernel 2.6.27
> > >
> >
> > Hi Jesper
> >
> > stab suffers from the same problem : its table driven, so works only for
> > packet smaller than a given size.
> >
> > I am not sure it will solve the ATM logic (with the 5 bytes overhead per
> > 48 bytes cell)
> >
> > btw, even on old kernels :
>
>
> How bad is the failure? If it is fixed, will it break existing
installations?
>
> Which probably means, is anyone but the original developers ever using it
> and therefore likely to notice?

This debugging train stems on part from spending quite some time being very
puzzled about the behavior of DSL against multiple HTB based fq codel
shapers.

So I'm glad it is a real bug at this layer rather than elsewhere. I'll just
nip off and write off those datasets now.

[-- Attachment #1.2: Type: text/html, Size: 2878 bytes --]

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Bloat mailing list
Bloat-JXvr2/1DY2fm6VMwtOF2vx4hnT+Y9+D1@public.gmane.org
https://lists.bufferbloat.net/listinfo/bloat

Re: tc linklayer ADSL calc broken after commit 56b765b79 (htb: improved accuracy at high rates)

[Jesper Dangaard Brouer]

On Wed, 29 May 2013 08:52:04 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Wed, 2013-05-29 at 15:13 +0200, Jesper Dangaard Brouer wrote:
> > I recently discovered that the (traffic control) tc linklayer
> > calculations for ATM/ADSL have been broken by:
> >  commit 56b765b79 (htb: improved accuracy at high rates).
> > 
> > Thus, people shaping on ADSL links, using e.g.:
> >  tc class add ... htb rate X ceil Y linklayer atm overhead 10
> > 
> > Will no-longer get ATM cell tax/overhead adjusted.
> > 
> > How can we solve/fix this?
> > Perhaps we can change to use the "stab" system instead (as it does
> > not seem to be broken by the commit).
> > 
[...]
> 
> stab suffers from the same problem : its table driven, so works only
> for packet smaller than a given size.

You are referring to GSO/GRO packets. Yes, one must disable GSO for
this to work. Regardless ATM/ADSL, you should disable GSO when shaping
at low speeds.  Sending 64000 byte on a 512Kbit/s takes approx 1 sec.

http://netoptimizer.blogspot.dk/2010/12/buffer-bloat-calculations.html


> I am not sure it will solve the ATM logic (with the 5 bytes overhead
> per 48 bytes cell)

Are you talking about, that for GSO frames we are not adding a encap
overhead to each "sub" skb.


-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Re: tc linklayer ADSL calc broken after commit 56b765b79 (htb: improved accuracy at high rates)

[Jesper Dangaard Brouer]

On Wed, 29 May 2013 15:50:34 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> On Wed, 29 May 2013 08:52:04 -0700
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> > On Wed, 2013-05-29 at 15:13 +0200, Jesper Dangaard Brouer wrote:
> > > I recently discovered that the (traffic control) tc linklayer
> > > calculations for ATM/ADSL have been broken by:
> > >  commit 56b765b79 (htb: improved accuracy at high rates).
> > > 
> > > Thus, people shaping on ADSL links, using e.g.:
> > >  tc class add ... htb rate X ceil Y linklayer atm overhead 10
> > > 
> > > Will no-longer get ATM cell tax/overhead adjusted.
> > > 
> > > How can we solve/fix this?
> > > Perhaps we can change to use the "stab" system instead (as it does
> > > not seem to be broken by the commit).
> > > 
> > > But how do we facilitate a change to use "stab" system (for all
> > > the scripts using the old option)?
> > > 
> > > Can we change the iproute2/tc command to handle this
> > > transparently, or should we give an error/warning if someone uses
> > > "tc" and "linklayer" on a kernel above v.3.8. ?
> > > 
[...]
> 
> How bad is the failure? If it is fixed, will it break existing
> installations?

There is no "failure", the ATM-aligned rate table send to the kernel is
silently ignored.

People using the linklayer ATM option will just be confused why their
shaping scripts does not work, when situation of bufferbloat occurs.

I guess that was why Dave Taht, was so confused, when he wanted to make
his scripts DSL aware...


> Which probably means, is anyone but the original developers ever
> using it and therefore likely to notice?

Me the "original developer" actually don't use as I don't have a ADSL
line any-longer.  I know of people using this.  Dan Siemon have it as
an option in his scripts (but uses VDSL with out ATM himself). I
recently got contacted from someone in China, asking me to reconstruct
my homepage: http://www.adsl-optimizer.dk/ as they were using it.

The question is how do we fix this in a backward compatible manor?


-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Re: tc linklayer ADSL calc broken after commit 56b765b79 (htb: improved accuracy at high rates)

[Jesper Dangaard Brouer]

On Wed, 29 May 2013 16:18:26 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2013-05-29 at 15:50 -0700, Stephen Hemminger wrote:
> > On Wed, 29 May 2013 08:52:04 -0700
> > Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > 
> > > On Wed, 2013-05-29 at 15:13 +0200, Jesper Dangaard Brouer wrote:
> > > > I recently discovered that the (traffic control) tc linklayer
> > > > calculations for ATM/ADSL have been broken by:
> > > >  commit 56b765b79 (htb: improved accuracy at high rates).
> > > > 
> > > > Thus, people shaping on ADSL links, using e.g.:
> > > >  tc class add ... htb rate X ceil Y linklayer atm overhead 10
> > > > 
> > > > Will no-longer get ATM cell tax/overhead adjusted.
> > > > 
> 
> Adding the logic on the kernel is doable, by adding some clean
> attributes so that tc can setup the feature, and report the attributes
> back.

Yes, doing the logic in the kernel might be a better solution.
But the question is how do we keep iproute2 backward compatible with
older kernels?



> cpus are fast today and can perform the atm cell/overhead faster than
> a table lookup.

Do remember that the target CPU is small embedded router boxes.
BUT I do agree that, the following code required, is probably faster
than a table lookup:

 int pkt_len = skb->len + (encap_overhead * gso_segments);
 int wire_sz = DIV_ROUND_UP(pkt_len,48)*53;

(I suspect, that the compiler might even optimize and remove any
real divisions, I bet Eric can tell us.)

Looking at how simple the above code is, I'm a little appalled by all
the table lookup infrastructure and hacks we added, to
support/implement this.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [Bloat] tc linklayer ADSL calc broken after commit 56b765b79 (htb: improved accuracy at high rates)

[Steinar H. Gunderson]

On Thu, May 30, 2013 at 11:15:47AM +0200, Jesper Dangaard Brouer wrote:
>  int pkt_len = skb->len + (encap_overhead * gso_segments);
>  int wire_sz = DIV_ROUND_UP(pkt_len,48)*53;
> 
> (I suspect, that the compiler might even optimize and remove any
> real divisions, I bet Eric can tell us.)

FWIW, GCC can change divisions by integer constants (even signed divisions)
to some multiplies and shifts, by way of some number theory magic.

/* Steinar */
-- 
Homepage: http://www.sesse.net/

Re: tc linklayer ADSL calc broken after commit 56b765b79 (htb: improved accuracy at high rates)

[Eric Dumazet]

On Thu, 2013-05-30 at 09:51 +0200, Jesper Dangaard Brouer wrote:
> On Wed, 29 May 2013 08:52:04 -0700
> Eric Dumazet <eric.dumazet@gmail.com> wrote:

> > I am not sure it will solve the ATM logic (with the 5 bytes overhead
> > per 48 bytes cell)
> 
> Are you talking about, that for GSO frames we are not adding a encap
> overhead to each "sub" skb.

This part is now done properly in qdisc_pkt_len_init() since linux-3.9


No I was really mentioning the DIV_ROUND_UP(pkt_len,48)*53 thing

Re: tc linklayer ADSL calc broken after commit 56b765b79 (htb: improved accuracy at high rates)

[Jesper Dangaard Brouer]

On Thu, 30 May 2013 07:39:10 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Thu, 2013-05-30 at 09:51 +0200, Jesper Dangaard Brouer wrote:
> > On Wed, 29 May 2013 08:52:04 -0700
> > Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> > > I am not sure it will solve the ATM logic (with the 5 bytes
> > > overhead per 48 bytes cell)
> > 
> > Are you talking about, that for GSO frames we are not adding a encap
> > overhead to each "sub" skb.
> 
> This part is now done properly in qdisc_pkt_len_init() since linux-3.9

Thanks for the pointer, but qdisc_pkt_len_init() only adds the
EthMAC+IP+TCP header size for each GSO segment (stored in
qdisc_skb_cb(skb)->pkt_len).
It is still missing the AAL5 encapsulation overhead per GSO segment.

Besides I can see that __qdisc_calculate_pkt_len() "forgets" this
information and overwrites qdisc_skb_cb(skb)->pkt_len (iif a stab is
defined on the qdisc).

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Re: tc linklayer ADSL calc broken after commit 56b765b79 (htb: improved accuracy at high rates)

[Jussi Kivilinna]

On 30.05.2013 18:55, Jesper Dangaard Brouer wrote:
> On Thu, 30 May 2013 07:39:10 -0700
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
>> On Thu, 2013-05-30 at 09:51 +0200, Jesper Dangaard Brouer wrote:
>>> On Wed, 29 May 2013 08:52:04 -0700
>>> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>>>> I am not sure it will solve the ATM logic (with the 5 bytes
>>>> overhead per 48 bytes cell)
>>>
>>> Are you talking about, that for GSO frames we are not adding a encap
>>> overhead to each "sub" skb.
>>
>> This part is now done properly in qdisc_pkt_len_init() since linux-3.9
> 
> Thanks for the pointer, but qdisc_pkt_len_init() only adds the
> EthMAC+IP+TCP header size for each GSO segment (stored in
> qdisc_skb_cb(skb)->pkt_len).
> It is still missing the AAL5 encapsulation overhead per GSO segment.
> 
> Besides I can see that __qdisc_calculate_pkt_len() "forgets" this
> information and overwrites qdisc_skb_cb(skb)->pkt_len (iif a stab is
> defined on the qdisc).
> 

Maybe change qdisc_pkt_len_init() to take the additional overhead value as
input and change __qdisc_calculate_pkt_len() use

	qdisc_pkt_len_init(skb, stab->szopts.overhead);

instead of

	pkt_len = skb->len + stab->szopts.overhead;

-Jussi

Re: tc linklayer ADSL calc broken after commit 56b765b79 (htb: improved accuracy at high rates)

[Eric Dumazet]

On Wed, 2013-05-29 at 15:13 +0200, Jesper Dangaard Brouer wrote:
> I recently discovered that the (traffic control) tc linklayer
> calculations for ATM/ADSL have been broken by:
>  commit 56b765b79 (htb: improved accuracy at high rates).
> 
> Thus, people shaping on ADSL links, using e.g.:
>  tc class add ... htb rate X ceil Y linklayer atm overhead 10
> 

It seems the "overhead 10" was never reported back by 
"tc -s class show dev ... "

Also, the "linklayer atm" changes the data[] part, and this one is not
matched in qdisc_get_rtab()

So two different rate specifications, but sharing same struct
tc_ratespec could be shared... Oh well.

It seems following fix would be needed anyway ?

[PATCH] net_sched: qdisc_get_rtab() must check data[] array

qdisc_get_rtab() should check not only the keys in struct tc_ratespec,
but also the full data[] array.

"tc ... linklayer atm " only perturbs values in the 256 slots array.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/sched/sch_api.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 2b935e7..281c1bd 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -291,17 +291,18 @@ struct qdisc_rate_table *qdisc_get_rtab(struct tc_ratespec *r, struct nlattr *ta
 {
 	struct qdisc_rate_table *rtab;
 
+	if (tab == NULL || r->rate == 0 || r->cell_log == 0 ||
+	    nla_len(tab) != TC_RTAB_SIZE)
+		return NULL;
+
 	for (rtab = qdisc_rtab_list; rtab; rtab = rtab->next) {
-		if (memcmp(&rtab->rate, r, sizeof(struct tc_ratespec)) == 0) {
+		if (!memcmp(&rtab->rate, r, sizeof(struct tc_ratespec)) &&
+		    !memcmp(&rtab->data, nla_data(tab), 1024)) {
 			rtab->refcnt++;
 			return rtab;
 		}
 	}
 
-	if (tab == NULL || r->rate == 0 || r->cell_log == 0 ||
-	    nla_len(tab) != TC_RTAB_SIZE)
-		return NULL;
-
 	rtab = kmalloc(sizeof(*rtab), GFP_KERNEL);
 	if (rtab) {
 		rtab->rate = *r;

[PATCH iproute2] htb: report overhead attribute

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

"tc class show dev ..." omits the overhead attribute for HTB.

After patch I have :

tc class add dev $DEV parent 1: classid 1:1 est 1sec 4sec htb \
    rate 12Mbit mtu 1500 quantum 1514 overhead 20

tc class show dev $DEV
class htb 1:1 root prio 0 rate 12000Kbit overhead 20 ceil 12000Kbit
burst 1500b cburst 1500b

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 tc/q_htb.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/tc/q_htb.c b/tc/q_htb.c
index caa47c2..e6b09bb 100644
--- a/tc/q_htb.c
+++ b/tc/q_htb.c
@@ -264,6 +264,8 @@ static int htb_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 				fprintf(f, "quantum %d ", (int)hopt->quantum);
 		}
 	    fprintf(f, "rate %s ", sprint_rate(hopt->rate.rate, b1));
+	    if (hopt->rate.overhead)
+		fprintf(f, "overhead %u ", hopt->rate.overhead);
 	    buffer = tc_calc_xmitsize(hopt->rate.rate, hopt->buffer);
 	    fprintf(f, "ceil %s ", sprint_rate(hopt->ceil.rate, b1));
 	    cbuffer = tc_calc_xmitsize(hopt->ceil.rate, hopt->cbuffer);

Re: [PATCH iproute2] htb: report overhead attribute

[Rick Jones]

On 06/02/2013 02:33 PM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> "tc class show dev ..." omits the overhead attribute for HTB.
>
> After patch I have :
>
> tc class add dev $DEV parent 1: classid 1:1 est 1sec 4sec htb \
>      rate 12Mbit mtu 1500 quantum 1514 overhead 20
>
> tc class show dev $DEV
> class htb 1:1 root prio 0 rate 12000Kbit overhead 20 ceil 12000Kbit
> burst 1500b cburst 1500b
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>   tc/q_htb.c |    2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/tc/q_htb.c b/tc/q_htb.c
> index caa47c2..e6b09bb 100644
> --- a/tc/q_htb.c
> +++ b/tc/q_htb.c
> @@ -264,6 +264,8 @@ static int htb_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
>   				fprintf(f, "quantum %d ", (int)hopt->quantum);
>   		}
>   	    fprintf(f, "rate %s ", sprint_rate(hopt->rate.rate, b1));
> +	    if (hopt->rate.overhead)
> +		fprintf(f, "overhead %u ", hopt->rate.overhead);

Is it (still) possible to have a negative overhead?

http://www.linksysinfo.org/index.php?threads/speedmod-with-tc-atm-qos-patch-for-adsl.31541/

rick jones

Re: [PATCH iproute2] htb: report overhead attribute

[Eric Dumazet]

On Mon, 2013-06-03 at 08:45 -0700, Rick Jones wrote:

> Is it (still) possible to have a negative overhead?
> 
> http://www.linksysinfo.org/index.php?threads/speedmod-with-tc-atm-qos-patch-for-adsl.31541/

overhead always has been unsigned in the kernel.

What you describe is a userland hack in tc command.

(or a bug)

Re: [PATCH iproute2] htb: report overhead attribute

[Jussi Kivilinna]

On 03.06.2013 18:45, Rick Jones wrote:
> On 06/02/2013 02:33 PM, Eric Dumazet wrote:
>> From: Eric Dumazet <edumazet@google.com>
>>
>> "tc class show dev ..." omits the overhead attribute for HTB.
>>
>> After patch I have :
>>
>> tc class add dev $DEV parent 1: classid 1:1 est 1sec 4sec htb \
>>      rate 12Mbit mtu 1500 quantum 1514 overhead 20
>>
>> tc class show dev $DEV
>> class htb 1:1 root prio 0 rate 12000Kbit overhead 20 ceil 12000Kbit
>> burst 1500b cburst 1500b
>>
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> ---
>>   tc/q_htb.c |    2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/tc/q_htb.c b/tc/q_htb.c
>> index caa47c2..e6b09bb 100644
>> --- a/tc/q_htb.c
>> +++ b/tc/q_htb.c
>> @@ -264,6 +264,8 @@ static int htb_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
>>                   fprintf(f, "quantum %d ", (int)hopt->quantum);
>>           }
>>           fprintf(f, "rate %s ", sprint_rate(hopt->rate.rate, b1));
>> +        if (hopt->rate.overhead)
>> +        fprintf(f, "overhead %u ", hopt->rate.overhead);
> 
> Is it (still) possible to have a negative overhead?
> 
> http://www.linksysinfo.org/index.php?threads/speedmod-with-tc-atm-qos-patch-for-adsl.31541/
> 

You can use 'tc-stab' for negative overhead.

http://stuff.onse.fi/man?program=tc-stab&section=8

-Jussi

Re: [PATCH iproute2] htb: report overhead attribute

[Jesper Dangaard Brouer]

On Mon, 03 Jun 2013 08:56:02 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Mon, 2013-06-03 at 08:45 -0700, Rick Jones wrote:
> 
> > Is it (still) possible to have a negative overhead?
> > 
> > http://www.linksysinfo.org/index.php?threads/speedmod-with-tc-atm-qos-patch-for-adsl.31541/
> 
> overhead always has been unsigned in the kernel.
> 
> What you describe is a userland hack in tc command.
> (or a bug)

Rick is referencing Russell Stuart's patches, where a negative overhead
was possible.
 http://ace-host.stuart.id.au/russell/files/tc/tc-atm/#history

But my patches got accepted into the kernel, where a negative
overhead was not possible. In retrospect, we should have supported a
negative overhead.

A negative overhead *is* a valid use-case, and we should work towards
supporting this. E.g. by changing the recent added "u16 overhead" in
struct psched_ratecfg to be "s16" (ref [1]) ?


[1] commit 01cb71d2d47 (net_sched: restore "overhead xxx" handling)
 https://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=01cb71d2d47b78354358e4bb938bb06323e17498

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Bad shaping at low rates, after commit 56b765b79 (htb: improved accuracy at high rates)

[Jesper Dangaard Brouer]

Hi again,

I found another regression by commit 56b765b79 (htb: improved accuracy
at high rates).

After the commit HTB does not honor network rate limiting below 500kbps.

I have found that the bandwidth problem is related to GSO being enabled
on the device.  To test the situation, I had to use a real NIC (as I
were not allowed to disable GSO on dev "lo", test below).

Testing with 100kbit/s:
-----------------------
A localhost dev "lo" reproducer commands:
 tc qdisc add dev lo root handle 1: htb default 1
 tc class add dev lo parent 1: classid 1:1 htb rate 100kbit
 netserver
 netperf -t TCP_STREAM -l 60 -H 127.0.0.1 -- -m 1024

Measuring the qdisc "rate" via:
 tc -s -d class show dev lo classid 1:1

I know the qdisc "rate" is not not very reliable/accurate, but I do
see a large difference (and so do netperf reported throughput).
 - Kernel 2.6.32-352.el6 gives rate 98752bit
 - Kernel 3.10.0-rc1     gives rate 390208bit

My real-NIC test show:
 With GSO: rate 153248bit
 Without GSO: rate 99320bit

Now, we just have to figure out why commit 56b765b79e (htb: improved
accuracy at high rates) breaks bandwidth shaping at low rate with GSO
enabled...

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Data: real NIC
==============

With GSO: rate 153248bit
---------
class htb 1:1 root prio 0 quantum 1250 rate 100000bit ceil 100000bit
burst 1600b/1 mpu 0b overhead 0b cburst 1600b/1 mpu 0b overhead 0b
level 0 Sent 844892 bytes 568 pkt (dropped 0, overlimits 0 requeues 0)
rate 153248bit 13pps backlog 0b 7p requeues 0 lended: 87 borrowed: 0
giants: 0 tokens: -847776554 ctokens: -847776554

Without GSO: rate 99320bit
------------
(disabled via: ethtool -K eth63 gso off gro off tso off)

class htb 1:1 root prio 0 quantum 1250 rate 100000bit ceil 100000bit
burst 1600b/1 mpu 0b overhead 0b cburst 1600b/1 mpu 0b overhead 0b
level 0 Sent 1706968 bytes 1164 pkt (dropped 0, overlimits 0 requeues
0) rate 99320bit 8pps backlog 0b 51p requeues 0 lended: 664 borrowed: 0
giants: 0 tokens: -121054491 ctokens: -121054491

Re: [PATCH iproute2] htb: report overhead attribute

[Eric Dumazet]

On Tue, 2013-06-04 at 13:11 +0200, Jesper Dangaard Brouer wrote:
> On Mon, 03 Jun 2013 08:56:02 -0700
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> > On Mon, 2013-06-03 at 08:45 -0700, Rick Jones wrote:
> > 
> > > Is it (still) possible to have a negative overhead?
> > > 
> > > http://www.linksysinfo.org/index.php?threads/speedmod-with-tc-atm-qos-patch-for-adsl.31541/
> > 
> > overhead always has been unsigned in the kernel.
> > 
> > What you describe is a userland hack in tc command.
> > (or a bug)
> 
> Rick is referencing Russell Stuart's patches, where a negative overhead
> was possible.
>  http://ace-host.stuart.id.au/russell/files/tc/tc-atm/#history
> 
> But my patches got accepted into the kernel, where a negative
> overhead was not possible. In retrospect, we should have supported a
> negative overhead.
> 
> A negative overhead *is* a valid use-case, and we should work towards
> supporting this. E.g. by changing the recent added "u16 overhead" in
> struct psched_ratecfg to be "s16" (ref [1]) ?
> 
> 
> [1] commit 01cb71d2d47 (net_sched: restore "overhead xxx" handling)
>  https://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=01cb71d2d47b78354358e4bb938bb06323e17498
> 

Again, you describe something that Vimal patch didn't broke, and should
be addressed on net-next.

My concern was restoring the overhead attribute that Vimal broke, and
this one was unsigned 16bits.

Allowing a negative offset is not free, it adds a conditional test,
because (len + overhead) could be negative.

Please note that a negative offset is possible with the STAB infra.

Re: [PATCH iproute2] htb: report overhead attribute

[Jesper Dangaard Brouer]

On Tue, 04 Jun 2013 06:58:31 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Tue, 2013-06-04 at 13:11 +0200, Jesper Dangaard Brouer wrote:
> > On Mon, 03 Jun 2013 08:56:02 -0700
> > Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > 
> > > On Mon, 2013-06-03 at 08:45 -0700, Rick Jones wrote:
> > > 
> > > > Is it (still) possible to have a negative overhead?
> > > > 
> > > > http://www.linksysinfo.org/index.php?threads/speedmod-with-tc-atm-qos-patch-for-adsl.31541/
> > > 
> > > overhead always has been unsigned in the kernel.
> > > 
> > > What you describe is a userland hack in tc command.
> > > (or a bug)
> > 
> > Rick is referencing Russell Stuart's patches, where a negative
> > overhead was possible.
> >  http://ace-host.stuart.id.au/russell/files/tc/tc-atm/#history
> > 
> > But my patches got accepted into the kernel, where a negative
> > overhead was not possible. In retrospect, we should have supported a
> > negative overhead.
> > 
> > A negative overhead *is* a valid use-case, and we should work
> > towards supporting this. E.g. by changing the recent added "u16
> > overhead" in struct psched_ratecfg to be "s16" (ref [1]) ?
(My statement should have been corrected to "s16"->"s32", never mind)

> > 
> > [1] commit 01cb71d2d47 (net_sched: restore "overhead xxx" handling)
> >  https://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=01cb71d2d47b78354358e4bb938bb06323e17498
> > 
> 
> Again, you describe something that Vimal patch didn't broke, and
> should be addressed on net-next.

Yes, I know, I also said "work towards supporting this", meaning
"net-next".


> My concern was restoring the overhead attribute that Vimal broke, and
> this one was unsigned 16bits.
> 
> Allowing a negative offset is not free, it adds a conditional test,
> because (len + overhead) could be negative.

Yes, I do realize that. But Vimal patch actually also broke the
"mpu" (Minimum Packet Unit) feature.  And we could combine this, and
get negative offset fix by a max(mpu,len), where mpu would be default
init'ed. 

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Re: Bad shaping at low rates, after commit 56b765b79 (htb: improved accuracy at high rates)

[Eric Dumazet]

On Tue, 2013-06-04 at 14:13 +0200, Jesper Dangaard Brouer wrote:
> Hi again,
> 
> I found another regression by commit 56b765b79 (htb: improved accuracy
> at high rates).
> 
> After the commit HTB does not honor network rate limiting below 500kbps.
> 
> I have found that the bandwidth problem is related to GSO being enabled
> on the device.  To test the situation, I had to use a real NIC (as I
> were not allowed to disable GSO on dev "lo", test below).

Well, do not forget lo mtu is 64K, so you'll also have to lower it for
your tests.

I have a good idea of what's going on for htb at low rates, I am testing
a fix, thanks for the report !

Re: Bad shaping at low rates, after commit 56b765b79 (htb: improved accuracy at high rates)

[Eric Dumazet]

On Tue, 2013-06-04 at 08:18 -0700, Eric Dumazet wrote:

> I have a good idea of what's going on for htb at low rates, I am testing
> a fix, thanks for the report !
> 

Yes, we need to convert whole thing to use ns units, instead
of a mix of 64ns and 1ns units.

Please test the following patch :

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 79b1876..6c53341 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -109,7 +109,7 @@ struct htb_class {
 	} un;
 	struct rb_node node[TC_HTB_NUMPRIO];	/* node for self or feed tree */
 	struct rb_node pq_node;	/* node for event queue */
-	psched_time_t pq_key;
+	s64 pq_key;
 
 	int prio_activity;	/* for which prios are we active */
 	enum htb_cmode cmode;	/* current mode of the class */
@@ -124,7 +124,7 @@ struct htb_class {
 	s64 buffer, cbuffer;	/* token bucket depth/rate */
 	psched_tdiff_t mbuffer;	/* max wait time */
 	s64 tokens, ctokens;	/* current number of tokens */
-	psched_time_t t_c;	/* checkpoint time */
+	s64 t_c;		/* checkpoint time */
 };
 
 struct htb_sched {
@@ -141,7 +141,7 @@ struct htb_sched {
 	struct rb_root wait_pq[TC_HTB_MAXDEPTH];
 
 	/* time of nearest event per level (row) */
-	psched_time_t near_ev_cache[TC_HTB_MAXDEPTH];
+	s64 near_ev_cache[TC_HTB_MAXDEPTH];
 
 	int defcls;		/* class where unclassified flows go to */
 
@@ -149,7 +149,7 @@ struct htb_sched {
 	struct tcf_proto *filter_list;
 
 	int rate2quantum;	/* quant = rate / rate2quantum */
-	psched_time_t now;	/* cached dequeue time */
+	s64 now;	/* cached dequeue time */
 	struct qdisc_watchdog watchdog;
 
 	/* non shaped skbs; let them go directly thru */
@@ -664,8 +664,8 @@ static void htb_charge_class(struct htb_sched *q, struct htb_class *cl,
  * next pending event (0 for no event in pq, q->now for too many events).
  * Note: Applied are events whose have cl->pq_key <= q->now.
  */
-static psched_time_t htb_do_events(struct htb_sched *q, int level,
-				   unsigned long start)
+static s64 htb_do_events(struct htb_sched *q, int level,
+			 unsigned long start)
 {
 	/* don't run for longer than 2 jiffies; 2 is used instead of
 	 * 1 to simplify things when jiffy is going to be incremented
@@ -857,7 +857,7 @@ static struct sk_buff *htb_dequeue(struct Qdisc *sch)
 	struct sk_buff *skb;
 	struct htb_sched *q = qdisc_priv(sch);
 	int level;
-	psched_time_t next_event;
+	s64 next_event;
 	unsigned long start_at;
 
 	/* try to dequeue direct packets as high prio (!) to minimize cpu work */
@@ -880,7 +880,7 @@ ok:
 	for (level = 0; level < TC_HTB_MAXDEPTH; level++) {
 		/* common case optimization - skip event handler quickly */
 		int m;
-		psched_time_t event;
+		s64 event;
 
 		if (q->now >= q->near_ev_cache[level]) {
 			event = htb_do_events(q, level, start_at);
@@ -1200,7 +1200,7 @@ static void htb_parent_to_leaf(struct htb_sched *q, struct htb_class *cl,
 	parent->un.leaf.q = new_q ? new_q : &noop_qdisc;
 	parent->tokens = parent->buffer;
 	parent->ctokens = parent->cbuffer;
-	parent->t_c = psched_get_time();
+	parent->t_c = ktime_to_ns(ktime_get());
 	parent->cmode = HTB_CAN_SEND;
 }
 
@@ -1417,8 +1417,8 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
 		/* set class to be in HTB_CAN_SEND state */
 		cl->tokens = PSCHED_TICKS2NS(hopt->buffer);
 		cl->ctokens = PSCHED_TICKS2NS(hopt->cbuffer);
-		cl->mbuffer = 60 * PSCHED_TICKS_PER_SEC;	/* 1min */
-		cl->t_c = psched_get_time();
+		cl->mbuffer = 60ULL * NSEC_PER_SEC;	/* 1min */
+		cl->t_c = ktime_to_ns(ktime_get());
 		cl->cmode = HTB_CAN_SEND;
 
 		/* attach to the hash list and parent's family */

Re: Bad shaping at low rates, after commit 56b765b79 (htb: improved accuracy at high rates)

[Eric Dumazet]

On Tue, 2013-06-04 at 08:55 -0700, Eric Dumazet wrote:
> On Tue, 2013-06-04 at 08:18 -0700, Eric Dumazet wrote:
> 
> > I have a good idea of what's going on for htb at low rates, I am testing
> > a fix, thanks for the report !
> > 
> 
> Yes, we need to convert whole thing to use ns units, instead
> of a mix of 64ns and 1ns units.
> 
> Please test the following patch :

Oh well, it works only on 64bit arches.

I'll send a more complete patch, but thats the idea

[PATCH] net_sched: htb: do not mix 1ns and 64ns time units

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

commit 56b765b79 ("htb: improved accuracy at high rates") added another
regression for low rates, because it mixes 1ns and 64ns time units.

So the maximum delay (mbuffer) was not 60 second, but 937 ms.

Lets convert all time fields to 1ns as 64bit arches are becoming the
norm.

Reported-by: Jesper Dangaard Brouer <brouer@redhat.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/sched/sch_htb.c |   34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 79b1876..e58b738 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -109,7 +109,7 @@ struct htb_class {
 	} un;
 	struct rb_node node[TC_HTB_NUMPRIO];	/* node for self or feed tree */
 	struct rb_node pq_node;	/* node for event queue */
-	psched_time_t pq_key;
+	s64	pq_key;
 
 	int prio_activity;	/* for which prios are we active */
 	enum htb_cmode cmode;	/* current mode of the class */
@@ -121,10 +121,10 @@ struct htb_class {
 	/* token bucket parameters */
 	struct psched_ratecfg rate;
 	struct psched_ratecfg ceil;
-	s64 buffer, cbuffer;	/* token bucket depth/rate */
-	psched_tdiff_t mbuffer;	/* max wait time */
-	s64 tokens, ctokens;	/* current number of tokens */
-	psched_time_t t_c;	/* checkpoint time */
+	s64	buffer, cbuffer;	/* token bucket depth/rate */
+	s64	mbuffer;		/* max wait time */
+	s64	tokens, ctokens;	/* current number of tokens */
+	s64	t_c;			/* checkpoint time */
 };
 
 struct htb_sched {
@@ -141,15 +141,15 @@ struct htb_sched {
 	struct rb_root wait_pq[TC_HTB_MAXDEPTH];
 
 	/* time of nearest event per level (row) */
-	psched_time_t near_ev_cache[TC_HTB_MAXDEPTH];
+	s64	near_ev_cache[TC_HTB_MAXDEPTH];
 
 	int defcls;		/* class where unclassified flows go to */
 
 	/* filters for qdisc itself */
 	struct tcf_proto *filter_list;
 
-	int rate2quantum;	/* quant = rate / rate2quantum */
-	psched_time_t now;	/* cached dequeue time */
+	int	rate2quantum;	/* quant = rate / rate2quantum */
+	s64	now;	/* cached dequeue time */
 	struct qdisc_watchdog watchdog;
 
 	/* non shaped skbs; let them go directly thru */
@@ -664,8 +664,8 @@ static void htb_charge_class(struct htb_sched *q, struct htb_class *cl,
  * next pending event (0 for no event in pq, q->now for too many events).
  * Note: Applied are events whose have cl->pq_key <= q->now.
  */
-static psched_time_t htb_do_events(struct htb_sched *q, int level,
-				   unsigned long start)
+static s64 htb_do_events(struct htb_sched *q, int level,
+			 unsigned long start)
 {
 	/* don't run for longer than 2 jiffies; 2 is used instead of
 	 * 1 to simplify things when jiffy is going to be incremented
@@ -857,7 +857,7 @@ static struct sk_buff *htb_dequeue(struct Qdisc *sch)
 	struct sk_buff *skb;
 	struct htb_sched *q = qdisc_priv(sch);
 	int level;
-	psched_time_t next_event;
+	s64 next_event;
 	unsigned long start_at;
 
 	/* try to dequeue direct packets as high prio (!) to minimize cpu work */
@@ -880,7 +880,7 @@ ok:
 	for (level = 0; level < TC_HTB_MAXDEPTH; level++) {
 		/* common case optimization - skip event handler quickly */
 		int m;
-		psched_time_t event;
+		s64 event;
 
 		if (q->now >= q->near_ev_cache[level]) {
 			event = htb_do_events(q, level, start_at);
@@ -1117,8 +1117,8 @@ htb_dump_class_stats(struct Qdisc *sch, unsigned long arg, struct gnet_dump *d)
 
 	if (!cl->level && cl->un.leaf.q)
 		cl->qstats.qlen = cl->un.leaf.q->q.qlen;
-	cl->xstats.tokens = cl->tokens;
-	cl->xstats.ctokens = cl->ctokens;
+	cl->xstats.tokens = PSCHED_NS2TICKS(cl->tokens);
+	cl->xstats.ctokens = PSCHED_NS2TICKS(cl->ctokens);
 
 	if (gnet_stats_copy_basic(d, &cl->bstats) < 0 ||
 	    gnet_stats_copy_rate_est(d, NULL, &cl->rate_est) < 0 ||
@@ -1200,7 +1200,7 @@ static void htb_parent_to_leaf(struct htb_sched *q, struct htb_class *cl,
 	parent->un.leaf.q = new_q ? new_q : &noop_qdisc;
 	parent->tokens = parent->buffer;
 	parent->ctokens = parent->cbuffer;
-	parent->t_c = psched_get_time();
+	parent->t_c = ktime_to_ns(ktime_get());
 	parent->cmode = HTB_CAN_SEND;
 }
 
@@ -1417,8 +1417,8 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
 		/* set class to be in HTB_CAN_SEND state */
 		cl->tokens = PSCHED_TICKS2NS(hopt->buffer);
 		cl->ctokens = PSCHED_TICKS2NS(hopt->cbuffer);
-		cl->mbuffer = 60 * PSCHED_TICKS_PER_SEC;	/* 1min */
-		cl->t_c = psched_get_time();
+		cl->mbuffer = 60ULL * NSEC_PER_SEC;	/* 1min */
+		cl->t_c = ktime_to_ns(ktime_get());
 		cl->cmode = HTB_CAN_SEND;
 
 		/* attach to the hash list and parent's family */

Re: [PATCH] net_sched: htb: do not mix 1ns and 64ns time units

[Jesper Dangaard Brouer]

On Tue, 04 Jun 2013 10:11:48 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> From: Eric Dumazet <edumazet@google.com>
> 
> commit 56b765b79 ("htb: improved accuracy at high rates") added
> another regression for low rates, because it mixes 1ns and 64ns time
> units.
> 
> So the maximum delay (mbuffer) was not 60 second, but 937 ms.
> 
> Lets convert all time fields to 1ns as 64bit arches are becoming the
> norm.

I'm of-cause happy as I need this fixed for big-HW machines at Red Hat.

But how is this 64-bit usage going to affect performance for smaller
ARM/MIPS based home routers, where shaping at these low rates is more
relevant?  (I'm just asking because I don't know, and just test this
on a 24-CPU machine).

Tested-by: Jesper Dangaard Brouer <brouer@redhat.com>

I have tested you patch, and it works for me.
Tested shaping at 100Kbit/s:
 - max "rate 100656bit" on dev lo
 - max "rate 102016bit" on a real  1 Gbit/s NIC (dev eth63).
 - max "rate 103256bit" on a real 10 Gbit/s NIC (dev eth31).

The traffic "of-cause" spikes due to the GSO frames, if measuring the
traffic at a higher resolution (I've seen upto 3 sec without traffic).

Thanks for the quick fix! :-)))
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Reproducer commands:
 tc qdisc add dev lo root handle 1: htb default 1
 tc class add dev lo parent 1: classid 1:1 htb rate 100kbit
 netserver
 netperf -t TCP_STREAM -l 60 -H 127.0.0.1 -- -m 1024

Measuring the qdisc "rate" via:
 tc -s -d class show dev lo classid 1:1

Data:
=====
>From dev "lo":
--------------
class htb 1:1 root prio 0 quantum 1250 rate 100000bit ceil 100000bit burst 1600b/1 mpu 0b overhead 0b cburst 1600b/1 mpu 0b overhead 0b level 0 
 Sent 597719 bytes 64 pkt (dropped 942, overlimits 0 requeues 0) 
 rate 100656bit 1pps backlog 0b 1p requeues 0 
 lended: 64 borrowed: 0 giants: 0
 tokens: -27387576 ctokens: -27387576

>From 1Gbit/s dev eth63:
-----------------------
class htb 1:1 root prio 0 quantum 1250 rate 100000bit ceil 100000bit burst 1600b/1 mpu 0b overhead 0b cburst 1600b/1 mpu 0b overhead 0b level 0 
 Sent 1801152 bytes 3224 pkt (dropped 0, overlimits 0 requeues 0) 
 rate 102016bit 9pps backlog 0b 12p requeues 0 
 lended: 2284 borrowed: 0 giants: 0
 tokens: -18923820 ctokens: -18923820


>From 10Gbit/s dev eth31:
------------------------
class htb 1:1 root prio 0 quantum 1250 rate 100000bit ceil 100000bit burst 1600b/1 mpu 0b overhead 0b cburst 1600b/1 mpu 0b overhead 0b level 0 
 Sent 855814 bytes 581 pkt (dropped 0, overlimits 0 requeues 0) 
 rate 103256bit 9pps backlog 0b 9p requeues 0 
 lended: 81 borrowed: 0 giants: 0
 tokens: -45419150 ctokens: -45419150

Re: [PATCH] net_sched: htb: do not mix 1ns and 64ns time units

[Dave Taht]

[-- Attachment #1.1: Type: text/plain, Size: 3426 bytes --]

On Tue, Jun 4, 2013 at 1:21 PM, Jesper Dangaard Brouer
<jbrouer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>wrote:

> On Tue, 04 Jun 2013 10:11:48 -0700
> Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> > From: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> >
> > commit 56b765b79 ("htb: improved accuracy at high rates") added
> > another regression for low rates, because it mixes 1ns and 64ns time
> > units.
> >
> > So the maximum delay (mbuffer) was not 60 second, but 937 ms.
> >
> > Lets convert all time fields to 1ns as 64bit arches are becoming the
> > norm.
>
> I'm of-cause happy as I need this fixed for big-HW machines at Red Hat.
>
> But how is this 64-bit usage going to affect performance for smaller
> ARM/MIPS based home routers, where shaping at these low rates is more
> relevant?  (I'm just asking because I don't know, and just test this
> on a 24-CPU machine).
>

I'm not worried about it but will find out shortly in cerowrt.

I take it this does not fix the DSL/ATM issue however?


>
> Tested-by: Jesper Dangaard Brouer <brouer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> I have tested you patch, and it works for me.
> Tested shaping at 100Kbit/s:
>  - max "rate 100656bit" on dev lo
>  - max "rate 102016bit" on a real  1 Gbit/s NIC (dev eth63).
>  - max "rate 103256bit" on a real 10 Gbit/s NIC (dev eth31).
>
> The traffic "of-cause" spikes due to the GSO frames, if measuring the
> traffic at a higher resolution (I've seen upto 3 sec without traffic).
>
> Thanks for the quick fix! :-)))
> --
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Sr. Network Kernel Developer at Red Hat
>   Author of http://www.iptv-analyzer.org
>   LinkedIn: http://www.linkedin.com/in/brouer
>
> Reproducer commands:
>  tc qdisc add dev lo root handle 1: htb default 1
>  tc class add dev lo parent 1: classid 1:1 htb rate 100kbit
>  netserver
>  netperf -t TCP_STREAM -l 60 -H 127.0.0.1 -- -m 1024
>
> Measuring the qdisc "rate" via:
>  tc -s -d class show dev lo classid 1:1
>
> Data:
> =====
> From dev "lo":
> --------------
> class htb 1:1 root prio 0 quantum 1250 rate 100000bit ceil 100000bit burst
> 1600b/1 mpu 0b overhead 0b cburst 1600b/1 mpu 0b overhead 0b level 0
>  Sent 597719 bytes 64 pkt (dropped 942, overlimits 0 requeues 0)
>  rate 100656bit 1pps backlog 0b 1p requeues 0
>  lended: 64 borrowed: 0 giants: 0
>  tokens: -27387576 ctokens: -27387576
>
> From 1Gbit/s dev eth63:
> -----------------------
> class htb 1:1 root prio 0 quantum 1250 rate 100000bit ceil 100000bit burst
> 1600b/1 mpu 0b overhead 0b cburst 1600b/1 mpu 0b overhead 0b level 0
>  Sent 1801152 bytes 3224 pkt (dropped 0, overlimits 0 requeues 0)
>  rate 102016bit 9pps backlog 0b 12p requeues 0
>  lended: 2284 borrowed: 0 giants: 0
>  tokens: -18923820 ctokens: -18923820
>
>
> From 10Gbit/s dev eth31:
> ------------------------
> class htb 1:1 root prio 0 quantum 1250 rate 100000bit ceil 100000bit burst
> 1600b/1 mpu 0b overhead 0b cburst 1600b/1 mpu 0b overhead 0b level 0
>  Sent 855814 bytes 581 pkt (dropped 0, overlimits 0 requeues 0)
>  rate 103256bit 9pps backlog 0b 9p requeues 0
>  lended: 81 borrowed: 0 giants: 0
>  tokens: -45419150 ctokens: -45419150
>
>


-- 
Dave Täht

Fixing bufferbloat with cerowrt:
http://www.teklibre.com/cerowrt/subscribe.html

[-- Attachment #1.2: Type: text/html, Size: 4526 bytes --]

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Bloat mailing list
Bloat-JXvr2/1DY2fm6VMwtOF2vx4hnT+Y9+D1@public.gmane.org
https://lists.bufferbloat.net/listinfo/bloat

Re: [PATCH] net_sched: htb: do not mix 1ns and 64ns time units

[Eric Dumazet]

On Tue, 2013-06-04 at 22:21 +0200, Jesper Dangaard Brouer wrote:

> But how is this 64-bit usage going to affect performance for smaller
> ARM/MIPS based home routers, where shaping at these low rates is more
> relevant?  (I'm just asking because I don't know, and just test this
> on a 24-CPU machine).

Well, by definition shaping at low rates is mostly using the timer
infra, and its already 64bit. You'll hardly spend time in HTB.

Note that psched_time_t is already 64bit wide.

Only psched_tdiff_t is 'unsigned long', and the change I made in this
patch will not change performance even on 32bit hosts.

Re: [PATCH] net_sched: htb: do not mix 1ns and 64ns time units

[Eric Dumazet]

On Tue, 2013-06-04 at 13:26 -0700, Dave Taht wrote:
> 

> 
> I'm not worried about it but will find out shortly in cerowrt.
> 
> I take it this does not fix the DSL/ATM issue however?
>  
> 
Really, the DSL/ATM stuff should use the STAB thing. 

I suppose you already disabled GSO/TSO anyway.

__qdisc_calculate_pkt_len() contains all we need, for this kind of
situation.

Re: [PATCH] net_sched: htb: do not mix 1ns and 64ns time units

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 04 Jun 2013 10:11:48 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> commit 56b765b79 ("htb: improved accuracy at high rates") added another
> regression for low rates, because it mixes 1ns and 64ns time units.
> 
> So the maximum delay (mbuffer) was not 60 second, but 937 ms.
> 
> Lets convert all time fields to 1ns as 64bit arches are becoming the
> norm.
> 
> Reported-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied and queued up for -stable, thanks.

RFC: Proposed fix for tc linklayer calc broken after commit 56b765b79 (htb: improved accuracy at high rates)

[Jesper Dangaard Brouer]

Requesting comments.

So, bacically commit 56b765b79 (htb: improved accuracy at high rates),
broke the "linklayer atm" handling.  As it didn't update the iproute tc util.

Treating this as a regression fix, this is the smallest and least
intrusive solution I could come up with.

I'm basically restoring the "linklayer atm" handling, by using the
__reserved field in struct tc_ratespec, to convey the chosen
linklayer option.


KERNEL patch:
=============

[PATCH RFC] net_sched: restore "linklayer atm" handling

From: Jesper Dangaard Brouer <brouer@redhat.com>

commit 56b765b79 ("htb: improved accuracy at high rates")
broke the "linklayer atm" handling.

 tc class add ... htb rate X ceil Y linklayer atm

This patch restores the "linklayer atm" handling, by using the
__reserved field in struct tc_ratespec, to convey the choosen
linklayer option.

This requires a corrosponding iproute2 tc fix, that updates this
field.  Older tc binaries can be detected by the kernel, as the
field would be zero.

Request-For-Comments-by: Jesper Dangaard Brouer <brouer@redhat.com>
---

 include/net/sch_generic.h      |    8 +++++++-
 include/uapi/linux/pkt_sched.h |   11 ++++++++++-
 net/sched/sch_generic.c        |    4 ++++
 3 files changed, 21 insertions(+), 2 deletions(-)


diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index e7f4e21..c9916b1 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -682,13 +682,18 @@ struct psched_ratecfg {
 	u64	rate_bps;
 	u32	mult;
 	u16	overhead;
+	u8	linklayer;
 	u8	shift;
 };
 
 static inline u64 psched_l2t_ns(const struct psched_ratecfg *r,
 				unsigned int len)
 {
-	return ((u64)(len + r->overhead) * r->mult) >> r->shift;
+	u64 pkt_len = len + r->overhead;
+
+	if (r->linklayer == TC_LINKLAYER_ATM)
+		pkt_len = DIV_ROUND_UP(pkt_len,48)*53;
+	return (pkt_len * r->mult) >> r->shift;
 }
 
 extern void psched_ratecfg_precompute(struct psched_ratecfg *r, const struct tc_ratespec *conf);
@@ -699,6 +704,7 @@ static inline void psched_ratecfg_getrate(struct tc_ratespec *res,
 	memset(res, 0, sizeof(*res));
 	res->rate = r->rate_bps >> 3;
 	res->overhead = r->overhead;
+	res->linklayer = r->linklayer;
 }
 
 #endif
diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index dbd71b0..71e1463 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -73,9 +73,18 @@ struct tc_estimator {
 #define TC_H_ROOT	(0xFFFFFFFFU)
 #define TC_H_INGRESS    (0xFFFFFFF1U)
 
+/* NOTES: Move to another .h file???
+ * NOTES: Need to corrospond to iproute2 tc/tc_core.h "enum link_layer".
+ */
+enum tc_link_layer {
+	TC_LINKLAYER_UNAWARE, /* Indicate unaware old iproute2 util */
+	TC_LINKLAYER_ETHERNET,
+	TC_LINKLAYER_ATM,
+};
+
 struct tc_ratespec {
 	unsigned char	cell_log;
-	unsigned char	__reserved;
+	__u8		linklayer;
 	unsigned short	overhead;
 	short		cell_align;
 	unsigned short	mpu;
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 2022408..8ff9a5e 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -908,6 +908,10 @@ void psched_ratecfg_precompute(struct psched_ratecfg *r,
 	memset(r, 0, sizeof(*r));
 	r->overhead = conf->overhead;
 	r->rate_bps = (u64)conf->rate << 3;
+	r->linklayer = conf->linklayer;
+	/* We could add a warn once here:
+	 *  if (r->linklayer == TC_LINKLAYER_UNAWARE)
+	 */
 	r->mult = 1;
 	/*
 	 * Calibrate mult, shift so that token counting is accurate



IPROUTE2 patch
==============
tc: convey linklayer information to the kernel

From: Jesper Dangaard Brouer <brouer@redhat.com>


---

 include/linux/pkt_sched.h |    8 +++++++-
 tc/q_htb.c                |    2 ++
 tc/tc_core.c              |    1 +
 3 files changed, 10 insertions(+), 1 deletions(-)


diff --git a/include/linux/pkt_sched.h b/include/linux/pkt_sched.h
index dbd71b0..8abdae6 100644
--- a/include/linux/pkt_sched.h
+++ b/include/linux/pkt_sched.h
@@ -73,9 +73,15 @@ struct tc_estimator {
 #define TC_H_ROOT	(0xFFFFFFFFU)
 #define TC_H_INGRESS    (0xFFFFFFF1U)
 
+enum tc_link_layer {
+	TC_LINKLAYER_UNAWARE, /* Indicate unaware old iproute2 util */
+	TC_LINKLAYER_ETHERNET,
+	TC_LINKLAYER_ATM,
+};
+
 struct tc_ratespec {
 	unsigned char	cell_log;
-	unsigned char	__reserved;
+	__u8		linklayer;
 	unsigned short	overhead;
 	short		cell_align;
 	unsigned short	mpu;
diff --git a/tc/q_htb.c b/tc/q_htb.c
index caa47c2..9b17412 100644
--- a/tc/q_htb.c
+++ b/tc/q_htb.c
@@ -267,6 +267,8 @@ static int htb_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 	    buffer = tc_calc_xmitsize(hopt->rate.rate, hopt->buffer);
 	    fprintf(f, "ceil %s ", sprint_rate(hopt->ceil.rate, b1));
 	    cbuffer = tc_calc_xmitsize(hopt->ceil.rate, hopt->cbuffer);
+	    // TODO: ADD print statements for linklayer
+
 	    if (show_details) {
 		fprintf(f, "burst %s/%u mpu %s overhead %s ",
 			sprint_size(buffer, b1),
diff --git a/tc/tc_core.c b/tc/tc_core.c
index 85b072e..9383fdc 100644
--- a/tc/tc_core.c
+++ b/tc/tc_core.c
@@ -129,6 +129,7 @@ int tc_calc_rtable(struct tc_ratespec *r, __u32 *rtab,
 		rtab[i] = tc_calc_xmittime(bps, sz);
 	}
 
+	r->linklayer = linklayer;
 	r->cell_align=-1; // Due to the sz calc
 	r->cell_log=cell_log;
 	return cell_log;

Re: RFC: Proposed fix for tc linklayer calc broken after commit 56b765b79 (htb: improved accuracy at high rates)

[Eric Dumazet]

On Thu, 2013-06-06 at 15:55 +0200, Jesper Dangaard Brouer wrote:
> Requesting comments.
> 
> So, bacically commit 56b765b79 (htb: improved accuracy at high rates),
> broke the "linklayer atm" handling.  As it didn't update the iproute tc util.
> 
> Treating this as a regression fix, this is the smallest and least
> intrusive solution I could come up with.
> 
> I'm basically restoring the "linklayer atm" handling, by using the
> __reserved field in struct tc_ratespec, to convey the chosen
> linklayer option.
> 
> 
> KERNEL patch:
> =============
> 
> [PATCH RFC] net_sched: restore "linklayer atm" handling
> 
> From: Jesper Dangaard Brouer <brouer@redhat.com>
> 
> commit 56b765b79 ("htb: improved accuracy at high rates")
> broke the "linklayer atm" handling.
> 
>  tc class add ... htb rate X ceil Y linklayer atm
> 
> This patch restores the "linklayer atm" handling, by using the
> __reserved field in struct tc_ratespec, to convey the choosen
> linklayer option.
> 
> This requires a corrosponding iproute2 tc fix, that updates this
> field.  Older tc binaries can be detected by the kernel, as the
> field would be zero.
> 
> Request-For-Comments-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
> 
>  include/net/sch_generic.h      |    8 +++++++-
>  include/uapi/linux/pkt_sched.h |   11 ++++++++++-
>  net/sched/sch_generic.c        |    4 ++++
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index e7f4e21..c9916b1 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -682,13 +682,18 @@ struct psched_ratecfg {
>  	u64	rate_bps;
>  	u32	mult;
>  	u16	overhead;
> +	u8	linklayer;
>  	u8	shift;
>  };
>  
>  static inline u64 psched_l2t_ns(const struct psched_ratecfg *r,
>  				unsigned int len)
>  {
> -	return ((u64)(len + r->overhead) * r->mult) >> r->shift;
> +	u64 pkt_len = len + r->overhead;
> +
> +	if (r->linklayer == TC_LINKLAYER_ATM)
> +		pkt_len = DIV_ROUND_UP(pkt_len,48)*53;

Is this working on 32bit kernel ?

> +	return (pkt_len * r->mult) >> r->shift;
>  }

I have no idea why you include so many people on your mails.

This looks like ATM link have real rate of 48/53 of the rate.


Since we have to distribute a new tc version, why not doing

"tc ... rate rate X ceil Y linklayer atm"

->

"tc ... rate  X*48/53 Y*48/53"

It avoids hard coded values in the kernel.

Or make it use STAB if people really want to count bits/cells instead of
bytes.

Lets try to keep this overhead out of the fast path.

Re: [PATCH iproute2] htb: report overhead attribute

[Stephen Hemminger]

On Sun, 02 Jun 2013 14:33:01 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> From: Eric Dumazet <edumazet@google.com>
> 
> "tc class show dev ..." omits the overhead attribute for HTB.
> 
> After patch I have :
> 
> tc class add dev $DEV parent 1: classid 1:1 est 1sec 4sec htb \
>     rate 12Mbit mtu 1500 quantum 1514 overhead 20
> 
> tc class show dev $DEV
> class htb 1:1 root prio 0 rate 12000Kbit overhead 20 ceil 12000Kbit
> burst 1500b cburst 1500b
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied.
And after that fixed indentation of q_htb.c

Re: [PATCH iproute2] htb: report overhead attribute

[Eric Dumazet]

On Fri, 2013-06-07 at 08:56 -0700, Stephen Hemminger wrote:

> Applied.
> And after that fixed indentation of q_htb.c

Yes, I saw the indentation was strange indeed ;)

Thanks