[Cake] overheads or rate calculation changed?

[Cake] overheads or rate calculation changed?

[Andy Furniss]

Been running a cake cobolt from april for some time using

tc qdisc add dev ppp0 handle 1:0 root cake bandwidth 19690kbit raw 
overhead 34 diffserv4 dual-srchost nat rtt 200ms

tc -s qdisc ls dev ppp0 (ppp0 is pppoe)

qdisc cake 1: root refcnt 2 bandwidth 19690Kbit diffserv4 dual-srchost 
nat rtt 200.0ms noatm overhead 56 via-ethernet

Which is backed off a couple of kbit and tests well (overhead 33 + udp 
flood is over limit)

With current cobalt + current tc the same command is under rate - I 
don't know if its rate calculation or overheads calc changed.

overhead 20 (-14) is still under but overhead 12 (-22) is over.

I was hoping one of those would be correct, but no.

Any ideas/known changes?

Re: [Cake] overheads or rate calculation changed?

[Andy Furniss]

Andy Furniss wrote:
> Been running a cake cobolt from april for some time using
> 
> tc qdisc add dev ppp0 handle 1:0 root cake bandwidth 19690kbit raw 
> overhead 34 diffserv4 dual-srchost nat rtt 200ms
> 
> tc -s qdisc ls dev ppp0 (ppp0 is pppoe)
> 
> qdisc cake 1: root refcnt 2 bandwidth 19690Kbit diffserv4 dual-srchost 
> nat rtt 200.0ms noatm overhead 56 via-ethernet
> 
> Which is backed off a couple of kbit and tests well (overhead 33 + udp 
> flood is over limit)
> 
> With current cobalt + current tc the same command is under rate - I 
> don't know if its rate calculation or overheads calc changed.
> 
> overhead 20 (-14) is still under but overhead 12 (-22) is over.
> 
> I was hoping one of those would be correct, but no.
> 
> Any ideas/known changes?

Bisected to -

f33c4d6a08cb21374ce52fc1438a8531bc84bcbe is the first bad commit
commit f33c4d6a08cb21374ce52fc1438a8531bc84bcbe
Author: Dave Taht <dave.taht@gmail.com>
Date:   Wed Nov 22 16:21:13 2017 -0800

     refactor cake_advance_shaper and ack_filter

     cake_advance_shaper now returns a modified len argument to
     reflect cake_overhead.
     skb_ack_filter variable replaced with ack

Re: [Cake] overheads or rate calculation changed?

[Jonathan Morton]

> On 21 Dec, 2017, at 2:54 am, Andy Furniss <adf.lists@gmail.com> wrote:
> 
>    refactor cake_advance_shaper and ack_filter
> 
>    cake_advance_shaper now returns a modified len argument to
>    reflect cake_overhead.
>    skb_ack_filter variable replaced with ack

Fixed.  At one point cake_advance_shaper() was still getting a packet length with overhead correction already applied, and was then applying it a second time.

 - Jonathan Morton

Re: [Cake] overheads or rate calculation changed?

[Kevin Darbyshire-Bryant]

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



> On 22 Dec 2017, at 06:38, Jonathan Morton <chromatix99@gmail.com> wrote:
> 
>> On 21 Dec, 2017, at 2:54 am, Andy Furniss <adf.lists@gmail.com> wrote:
>> 
>>   refactor cake_advance_shaper and ack_filter
>> 
>>   cake_advance_shaper now returns a modified len argument to
>>   reflect cake_overhead.
>>   skb_ack_filter variable replaced with ack
> 
> Fixed.  At one point cake_advance_shaper() was still getting a packet length with overhead correction already applied, and was then applying it a second time.

I’m confused.  A diff between where cobalt was yesterday and now this morning:

