Re: [Cake] overheads or rate calculation changed?

[Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk>]

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