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

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

[Hagen Paul Pfeifer]

Not sure if suitable but netem rate have a cell mechanism as well. See man netem

Eric Dumazet <eric.dumazet@gmail.com> schrieb:

>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.
>
>
>
>_______________________________________________
>Bloat mailing list
>Bloat@lists.bufferbloat.net
>https://lists.bufferbloat.net/listinfo/bloat
>

Re: [Bloat] 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;

Re: [Bloat] 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: [Bloat] 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: [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: [Bloat] 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)

[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: [Bloat] 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: [Bloat] tc linklayer ADSL calc broken after commit 56b765b79 (htb: improved accuracy at high rates)

[Dave Taht]

[-- Attachment #1: Type: text/plain, Size: 1988 bytes --]

On May 29, 2013 6:50 PM, "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. ?
> > >
> > >
> > > 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 #2: Type: text/html, Size: 2786 bytes --]

Re: [Bloat] 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: [Bloat] 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: [Bloat] 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 :

[Bloat] 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