diff --git a/sch_cake.c b/sch_cake.c
index 021b215..49ecf5e 100644
--- a/sch_cake.c
+++ b/sch_cake.c
@@ -151,6 +151,7 @@ struct cake_host {
 	u32 dsthost_tag;
 	u16 srchost_refcnt;
 	u16 dsthost_refcnt;
+	u32 pad;
 };

 struct cake_heap_entry {
@@ -1882,10 +1883,6 @@ retry:
 	b->tin_ecn_mark += !!flow->cvars.ecn_marked;
 	qdisc_bstats_update(sch, skb);

-	len = cake_overhead(q, qdisc_pkt_len(skb));
-	flow->deficit -= len;
-	b->tin_deficit -= len;
-
 	/* collect delay stats */
 	delay = now - cobalt_get_enqueue_time(skb);
 	b->avge_delay = cake_ewma(b->avge_delay, delay, 8);
@@ -1894,7 +1891,10 @@ retry:
 	b->base_delay = cake_ewma(b->base_delay, delay,
 				  delay < b->base_delay ? 2 : 8);

-	cake_advance_shaper(q, b, len, now, false);
+	len = cake_advance_shaper(q, b, len, now, false);
+	flow->deficit -= len;
+	b->tin_deficit -= len;
+
 	if (q->time_next_packet > now && sch->q.qlen) {
 		u64 next = min(q->time_next_packet, q->failsafe_next_packet);
 #if LINUX_VERSION_CODE < KERNEL_VERSION(4, 8, 0)
@@ -1959,7 +1959,7 @@ static void cake_set_rate(struct cake_tin_data *b, u64 rate, u32 mtu,
 	 */
 	static const u64 MIN_RATE = 64;
 	u64 rate_ns = 0;
-	u8  rate_shft = 0;
+	u8  rate_shft = 2;
 	cobalt_time_t byte_target_ns;
 	u32 byte_target = mtu;


pad is back, and rate_shft = 2 in addition to the document overhead stuff moving.

Is this a bad merge?   Which is correct?

If it helps, this is a list of the commits that are in post pull cobalt but not in pre pull cobalt.

commit dd4054684a9850d41f267f03ce758980cc4997c4
Author: Jonathan Morton <chromatix99@gmail.com>
Date:   Fri Dec 22 08:36:07 2017 +0200

    Fix overhead accounting after Dave's refactor.

commit 718114af963740564e7d014b5e0d56ccc7387713
Merge: 8052827 49776da
Author: Jonathan Morton <chromatix99@gmail.com>
Date:   Fri Dec 22 08:33:36 2017 +0200

    Merge branch 'cobalt' of https://github.com/dtaht/sch_cake into cobalt

commit 80528271b0eef37064b04d213071faf56f3f01bb
Author: Jonathan Morton <chromatix99@gmail.com>
Date:   Fri Jul 21 15:18:56 2017 +0300

    Improve ingress-mode failsafe behaviour under sustained prior load.

commit 06b301f3a0c7719bddfba4e7f71dc437ad53b88c
Author: Jonathan Morton <chromatix99@gmail.com>
Date:   Fri Jul 21 15:07:34 2017 +0300

    Add a failsafe to ingress mode, so it always delivers some packets even at high drop rates.

commit 2ed46bba1a97feb6ce728abbfbf6d18211317f33
Author: Jonathan Morton <chromatix99@gmail.com>
Date:   Sat May 13 00:07:14 2017 +0300

    Try to solve higher sparse latency in ingress mode when non-ECN bulk flows are present.

commit 79b9b76b5dc65e8f8d629bbe636443bf04a6b37f
Author: Jonathan Morton <chromatix99@gmail.com>
Date:   Sat May 6 00:50:07 2017 +0300

    Rate variables in diffserv_*() shouldn't be 64-bit; this causes difficulty with dividing them on 32-bit platforms.

commit 3ab36e8d8431eb941ceb0608b739fa02a325de88
Author: Jonathan Morton <chromatix99@gmail.com>
Date:   Sat May 6 00:39:31 2017 +0300

    Properly initialise the policy array.

commit 833c87c20f5de0165957326677334009d10aed37
Author: Jonathan Morton <chromatix99@gmail.com>
Date:   Fri May 5 22:53:59 2017 +0300

    Try to handle decaying flows and packet trains more efficiently.

commit 8978b2487b6adbc4f17612e5e123125971eed4fa
Author: Jonathan Morton <chromatix99@gmail.com>
Date:   Fri May 5 18:02:45 2017 +0300

    Proper fix for diffserv-llt mode.



Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] overheads or rate calculation changed?

[Jonathan Morton]

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

Git seems to regularly get confused when similar code changes occur in
parallel in different branches.  In this case, I had the original version
of ingress mode while the public tree had the reconstructed version -
almost identical, but still constituting a merge conflict.  I really don't
know why the 'pad' thing wasn't similarly flagged.

If there's an easy way to simply accept the remote version of the file as
correct and not bother generating a merge commit, I couldn't find it.

In any case I need to thoroughly review all this code before I can sign off
on it.  That'll also give me an opportunity to sort out the stats somehow.

- Jonathan Morton

[-- Attachment #2: Type: text/html, Size: 764 bytes --]

Re: [Cake] overheads or rate calculation changed?

[Kevin Darbyshire-Bryant]

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



> On 22 Dec 2017, at 10:00, Jonathan Morton <chromatix99@gmail.com> wrote:
> 
> Git seems to regularly get confused when similar code changes occur in parallel in different branches.  In this case, I had the original version of ingress mode while the public tree had the reconstructed version - almost identical, but still constituting a merge conflict.  I really don't know why the 'pad' thing wasn't similarly flagged.
> 
> If there's an easy way to simply accept the remote version of the file as correct and not bother generating a merge commit, I couldn't find it.

I’ve been unable to discern what distribution/branch/merge strategy is being aimed for here so can’t offer constructive advice.  Git has always done what it’s supposed to for me, that may have been not quite what I expected but it’s always been logical.

> 
> In any case I need to thoroughly review all this code before I can sign off on it.  That'll also give me an opportunity to sort out the stats somehow.
> 
> - Jonathan Morton


Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] overheads or rate calculation changed?

[Dave Taht]

Thx andy and jon for the bisect and the fix. I am deep in the jungles
of nicaragua at the moment, and won't be doing much with any codebases
until I get out in mid january - the internet is terrible here, and
where I am hasn't had power in a couple days, either!

Expect sparse emails from me til then.

(But the food, surf, and alcohol are fantastic, and I'm trying to fix
the local wifi AP pointed 8km into town).

I kind of gave up on the mainlining attempt this go-round (I don't
understand why the kbuildbot kicked it back, there's no stack use in
the function it dislikes that I can see!?).

if this shaper fix was 'the last bug" on the reorganized code, I'd
support someone (with reliable internet) pushing it to lede.

On Thu, Dec 21, 2017 at 10:38 PM, Jonathan Morton <chromatix99@gmail.com> wrote:
>> On 21 Dec, 2017, at 2:54 am, Andy Furniss <adf.lists@gmail.com> wrote:
>>
>>    refactor cake_advance_shaper and ack_filter
>>
>>    cake_advance_shaper now returns a modified len argument to
>>    reflect cake_overhead.
>>    skb_ack_filter variable replaced with ack
>
> Fixed.  At one point cake_advance_shaper() was still getting a packet length with overhead correction already applied, and was then applying it a second time.
>
>  - Jonathan Morton
>
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake



-- 

Dave Täht
CEO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-669-226-2619

Re: [Cake] overheads or rate calculation changed?

[Andy Furniss]

Jonathan Morton wrote:
>> On 21 Dec, 2017, at 2:54 am, Andy Furniss <adf.lists@gmail.com> wrote:
>>
>>     refactor cake_advance_shaper and ack_filter
>>
>>     cake_advance_shaper now returns a modified len argument to
>>     reflect cake_overhead.
>>     skb_ack_filter variable replaced with ack
> 
> Fixed.  At one point cake_advance_shaper() was still getting a packet length with overhead correction already applied, and was then applying it a second time.

Thanks, seems good now, well, as it was before anyway :-)

Still a bit confusing for users having to look at max_len in stats to 
judge what overhead to use if they know what over ip they want.

Maybe some hint in the man page about turn off gro and look at max_len 
would help when using raw.

IIRC this was discussed before, I think conclusion was it's the 
kernel/ppp being different/wrong sometimes in what it reports to cake.

My specific case is pppoe where I need IP + 34. I can see 1500 as 
max_len, which = IP length so raw overhead 34 is correct.

On a pure eth to get the same I need raw overhead 20 as max_len is 1514.

This is the (confusing) output from pppoe

./tc qdisc add dev ppp0 handle 1:0 root cake bandwidth 19690kbit raw 
overhead 34 diffserv4 dual-srchost nat rtt 200ms

qdisc cake 1: dev ppp0 root refcnt 2 bandwidth 19690Kbit diffserv4 
dual-srchost nat rtt 200.0ms noatm overhead 56 via-ethernet 
total_overhead 56 hard_header_len 22
  Sent 776346756 bytes 883288 pkt (dropped 284882, overlimits 1429302 
requeues 0)
  backlog 0b 0p requeues 0
  memory used: 4196096b of 4Mb
  capacity estimate: 19690Kbit
                    Bulk  Best Effort        Video        Voice
   thresh       1230Kbit    19690Kbit     9845Kbit     4922Kbit
   target         14.8ms       10.0ms       10.0ms       10.0ms
   interval      204.8ms      200.0ms      200.0ms      200.0ms
   pk_delay        186us        1.6ms          0us        112us
   av_delay         11us         94us          0us          4us
   sp_delay          1us          4us          0us          4us
   pkts             1160      1166876            0          134
   bytes          457795   1202631653            0        10176
   way_inds            0         3813            0            0
   way_miss         1126        10503            0            3
   way_cols            0            0            0            0
   drops               0       284882            0            0
   marks               0            0            0            0
   ack_drop            0            0            0            0
   sp_flows            0            1            0            0
   bk_flows            0            1            0            0
   un_flows            0            0            0            0
   max_len           746         1500            0           76



On eth raw overhead 34 gives different results, only seeing max_len 
gives the clue to take 14 to get IP + 34.

tc qdisc add dev enp6s0 handle 1:0 root cake bandwidth 19690kbit raw 
overhead 34

qdisc cake 1: dev enp6s0 root refcnt 2 bandwidth 19690Kbit diffserv3 
triple-isolate rtt 100.0ms noatm overhead 48 via-ethernet total_overhead 
48 hard_header_len 14
  Sent 24102094 bytes 15934 pkt (dropped 1, overlimits 31841 requeues 0)
  backlog 0b 0p requeues 0
  memory used: 27Kb of 4Mb
  capacity estimate: 19690Kbit
                    Bulk  Best Effort        Voice
   thresh       1230Kbit    19690Kbit     4922Kbit
   target         14.8ms        5.0ms        5.0ms
   interval      109.8ms      100.0ms      100.0ms
   pk_delay          0us        1.7ms          0us
   av_delay          0us        1.7ms          0us
   sp_delay          0us        229us          0us
   pkts                0        15935            0
   bytes               0     24103608            0
   way_inds            0            0            0
   way_miss            0            4            0
   way_cols            0            0            0
   drops               0            1            0
   marks               0            0            0
   ack_drop            0            0            0
   sp_flows            0            0            0
   bk_flows            0            1            0
   un_flows            0            0            0
   max_len             0         1514            0

Re: [Cake] overheads or rate calculation changed?

[Sebastian Moeller]

Hi Andy,


> On Dec 23, 2017, at 00:38, Andy Furniss <adf.lists@gmail.com> wrote:
> 
> Jonathan Morton wrote:
>>> On 21 Dec, 2017, at 2:54 am, Andy Furniss <adf.lists@gmail.com> wrote:
>>> 
>>>    refactor cake_advance_shaper and ack_filter
>>> 
>>>    cake_advance_shaper now returns a modified len argument to
>>>    reflect cake_overhead.
>>>    skb_ack_filter variable replaced with ack
>> Fixed.  At one point cake_advance_shaper() was still getting a packet length with overhead correction already applied, and was then applying it a second time.
> 
> Thanks, seems good now, well, as it was before anyway :-)
> 
> Still a bit confusing for users having to look at max_len in stats to judge what overhead to use if they know what over ip they want.
> 
> Maybe some hint in the man page about turn off gro and look at max_len would help when using raw.
> 
> IIRC this was discussed before, I think conclusion was it's the kernel/ppp being different/wrong sometimes in what it reports to cake.
> 
> My specific case is pppoe where I need IP + 34. I can see 1500 as max_len, which = IP length so raw overhead 34 is correct.
> 
> On a pure eth to get the same I need raw overhead 20 as max_len is 1514.
> 
> This is the (confusing) output from pppoe
> 
> ./tc qdisc add dev ppp0 handle 1:0 root cake bandwidth 19690kbit raw overhead 34 diffserv4 dual-srchost nat rtt 200ms
> 
> qdisc cake 1: dev ppp0 root refcnt 2 bandwidth 19690Kbit diffserv4 dual-srchost nat rtt 200.0ms noatm overhead 56 via-ethernet total_overhead 56 hard_header_len 22

	This is still broken, the overhead the should just be 34, and hard_header_len should be 0, even though for pppoe 22 is not completely wrong (14 + 8). I currently see the same with PPPoE+VLAN I get 26. So either that is a change in current cake or the kernel actually started to report a more precise hard_header_len...


