tcp: Add hystart++ to our cubic implementation.

[rrs]

Summary

As promised to the transport call on 11/4/22 here is an implementation
of hystart++ for cubic.

Now question for reviewers, I have taken the sysctl's and raised them to cc.c level, I am
wondering if the state should also go there so we can commonize the code (a lot
of it was just duplicated from cc_newreno.c which begs for a common set of
fuctions). But for a first cut I wanted to put this out and get thoughts??

Test Plan

Setup to use cubic and then test in various lab scenarios with BB logging on to make
sure we see us going into both CSS and out without loss.

[rrs]

Note missing yet from this diff is the ctl_output() function so you can actually enable it.

[rrs]

Ok this straightens out the allowed flag to be up at the ccv->flags
level and gets rid of the need for a socket option for the specific
allowing of hystart++ on the CC module. Instead if the flag gets
set any CC module that supports hystart++ can then use it.

[rscheff]

sys/netinet/tcp_stacks/rack.c

12857

another magic number here...

20728

Can we not have hard coded magic numbers for optval?

[rrs]

With a bit of testing I figured out I had missed a few things. Now this version
works per the draft. Note that I may have found a problem with the draft though.
Am now waiting on an answer from Praveen.

[rrs]

sys/netinet/tcp_stacks/rack.c

12857

sure I can get these turned into defines....

20728

sure I can do that ;)

[rrs]

Fix Richards magic numbers.

[rrs]

Richard:

One thing that is interesting to me about cubic is that it has this flag
"in slow start" that is added to the flags field, but I never see that
flag be removed.. i.e. when we enter CA.

I am not sure if the flag is old and no longer paid attention too or
what.. but it seems a bit strange to be in CA and still see the
"in slow start" flag on.

Maybe you have some idea how it used to be used??

[rrs]

Ahh I see if you get a "RTO event" or an "Application limited" event you
remove the flag. But what about NDUPACK.. (where a loss is taking you
out of SS).. and for that matter maybe we should also when we leave
CSS to CA remove this flag...

What do you think Richard?

[rrs]

Update the diff to take into account Neal Cardwell's comments on tcpm.. i.e.
make the comparison for rounds SEQ_GT not SEQ_GEQ.

[lstewart]

I won't have the time to review this in the near future but would note that many of the things I flagged for the original newreno hystart++ implementation also apply to this implementation. However, I wouldn't call any of them blockers so don't have an objection to you committing this at your discretion.

[rrs]

Forgot to update the manual pages as well since the mib variables
are now at the CC level not the lower one. Also make sure the internet draft
version is sited in the man.

[rrs]

Let;s expand the GT to GT or equal if snd_max is above snd_una. This makes
sure we are not application limited.

Let's also fix the oscillation I found between CSS and out. I am not, as yet, addressing
the app limited having us come out of CSS..

[tuexen]

I would suggest to use the spelling HyStart++ in man-pages and comments consistently as in draft-ietf-tcpm-hystartplusplus.

share/man/man4/mod_cc.4

88 ↗

(On Diff #100078)

I guess changing control to coyntrol was not intended.

[rrs]

Fix the inadvertent change Michael noted in the man, as well
as follow the draft but add my observations in the comment about
it causing cycling in and out of CSS

[rrs]

Update to draft-04

[rscheff]

In D33035#746791, @rrs wrote:

Ahh I see if you get a "RTO event" or an "Application limited" event you
remove the flag. But what about NDUPACK.. (where a loss is taking you
out of SS).. and for that matter maybe we should also when we leave
CSS to CA remove this flag...

What do you think Richard?

Not quite sure what you mean; the IN_SLOWSTART is removed in CongAvoidance effectively unconditionally (well, the condition is, that we entered CA (cwnd > ssthresh), and that the flag was set.

It got added to track the best point when the cubic epoch (t_last_cong) is initialized, as we found that continously tracking /updating while still in slow start is not good enough, if you transition directly from SS to AppLimited - the app limited event can be delayed such that an "older" t_last_cong results in unexpectedly large cwnd adjustements by cubic...

Hope this explanation makes sense.

(The clearing is two flags, spread out across two lines:

if (CCV(ccv, snd_cwnd) <= CCV(ccv, snd_ssthresh) ||
    cubic_data->min_rtt_ticks == TCPTV_SRTTBASE) {
        cubic_data->flags |= CUBICFLAG_IN_SLOWSTART;  # < set while sending data with cwnd < ssthresh -> SS
        newreno_cc_ack_received(ccv, type);
} else {
        if ((cubic_data->flags & CUBICFLAG_RTO_EVENT) &&
            (cubic_data->flags & CUBICFLAG_IN_SLOWSTART)) {
                /* RFC8312 Section 4.7 */
                cubic_data->flags &= ~(CUBICFLAG_RTO_EVENT |
                                       CUBICFLAG_IN_SLOWSTART);             # < cleared is we leave SS due to an RTO event, with different actions
                cubic_data->max_cwnd = CCV(ccv, snd_cwnd);
                cubic_data->K = 0;
        } else if (cubic_data->flags & (CUBICFLAG_IN_SLOWSTART |
                                 CUBICFLAG_IN_APPLIMIT)) {                           # < if either flag is set
                cubic_data->flags &= ~(CUBICFLAG_IN_SLOWSTART |
                                       CUBICFLAG_IN_APPLIMIT);                       # < we clear it here - so cleared whenever cwnd > ssthresh.
                cubic_data->t_last_cong = ticks;
                cubic_data->K = cubic_k(cubic_data->max_cwnd /
                                        CCV(ccv, t_maxseg));
        }

[cc]

sys/netinet/cc/cc_cubic.c

174

Like a mss is used in cubic_cong_signal(), and as ABC is involved, for accurate calculation, suggest to use "u_int mss = tcp_maxseg(ccv->ccvc.tcp);", and use mss instead of t_maxseg below.

sys/netinet/cc/cc_cubic.h

96–101

Can use uint32_t, as snd_cwnd uses that now.