> On 23 Apr 2018, at 11:50, Jonathan Morton <chromatix99@gmail.com> wrote:
> 
>> Right. Pushed the patch to the cobalt branch, feel free to fix it up :)
> 
> Given that I basically had to revert 80% of it and start again, perhaps not the best policy.

Ha :-)

cobalt_should_drop(&flow->cvars, &b->cparams, now, skb,
                        b->bulk_flow_count * !!(q->rate_flags & CAKE_FLAG_INGRESS)

I like the conditional multiply (by 1) based on ingress mode, so we only pass a non zero flow count in ingress mode only.  Saves passing an additional parameter which I disliked but could see no way around.


I’m not sure that the replacement code in cobalt_should_drop itself is an equivalent of what Toke tested in his many combinations:

        bool over_target = sojourn > p->target && (
                           sojourn > p->mtu_time * bulk_flows * 2 ||
                           sojourn > p->mtu_time * 4 );

So in egress mode the first part of the OR statement will always be true (p->mtu_time * bulk_flows (0) * 2) is always 0, I’d like to think sojourn will always be bigger than 0.  So at run time it simplifies to

bool over_target = sojourn > p->target


ingress mode is more interesting since bulk_flows is now non zero.

sojourn > p->mtu_time * bulk_flows * 2 (I think in essence this is ‘permit up to 2 outstanding MTU packets(time) per flow overall) - this wasn’t tested
OR
sojourn > p->mtu_time * 4 ); (up to 4 MTU packets(time) on this flow only)   - this I believe was tested.

and this particular combination of the two wasn’t tested.   I wonder if I could politely ask Toke to run his test on this version.

I’m arguing/commenting from a position of ignorance and a double migraine yesterday, so to say I’m foggy today is an understatement!


Cheers,

Kevin D-B

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