> Sent 776346756 bytes 883288 pkt (dropped 284882, overlimits 1429302 requeues 0)
> backlog 0b 0p requeues 0
> memory used: 4196096b of 4Mb
> capacity estimate: 19690Kbit
>                   Bulk  Best Effort        Video        Voice
>  thresh       1230Kbit    19690Kbit     9845Kbit     4922Kbit
>  target         14.8ms       10.0ms       10.0ms       10.0ms
>  interval      204.8ms      200.0ms      200.0ms      200.0ms
>  pk_delay        186us        1.6ms          0us        112us
>  av_delay         11us         94us          0us          4us
>  sp_delay          1us          4us          0us          4us
>  pkts             1160      1166876            0          134
>  bytes          457795   1202631653            0        10176
>  way_inds            0         3813            0            0
>  way_miss         1126        10503            0            3
>  way_cols            0            0            0            0
>  drops               0       284882            0            0
>  marks               0            0            0            0
>  ack_drop            0            0            0            0
>  sp_flows            0            1            0            0
>  bk_flows            0            1            0            0
>  un_flows            0            0            0            0
>  max_len           746         1500            0           76
> 
> 
> 
> On eth raw overhead 34 gives different results, only seeing max_len gives the clue to take 14 to get IP + 34.
> 
> tc qdisc add dev enp6s0 handle 1:0 root cake bandwidth 19690kbit raw overhead 34
> 
> qdisc cake 1: dev enp6s0 root refcnt 2 bandwidth 19690Kbit diffserv3 triple-isolate rtt 100.0ms noatm overhead 48 via-ethernet total_overhead 48 hard_header_len 14

	This looks like expected, the in-kernel overhead is added to the specified overhead, just like in the past. I really really wished cake would report the packet size before and _after_ its overhead addition making sanity checking much easier. BTW tc's stab might be useful as an external check... I also wonder whether the kernel's behaviour in regards to ppp interfaces changed between kernel 4.4 and newer ones, I see the same weirdness:
root@router:~# uname -a
Linux router 4.9.67 #0 Sun Dec 17 11:53:32 2017 mips GNU/Linux
excerpt from tc -s:
qdisc cake 800f: dev pppoe-wan root refcnt 2 bandwidth 9545Kbit diffserv3 dual-srchost nat rtt 100.0ms noatm overhead 34 via-ethernet total_overhead 34 hard_header_len 26 mpu 64 

I note that hard_header_len 26 sort of matches the situation on the ground: 
Mine:
14 (SRCMAC, DSTMAC, ethertype) + 8 (PPP, PPPOE), + 4 (VLAN) = 26
Yours: 
14 (SRCMAC, DSTMAC, ethertype) + 8 (PPP, PPPOE) = 22

I guess I should try with different kernels and recent sch_cake...




Best Regards
	Sebastian


