Simplify cubic_ack_received

[kmacy]

Summary

The logic remains unchanged (with one exception) but the branching is much easier to read.

The one exception is that - if it is possible to reach a state where min_rtt_ticks is zero but we are *not* in slowstart the ack will be processed whereas before it would have been ignored.

[hiren]

@kmacy, about the exception you noted, I am a little puzzled.
a state where min_rtt_ticks is zero but we are *not* in slowstart - could still be processed in the old code if other rfc3465 conditions align as the guarding 'if' looks like this:

(!V_tcp_do_rfc3465 ||
CCV(ccv, snd_cwnd) <= CCV(ccv, snd_ssthresh) || ​
(V_tcp_do_rfc3465 && ccv->flags & CCF_ABC_SENTAWND))

sys/netinet/cc/cc_cubic.c

147

Can we move this 'if' condition above the prev 'if' to keep the flow closer to that of the original file? or does it break the logic in a way I am not understanding?

[hiren]

@kmacy Can you please looks at my comments? I like the changes otherwise.

Anyone else want to weigh in?

[kmacy]

With regards to:

The one exception is that - if it is possible to reach a state where min_rtt_ticks is zero but we are *not* in slowstart the ack will be processed whereas before it would have been ignored.

I think this change is actually a functional improvement.

sys/netinet/cc/cc_cubic.c

147

It breaks the logic. We would be skipping newreno slow start handling.

[gnn]

LGTM

[rscheff]

Is this diff still alive? With all the cubic fixes that went in recently, this patch may need to be rewritten to apply cleanly.

Breaking complex conditionals up to ease the understanding seems a good thing to me.

[imp]

Tagging for srcmgr review... likely can just be closed though. This code has changed a lot and I'm not sure how to apply it.

[kbowling]

@imp if I recall this was something we were looking at on my team at LLNW, if it were in Bugzilla I'd say "Close - OBE" is appropriate.

@cc since you've been working on CUBIC lately, the only thing possibly still relevant here is turning the first level if into the early returns.. it removes one level of indention which might help justify a couple other nearby style(9) fixes but it's not worth holding this review open.

[cc]

In D7986#1184580, @kbowling wrote:

@imp if I recall this was something we were looking at on my team at LLNW, if it were in Bugzilla I'd say "Close - OBE" is appropriate.

@cc since you've been working on CUBIC lately, the only thing possibly still relevant here is turning the first level if into the early returns.. it removes one level of indention which might help justify a couple other nearby style(9) fixes but it's not worth holding this review open.

Agree. It's not worth holding this old review open. Close it looks appropriate.