> Sent 24102094 bytes 15934 pkt (dropped 1, overlimits 31841 requeues 0)
> backlog 0b 0p requeues 0
> memory used: 27Kb of 4Mb
> capacity estimate: 19690Kbit
>                   Bulk  Best Effort        Voice
>  thresh       1230Kbit    19690Kbit     4922Kbit
>  target         14.8ms        5.0ms        5.0ms
>  interval      109.8ms      100.0ms      100.0ms
>  pk_delay          0us        1.7ms          0us
>  av_delay          0us        1.7ms          0us
>  sp_delay          0us        229us          0us
>  pkts                0        15935            0
>  bytes               0     24103608            0
>  way_inds            0            0            0
>  way_miss            0            4            0
>  way_cols            0            0            0
>  drops               0            1            0
>  marks               0            0            0
>  ack_drop            0            0            0
>  sp_flows            0            0            0
>  bk_flows            0            1            0
>  un_flows            0            0            0
>  max_len             0         1514            0
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] overheads or rate calculation changed?

[Andy Furniss]

Sebastian Moeller wrote:

>> qdisc cake 1: dev enp6s0 root refcnt 2 bandwidth 19690Kbit diffserv3 triple-isolate rtt 100.0ms noatm overhead 48 via-ethernet total_overhead 48 hard_header_len 14
> 
> 	This looks like expected, the in-kernel overhead is added to the specified overhead, just like in the past. I really really wished cake would report the packet size before and _after_ its overhead addition making sanity checking much easier. BTW tc's stab might be useful as an external check... I also wonder whether the kernel's behaviour in regards to ppp interfaces changed between kernel 4.4 and newer ones, I see the same weirdness:

FWIW my pppoe router/pvr/nas box is running 4.1.46. It's a bay-trail dc 
board and they are prone to cstate bug locks, which is why I'm not 
running anything newer!

Re: [Cake] overheads or rate calculation changed?

[Sebastian Moeller]

> On Dec 23, 2017, at 10:59, Andy Furniss <adf.lists@gmail.com> wrote:
> 
> Sebastian Moeller wrote:
> 
>>> qdisc cake 1: dev enp6s0 root refcnt 2 bandwidth 19690Kbit diffserv3 triple-isolate rtt 100.0ms noatm overhead 48 via-ethernet total_overhead 48 hard_header_len 14
>> 	This looks like expected, the in-kernel overhead is added to the specified overhead, just like in the past. I really really wished cake would report the packet size before and _after_ its overhead addition making sanity checking much easier. BTW tc's stab might be useful as an external check... I also wonder whether the kernel's behaviour in regards to ppp interfaces changed between kernel 4.4 and newer ones, I see the same weirdness:
> 
> FWIW my pppoe router/pvr/nas box is running 4.1.46. It's a bay-trail dc board and they are prone to cstate bug locks, which is why I'm not running anything newer!
> 

Thanks for saving me the experiments...

Mmh, that would indicate that dev->hard_header_len might not be the relevant variable, it might be necessary to look into the IP packets total length and the reported packet/frame size to figure out what the kernel really added (since the kernel addition should not change it might be enough to do this only for the first IP packet and cache that value).

Best Regards
	Sebastian

Re: [Cake] overheads or rate calculation changed?

[Ryan Mounce]

On 4.9 I am seeing a max_len equal to my IP MTU of on PPPoE
interfaces, for both egress (hard_header_len = 26) and ingress via ifb
(hard_header_len = 14). At least this issue had been offset by the
double-overhead bug for a little while :)

Regards,
Ryan Mounce


On 23 December 2017 at 23:25, Sebastian Moeller <moeller0@gmx.de> wrote:
>
>> On Dec 23, 2017, at 10:59, Andy Furniss <adf.lists@gmail.com> wrote:
>>
>> Sebastian Moeller wrote:
>>
>>>> qdisc cake 1: dev enp6s0 root refcnt 2 bandwidth 19690Kbit diffserv3 triple-isolate rtt 100.0ms noatm overhead 48 via-ethernet total_overhead 48 hard_header_len 14
>>>      This looks like expected, the in-kernel overhead is added to the specified overhead, just like in the past. I really really wished cake would report the packet size before and _after_ its overhead addition making sanity checking much easier. BTW tc's stab might be useful as an external check... I also wonder whether the kernel's behaviour in regards to ppp interfaces changed between kernel 4.4 and newer ones, I see the same weirdness:
>>
>> FWIW my pppoe router/pvr/nas box is running 4.1.46. It's a bay-trail dc board and they are prone to cstate bug locks, which is why I'm not running anything newer!
>>
>
> Thanks for saving me the experiments...
>
> Mmh, that would indicate that dev->hard_header_len might not be the relevant variable, it might be necessary to look into the IP packets total length and the reported packet/frame size to figure out what the kernel really added (since the kernel addition should not change it might be enough to do this only for the first IP packet and cache that value).
>
> Best Regards
>         Sebastian
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] overheads or rate calculation changed?

[Sebastian Moeller]

Hi Ryan,


> On Dec 23, 2017, at 14:11, Ryan Mounce <ryan@mounce.com.au> wrote:
> 
> On 4.9 I am seeing a max_len equal to my IP MTU of on PPPoE
> interfaces,

That is the traditional indicator, that the kernel does not automatically add overhead for ppp interfaces, and that still seems valid.

> for both egress (hard_header_len = 26) and ingress via ifb
> (hard_header_len = 14).

	Yes, that matches what we saw before...

> At least this issue had been offset by the
> double-overhead bug for a little while :)

	I guess we need to retire the automatic adjustments the way they are done now and I will need to write an instruction how to deduce the applied overhead manually. That or I try to see whether my idea "compare the first IP packets skb->length with the value of its total length header field and report and correct for the difference between the two... But that requires some research first why the hard_header_len values for pppoe interfaces are as we see them (my hypothesis is that the kernel actually accounts for all known overheads for the whole interface stack (in my case pppoe-wan -> eth1.7 -> eth1) somehow)
I guess I should have looked at the actual assumed kernel-added overhead earlier, but I only went as far as testing real ethernet interfaces. I maintain that if cake would report the max_len_with_overhead along with max_len this would have been way more prominent and easier to debug.


Best Regards
	Sebastian

> 
> Regards,
> Ryan Mounce
> 
> 
> On 23 December 2017 at 23:25, Sebastian Moeller <moeller0@gmx.de> wrote:
>> 
>>> On Dec 23, 2017, at 10:59, Andy Furniss <adf.lists@gmail.com> wrote:
>>> 
>>> Sebastian Moeller wrote:
>>> 
>>>>> qdisc cake 1: dev enp6s0 root refcnt 2 bandwidth 19690Kbit diffserv3 triple-isolate rtt 100.0ms noatm overhead 48 via-ethernet total_overhead 48 hard_header_len 14
>>>>     This looks like expected, the in-kernel overhead is added to the specified overhead, just like in the past. I really really wished cake would report the packet size before and _after_ its overhead addition making sanity checking much easier. BTW tc's stab might be useful as an external check... I also wonder whether the kernel's behaviour in regards to ppp interfaces changed between kernel 4.4 and newer ones, I see the same weirdness:
>>> 
>>> FWIW my pppoe router/pvr/nas box is running 4.1.46. It's a bay-trail dc board and they are prone to cstate bug locks, which is why I'm not running anything newer!
>>> 
>> 
>> Thanks for saving me the experiments...
>> 
>> Mmh, that would indicate that dev->hard_header_len might not be the relevant variable, it might be necessary to look into the IP packets total length and the reported packet/frame size to figure out what the kernel really added (since the kernel addition should not change it might be enough to do this only for the first IP packet and cache that value).
>> 
>> Best Regards
>>        Sebastian
>> _______________________________________________
>> Cake mailing list
>> Cake@lists.bufferbloat.net
>> https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] overheads or rate calculation changed?

[Sebastian Moeller]

Hi All,

just had a look for hard_header_len in the linux kernel:
linux/include/linux/netdevice.h:
*      @hard_header_len: Maximum hardware header length.
 *      @min_header_len:  Minimum hardware header length

this seems to corroborate our observation that hard_header_len is not a veridical representation of the actual hardware header length, so I assume the values cake returns are actually true. It also indicates that except for pure ethernet interfaces hard_header_len is _not_ the right parameter to evaluate for what cake is evaluating it for...

Best Regards
	Sebastian




> On Dec 23, 2017, at 15:21, Sebastian Moeller <moeller0@gmx.de> wrote:
> 
> Hi Ryan,
> 
> 
>> On Dec 23, 2017, at 14:11, Ryan Mounce <ryan@mounce.com.au> wrote:
>> 
>> On 4.9 I am seeing a max_len equal to my IP MTU of on PPPoE
>> interfaces,
> 
> That is the traditional indicator, that the kernel does not automatically add overhead for ppp interfaces, and that still seems valid.
> 
>> for both egress (hard_header_len = 26) and ingress via ifb
>> (hard_header_len = 14).
> 
> 	Yes, that matches what we saw before...
> 
>> At least this issue had been offset by the
>> double-overhead bug for a little while :)
> 
> 	I guess we need to retire the automatic adjustments the way they are done now and I will need to write an instruction how to deduce the applied overhead manually. That or I try to see whether my idea "compare the first IP packets skb->length with the value of its total length header field and report and correct for the difference between the two... But that requires some research first why the hard_header_len values for pppoe interfaces are as we see them (my hypothesis is that the kernel actually accounts for all known overheads for the whole interface stack (in my case pppoe-wan -> eth1.7 -> eth1) somehow)
> I guess I should have looked at the actual assumed kernel-added overhead earlier, but I only went as far as testing real ethernet interfaces. I maintain that if cake would report the max_len_with_overhead along with max_len this would have been way more prominent and easier to debug.
> 
> 
> Best Regards
> 	Sebastian
> 
>> 
>> Regards,
>> Ryan Mounce
>> 
>> 
>> On 23 December 2017 at 23:25, Sebastian Moeller <moeller0@gmx.de> wrote:
>>> 
>>>> On Dec 23, 2017, at 10:59, Andy Furniss <adf.lists@gmail.com> wrote:
>>>> 
>>>> Sebastian Moeller wrote:
>>>> 
>>>>>> qdisc cake 1: dev enp6s0 root refcnt 2 bandwidth 19690Kbit diffserv3 triple-isolate rtt 100.0ms noatm overhead 48 via-ethernet total_overhead 48 hard_header_len 14
>>>>>    This looks like expected, the in-kernel overhead is added to the specified overhead, just like in the past. I really really wished cake would report the packet size before and _after_ its overhead addition making sanity checking much easier. BTW tc's stab might be useful as an external check... I also wonder whether the kernel's behaviour in regards to ppp interfaces changed between kernel 4.4 and newer ones, I see the same weirdness:
>>>> 
>>>> FWIW my pppoe router/pvr/nas box is running 4.1.46. It's a bay-trail dc board and they are prone to cstate bug locks, which is why I'm not running anything newer!
>>>> 
>>> 
>>> Thanks for saving me the experiments...
>>> 
>>> Mmh, that would indicate that dev->hard_header_len might not be the relevant variable, it might be necessary to look into the IP packets total length and the reported packet/frame size to figure out what the kernel really added (since the kernel addition should not change it might be enough to do this only for the first IP packet and cache that value).
>>> 
>>> Best Regards
>>>       Sebastian
>>> _______________________________________________
>>> Cake mailing list
>>> Cake@lists.bufferbloat.net
>>> https://lists.bufferbloat.net/listinfo/cake
> 
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] overheads or rate calculation changed?

[Jonathan Morton]

> On 23 Dec, 2017, at 11:03 pm, Sebastian Moeller <moeller0@gmx.de> wrote:
> 
> just had a look for hard_header_len in the linux kernel:
> linux/include/linux/netdevice.h:
> *      @hard_header_len: Maximum hardware header length.
> *      @min_header_len:  Minimum hardware header length
> 
> this seems to corroborate our observation that hard_header_len is not a veridical representation of the actual hardware header length, so I assume the values cake returns are actually true. It also indicates that except for pure ethernet interfaces hard_header_len is _not_ the right parameter to evaluate for what cake is evaluating it for...

I'll keep that in mind for my review pass.

 - Jonathan Morton

Re: [Cake] overheads or rate calculation changed?

[Kevin Darbyshire-Bryant]

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



> On 23 Dec 2017, at 21:03, Sebastian Moeller <moeller0@gmx.de> wrote:
> 
> Hi All,
> 
> just had a look for hard_header_len in the linux kernel:
> linux/include/linux/netdevice.h:
> *      @hard_header_len: Maximum hardware header length.
> *      @min_header_len:  Minimum hardware header length
> 
> this seems to corroborate our observation that hard_header_len is not a veridical representation of the actual hardware header length, so I assume the values cake returns are actually true. It also indicates that except for pure ethernet interfaces hard_header_len is _not_ the right parameter to evaluate for what cake is evaluating it for...

What came as a surprise to me the other day is that whatever ‘overhead’ you specify on the command line must *include* the hard_header_len figure, since the code subtracts ‘hard header len’ from the passed overhead value.  I’ve probably been doing this wrong for… who knows how long.

        if (tb[TCA_CAKE_OVERHEAD]) {
                if (tb[TCA_CAKE_ETHERNET])    <<<— this is really a synonym for ‘raw’, in my case it isn’t passed so else is exec
                        q->rate_overhead = -(nla_get_s32(tb[TCA_CAKE_ETHERNET]));
                else
                        q->rate_overhead = -(qdisc_dev(sch)->hard_header_len);     <<<—note the sneaky minus!

                q->rate_overhead += nla_get_s32(tb[TCA_CAKE_OVERHEAD]);

For a while I’ve manually been passing ’12’ as a ‘bridged-ptm ether-vlan’ equivalent except I should have been passing ’26’.  Instead I’ve been reducing the length of packets by 2 bytes :-)   I now just pass the relevant keywords.

Cheers,

Kevin D-B
Falling into traps so you don’t have to(tm)

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] overheads or rate calculation changed?

[Jonathan Morton]

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

If you combine the 'raw' keyword with an overhead spec, that disables the
hard_header_len compensation.

- Jonathan Morton

[-- Attachment #2: Type: text/html, Size: 170 bytes --]

Re: [Cake] overheads or rate calculation changed?

[Kevin Darbyshire-Bryant]

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



> On 24 Dec 2017, at 10:39, Jonathan Morton <chromatix99@gmail.com> wrote:
> 
> If you combine the 'raw' keyword with an overhead spec, that disables the hard_header_len compensation.
> 
> - Jonathan Morton

Indeed.  I think all I’m proving is ‘make something idiot proof…..they’ll only go and improve the idiot’… i.e. me :-)


Happy holiday season to all.  I’m off to find another trap to fall into ;-)

Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] overheads or rate calculation changed?

[Sebastian Moeller]

Hi Kevin,

On December 24, 2017 11:34:15 AM GMT+01:00, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
>
>
>> On 23 Dec 2017, at 21:03, Sebastian Moeller <moeller0@gmx.de> wrote:
>> 
>> Hi All,
>> 
>> just had a look for hard_header_len in the linux kernel:
>> linux/include/linux/netdevice.h:
>> *      @hard_header_len: Maximum hardware header length.
>> *      @min_header_len:  Minimum hardware header length
>> 
>> this seems to corroborate our observation that hard_header_len is not
>a veridical representation of the actual hardware header length, so I
>assume the values cake returns are actually true. It also indicates
>that except for pure ethernet interfaces hard_header_len is _not_ the
>right parameter to evaluate for what cake is evaluating it for...
>
>What came as a surprise to me the other day is that whatever ‘overhead’
>you specify on the command line must *include* the hard_header_len
>figure, since the code subtracts ‘hard header len’ from the passed
>overhead value.  I’ve probably been doing this wrong for… who knows how
>long.

Well, cake's motives were pure... Unfortunately it seems that the implementation just worked for Ethernet....
This behavior is also not terribly strongly advertized, probably a good thing with the currently exposed opportunities for further precision improvements.


>
>        if (tb[TCA_CAKE_OVERHEAD]) {
>if (tb[TCA_CAKE_ETHERNET])    <<<— this is really a synonym for ‘raw’,
>in my case it isn’t passed so else is exec
>              q->rate_overhead = -(nla_get_s32(tb[TCA_CAKE_ETHERNET]));
>                else
>q->rate_overhead = -(qdisc_dev(sch)->hard_header_len);     <<<—note the
>sneaky minus!
>
>                q->rate_overhead += nla_get_s32(tb[TCA_CAKE_OVERHEAD]);
>
>For a while I’ve manually been passing ’12’ as a ‘bridged-ptm
>ether-vlan’ equivalent except I should have been passing ’26’.  Instead
>I’ve been reducing the length of packets by 2 bytes :-)   I now just
>pass the relevant keywords.

The key words probably do not really save you... Plus the ptm accounting as done by cake is imprecise, or rather it is imprecise and incurres a (very small) per packet cost, while statically reducing the ptm bearer's synchrate by 100-100*64/65 at configuration time, is more precise and has 0 added per packet cost, but I digress.

Best Regards
        Sebastian


>
>Cheers,
>
>Kevin D-B
>Falling into traps so you don’t have to(tm)
>
>012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [Cake] overheads or rate calculation changed?

[Sebastian Moeller]

Hi Kevin, hi all,

On December 24, 2017 11:46:31 AM GMT+01:00, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
>
>
>> On 24 Dec 2017, at 10:39, Jonathan Morton <chromatix99@gmail.com>
>wrote:
>> 
>> If you combine the 'raw' keyword with an overhead spec, that disables
>the hard_header_len compensation.
>> 
>> - Jonathan Morton
>
>Indeed.  I think all I’m proving is ‘make something idiot
>proof…..they’ll only go and improve the idiot’… i.e. me :-)

Haha, funny ;). But with the current state of the cake correcting for hard_header_len, while this is not guaranteed to be the overhead the kernel adds to skb->Len; I would claim cake is not idiot proof, but rather has a very tricky default mode...
I do add that reporting max_len_after_overhead_accounting in addition to max_len would have made it much more likely to catch this behaviour much earlier...

Best Regards & happy holidays
        Sebastian



>
>
>Happy holiday season to all.  I’m off to find another trap to fall into
>;-)
>
>Cheers,
>
>Kevin D-B
>
>012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [Cake] overheads or rate calculation changed?

[Jonathan Morton]

> On 23 Dec, 2017, at 11:03 pm, Sebastian Moeller <moeller0@gmx.de> wrote:
> 
> just had a look for hard_header_len in the linux kernel:
> linux/include/linux/netdevice.h:
> *      @hard_header_len: Maximum hardware header length.
> *      @min_header_len:  Minimum hardware header length
> 
> this seems to corroborate our observation that hard_header_len is not a veridical representation of the actual hardware header length, so I assume the values cake returns are actually true. It also indicates that except for pure ethernet interfaces hard_header_len is _not_ the right parameter to evaluate for what cake is evaluating it for...

Turns out min_header_len is always either zero or 14, and is scarcely used anywhere.  It seems to be completely ignored by non-Ethernet interfaces.

However, it appears that the correct value is stored implicitly in each skb, and can be obtained through skb_network_offset(skb) - that's the offset from the beginning of the packet to the IP header (assuming it's an IP packet).  This suggests to me that the via-ethernet keyword can be retired, in favour of unconditionally subtracting that value from each packet length before applying overhead compensation, and setting the *default* overhead compensation to hard_header_len (to emulate the current default behaviour).

What we would lose that way is the present capability to add an overhead to the raw packet length as reported by Linux.  However, since that doesn't reliably correspond to an actual packet length on the wire, that's not really a useful capability to keep, except for direct comparison with other overhead compensation methods.

Comments?

 - Jonathan Morton

Re: [Cake] overheads or rate calculation changed?

[Sebastian Moeller]

Hi Jonathan,

> On Jan 6, 2018, at 21:44, Jonathan Morton <chromatix99@gmail.com> wrote:
> 
>> On 23 Dec, 2017, at 11:03 pm, Sebastian Moeller <moeller0@gmx.de> wrote:
>> 
>> just had a look for hard_header_len in the linux kernel:
>> linux/include/linux/netdevice.h:
>> *      @hard_header_len: Maximum hardware header length.
>> *      @min_header_len:  Minimum hardware header length
>> 
>> this seems to corroborate our observation that hard_header_len is not a veridical representation of the actual hardware header length, so I assume the values cake returns are actually true. It also indicates that except for pure ethernet interfaces hard_header_len is _not_ the right parameter to evaluate for what cake is evaluating it for...
> 
> Turns out min_header_len is always either zero or 14, and is scarcely used anywhere.  It seems to be completely ignored by non-Ethernet interfaces.

	Yepp, min_header_len also does not sound to offer the guarantees we want.

> 
> However, it appears that the correct value is stored implicitly in each skb, and can be obtained through skb_network_offset(skb) - that's the offset from the beginning of the packet to the IP header (assuming it's an IP packet).  

	That sounds great.

> This suggests to me that the via-ethernet keyword can be retired, in favour of unconditionally subtracting that value from each packet length before applying overhead compensation, and setting the *default* overhead compensation to hard_header_len (to emulate the current default behaviour).

	No, the current behaviour of cake is an outlier, no other qdisc does this:

qdisc cake 8009: dev pppoe-wan root refcnt 2 bandwidth 9545Kbit diffserv3 dual-srchost nat rtt 100.0ms noatm overhead 34 via-ethernet total_overhead 34 hard_header_len 26 mpu 64 
 Sent 673788150 bytes 5116677 pkt (dropped 535, overlimits 721725 requeues 0) 
 backlog 0b 0p requeues 0 
 memory used: 224192b of 4Mb
 capacity estimate: 9545Kbit
                   Bulk  Best Effort        Voice
  thresh      596560bit     9545Kbit     2386Kbit
  target         30.5ms        5.0ms        7.6ms
  interval      125.5ms      100.0ms      102.6ms
  pk_delay          0us        1.0ms        282us
  av_delay          0us         47us         20us
  sp_delay          0us          7us          9us
  pkts                0      5019757        97455
  bytes               0    655197066     19357299
  way_inds            0        81110            7
  way_miss            0       432498         1536
  way_cols            0            0            0
  drops               0          535            0
  marks               0           58            0
  ack_drop            0            0            0
  sp_flows            0            0            0
  bk_flows            0            1            0
  un_flows            0            0            0
  max_len             0         1492          576

hard_header_len is 26 which fits with the fact that the same packet will first see 8 bytes pppoe overhead added, then 4 byte vlan and 14 byte kernel-ethernet for a total of 26, but as you see from max_len the packet size indicates that the kernel auto-added nothing (MTU1500 - 8 Byte PPPoE overhead = 1492). 

On the ingress side (of the same interface!) I see:
qdisc cake 800a: dev ifb4pppoe-wan root refcnt 2 bandwidth 46246Kbit diffserv3 dual-dsthost nat ingress rtt 100.0ms noatm overhead 34 via-ethernet total_overhead 34 hard_header_len 14 mpu 64 
 Sent 17412911204 bytes 13164414 pkt (dropped 3720, overlimits 19196199 requeues 0) 
 backlog 0b 0p requeues 0 
 memory used: 490048b of 4Mb
 capacity estimate: 46246Kbit
                   Bulk  Best Effort        Voice
  thresh       2890Kbit    46246Kbit    11561Kbit
  target          6.3ms        5.0ms        5.0ms
  interval      101.3ms      100.0ms      100.0ms
  pk_delay          0us        458us        165us
  av_delay          0us         44us         22us
  sp_delay          0us         15us         14us
  pkts                0     13088522        79612
  bytes               0  17401576669     16506510
  way_inds            0        80004            0
  way_miss            0       416460           17
  way_cols            0           83            0
  drops               0         3720            0
  marks               0         1151            0
  ack_drop            0            0            0
  sp_flows            0            0            0
  bk_flows            0            1            0
  un_flows            0            0            0
  max_len             0         1492         1492

So for the ifb the kernel ignored PPPoE and vlan overhead, but again max_len tells us that the kernel did not add anything at all.

Traditional "tc stab" would in both cases have done the correct thing (albeit accidentally as the kernel simply did not "tamper" with skb->len) with the requested "overhead 34".
However, cake accidentally accounted for 8 bytes on egress and 20 bytes on ingress instead. This is less than ideal especially since the mis-accounting is different for the two directions of the same interface (disclaimer, sqm-scripts currently only allows to configure a single overhead value that is indiscriminately applied to both ingress and egress, this really does not work well different pre-adjustments performed on both directions; you could argue that this should be fixed in sqm-scripts ;) )

I thibk cake should offer a mode in which it behaves as all other qdiscs currently do and not do auto correction at all and a mode where it corrects for the right amount, but keeping the current ake cbehavior will not help anybody.... but most likely i misunderstood your proposal in that regard.


> 
> What we would lose that way is the present capability to add an overhead to the raw packet length as reported by Linux.

	That is what we could aim to keep (at least with the raw keyword or a new rawoverhead keyword); the least we need to do is keep reporting how many bytes were auto-adjusted (only after dumping hard_header_len in the output it became clear to me that cake's assumptions about skb-> hard_header_len only held true for plain ethernet interfaces). 
Or put differently, if we can run with the new mode you propose and keep reporting the amount of auto adjustment (like what is currently reported as hard_header_len, but with a more appropriate name) and see that it does the right thing, we could remove the old way...

Question: if the user does not specify an overhead at all (via keyword or explicitly as overhead NN) even the proposed new mode will not do anything but simply take skb->len? In that case maybe keeping the "raw" behaviour as cake currently does might not be required.

>  However, since that doesn't reliably correspond to an actual packet length on the wire, that's not really a useful capability to keep, except for direct comparison with other overhead compensation methods.

	Yes, we might keep that capability to easily compare the different methods, as long as the kernel has 3 different methods we might as well used them to "calibrate"/sanity-check each other ;) (but my thinking is more like if you figure out the correct solution for the accounting in cake, it might make sense to teach stab the same trick)

> 
> Comments?


For all the words above, I really only would like to see three things in regards to the auto overhead accounting:
1) report what cake does routinely in the output of tc -s qdisc (including the actual amount of the adjustment)

2) do the right thing, so if at all reliably possible correct for the non-IP packet part of skb->len (that is not directly caused by running atm/ptm 53/48 or 65/64 expansion)

3) allow the user some way to disable that auto-accounting (but I think only doing that if no overhead keyword of any kind was specified is finw with me, that way cake still stays compatible with the generic stab method)

Thanks for doing this great work!

Best Regards

> 
> - Jonathan Morton
> 

Re: [Cake] overheads or rate calculation changed?

[Jonathan Morton]

> On 7 Jan, 2018, at 12:46 am, Sebastian Moeller <moeller0@gmx.de> wrote:
> 
> I thibk cake should offer a mode in which it behaves as all other qdiscs currently do and not do auto correction at all and a mode where it corrects for the right amount, but keeping the current ake cbehavior will not help anybody.... but most likely i misunderstood your proposal in that regard.

Let me explain a little more clearly, using your example:

Here "by default" means with no overhead keywords used at all.  Currently, Cake does nothing with the packet lengths by default, acting as though "raw" was specified.

Instead, Cake will act as though "overhead 26" were specified on egress and "overhead 14" on ingress in your case.  Unlike its current behaviour, it will recognise that it's actually getting raw IP packets, and won't first attempt to subtract a non-existent header from them.  That's still different from tc-stab behaviour, but probably more useful.

Conversely, with a typical Ethernet interface, it will act as though "overhead 14" were specified on both egress and ingress, and will effectively leave the packet length alone, recognising that there is in fact an Ethernet frame header on the front of the packets handed in.

If you then specify "overhead 34", you'll get exactly that, relative to the transport-layer packet (that is, IP).

You'll get visibility into this behaviour through the output of the current configuration, which will include an overhead keyword.  And all this assumes that skb_network_offset(skb) *actually* does what I think it does.

What I'm not sure about yet is whether to keep "raw" and "via-ethernet" with their current meanings - as that would probably have a (small) runtime overhead as well as requiring more serious thought into how to organise the configuration interface.  Otherwise, I do intend to keep "raw" as a way to reset to the default configuration.

 - Jonathan Morton

Re: [Cake] overheads or rate calculation changed?

[Sebastian Moeller]

Hi Jonathan,

> On Jan 7, 2018, at 01:33, Jonathan Morton <chromatix99@gmail.com> wrote:
> 
>> On 7 Jan, 2018, at 12:46 am, Sebastian Moeller <moeller0@gmx.de> wrote:
>> 
>> I thibk cake should offer a mode in which it behaves as all other qdiscs currently do and not do auto correction at all and a mode where it corrects for the right amount, but keeping the current ake cbehavior will not help anybody.... but most likely i misunderstood your proposal in that regard.
> 
> Let me explain a little more clearly, using your example:
> 
> Here "by default" means with no overhead keywords used at all.  Currently, Cake does nothing with the packet lengths by default, acting as though "raw" was specified.

	Yes, I believe we should keep a mode that does exactly that, but this might be done with an explicit keyword IMHO. Not having that would require that cake always does the right thing and as the history of this feature shows, this is hard to guarantee.

> 
> Instead, Cake will act as though "overhead 26" were specified on egress and "overhead 14" on ingress in your case.  
> Unlike its current behaviour, it will recognise that it's actually getting raw IP packets, and won't first attempt to subtract a non-existent header from them.  That's still different from tc-stab behaviour, but probably more useful.

	Why behave like "overhead 26" was specified? The really desired behaviour seems to subtract (skb->len - IP packet size) and neither the 26 nor the 14 where actually added to skb->len. So I a still confused about what you propose cake to report.

BTW do you see a 100% correct way to get to (skb->len - IP packet size)?


> 
> Conversely, with a typical Ethernet interface, it will act as though "overhead 14" were specified on both egress and ingress, and will effectively leave the packet length alone, recognising that there is in fact an Ethernet frame header on the front of the packets handed in.

	Ah, I begin to understand my disconnect; this is not ideal behavior as with the overhead accounting we want to be able to "model" the bottleneck overhead and not only the shaped interface overhead; think shaping for an  IPoA link (IP over ATM) from a non-ATM router connected via ethernet to the ATM-modem, the 14 bytes kernel accounted overhead are neither required nor desired, if I specify overhead 10, the kernel ideally will pass (for a full MTU 1500 packet):
1500 (MTU) + 14 (kernel added MACs/ethertype) + 10 (specified cake overhead) - 14 (remove the kernel-added MACs/ethertype)

Now, if you only talk about the case that the user did not specify an overhead in the cake invocation, using the 1514 skb->len without further modifications, than we violently agree. We really need this "behave like all other qdiscs" mode so that cake can be used with "tc stab" exactly as awkward as all other qdiscs, it would be really unfortunate if cake would require different "tc stab" magic than say HTB+fq_codel.


> 
> If you then specify "overhead 34", you'll get exactly that, relative to the transport-layer packet (that is, IP).

	That is the best thing to do agreed, but from your description above I am not sure about the exact conditions you propose to do that.

> 
> You'll get visibility into this behaviour through the output of the current configuration, which will include an overhead keyword.  And all this assumes that skb_network_offset(skb) *actually* does what I think it does.

	The overhead keyword is nice in that it shows what the user requested, but we really NEED to report also how cake interpreted that request, so we NEED to explicitly report the auto correction value in addition to the requested overhead (and for ease of use we maybe should also report the amount of overhead cake added to skb->len (which equals (requested overhead - auto correction value)))

> 
> What I'm not sure about yet is whether to keep "raw"

	I am not that impressed by the overloading that raw already has (ATM, PTM, overhead...), but the functionality to not-do the auto accounting seems required (unless we guarantee that cake always does the correct thing; and for that we need to explicit;y report what cake "did" and what the user asked it to do).

> and "via-ethernet" with their current meanings -

	THe current via_ethernet behavior seems not requred anymore, ONCE we unconditionally report the amount of auto corrected overhead (which could well be 0).

> as that would probably have a (small) runtime overhead as well as requiring more serious thought into how to organise the configuration interface.  


> Otherwise, I do intend to keep "raw" as a way to reset to the default configuration.

	That is not what raw does ATM, for the via-ethernet mechanism raw is not the default (how a bout a new default compound keyword?)

Best Regards
	Sebastian

> 
> - Jonathan Morton
> 

Re: [Cake] overheads or rate calculation changed?

[Jonathan Morton]

> On 7 Jan, 2018, at 10:19 am, Sebastian Moeller <moeller0@gmx.de> wrote:
> 
> We really need this "behave like all other qdiscs" mode so that cake can be used with "tc stab" exactly as awkward as all other qdiscs, it would be really unfortunate if cake would require different "tc stab" magic than say HTB+fq_codel.

Okay, that's a valid use-case for not molesting the packet size at all.  I'll have to think about how best to interface that to userspace.

>> If you then specify "overhead 34", you'll get exactly that, relative to the transport-layer packet (that is, IP).
> 
> That is the best thing to do agreed, but from your description above I am not sure about the exact conditions you propose to do that.

It'll be what you get in all cases, *unless* you specifically invoke the stab-compatible behaviour.

 - Jonathan Morton