tcp: Add hystart-plus to cc_newreno and rack.

[rrs]

Summary

TCP Hystart draft version -03:
https://datatracker.ietf.org/doc/html/draft-ietf-tcpm-hystartplusplus

Is a new version of hystart that allows one to carefully exit slow start if the RTT
spikes too much. The newer version has a slower-slow-start so to speak that then
kicks in for five round trips. To see if you exited too early, if not into congestion avoidance.
This commit will add that feature to our newreno CC and add the needed bits in rack to
be able to enable it.

Test Plan

I will be using the NF lab to test and validate that the hystart
works as planned via the BBlogs I will have it generate. I have
pretested an earlier version of this patch already but am just getting
setup to retest after a shuffle to get all of the variables and state
as newreno specific fields instead of ccv fields. I won't finish commiting
this patch until I have completely retested it as well as put it on some
live big-I traffic at NF to make sure it is performing as expected.

[tuexen]

sys/netinet/cc/cc_newreno.c

664

Can you use hystart++?

666

Use HyStart++ instead of hystart.

671

Use HyStart++ instead of hystart. Maybe "The number of MSS in the CWND before HyStart++ is active".

676

Use HyStart++ instead of hystart. Remove tailing ?.

681

Use HyStart++ instead of hystart. Remove tailing ?.

686

Use HyStart++ instead of hystart. Remove tailing ?.

691

Use HyStart++ instead of hystart. Maybe "The divisor to the growth when in HyStart++ CSS".

696

Use HyStart++ instead of hystart. Maybe "The number of round HyStart++ lasts in CSS before falling to CC".

701

Use HyStart++ instead of hystart.

sys/netinet/cc/cc_newreno.h

47

Maybe you can come up with a more descriptive name than nflags. Since newreno_flags is already taken, what about hystart_flags?

65

Maybe using a prefix of CC_NEWRENO_HYSTART_ instead of NF_HYSTART is more descriptive? NF triggers Netflix, when I read it...

sys/netinet/tcp.h

313

Maybe use TCP_RACK_ENABLE_HYSTART, since right now, this is RACK specific.

sys/netinet/tcp_stacks/rack.c

1398

I think the above changes do not belong into the patch adding HyStart++ support.

1401

hystart++ instead of hystart?

1403

Use HyStart++ instead of hystart.

20075

Maybe use TCP_RACK_ENABLE_HYSTART, since this is RACK specific right now?

[rscheff]

sys/netinet/cc/cc_newreno.c

664

the sysctl name doesn't like "++" and the compiler also interprets a trailing ++ special...

In ECN++ (D23230) I used verbatim "plusplus" as suffix to work around.

[rrs]

sys/netinet/cc/cc_newreno.c

664

I was wondering if hystart++ would work, I will do like you and
use hystartplusplus :)

sys/netinet/cc/cc_newreno.h

47

No for this one we will use the newreno_flags field and correct the
issue between beta_ecn and beta_ecn_enabled. Which requires changes
in rack.

sys/netinet/tcp_stacks/rack.c

1398

Hmm well its a matter of growing to having too many features and the
need to have the features in one place. While yes, its not specific to
the hystart feature, it is specific to adding a feature. And adding one more
has made feature enabling/disabling too cumbersome to be mixed in
with misc. Since hystart is growing this feature then it makes sense
to me that the rearrangement should be here.

1401

Has Richard Noted it will have to be hystartplusplus

[rrs]

Address Michael's comments.

[lstewart]

I haven't finished reviewing yet but some initial thoughts in addition to my inline comments:

• Nit: please submit diffs with full context as it makes reviewing easier
• You need to address how enabling Hystart++ when using cc_newreno with the default FreeBSD stack will work
• The implementation pessimises the common case where newreno will be used without ABE or Hystart++ which is probably undesirable. The module's lack of a cb_init() hook implementation was very deliberate to avoid such pessimisation and also as noted inline, assumptions are made elsewhere that cc_newreno can always be used as an algo of last resort so we cannot allow init to ever fail.

sys/netinet/cc/cc.h

167

The CCF_ABC_SENTAWND flag passed in the ccv already provides an appropriate "round" marker which I suspect should be used instead of introducing a new hook and associated round tracking machinery.

173

I don't think this new hook is needed given that everything it achieves could be handled in the ack_received hook?

Also, requiring flight at send presumes a level of state tracking which is not performed by the FreeBSD default stack. I don't think the implementation should preclude the default stack being able to use it.

sys/netinet/cc/cc_newreno.c

178

cc_newreno is the "algo of last resort" and is not allowed to fail initialisation.

[rrs]

In D32373#735881, @lstewart wrote:

I haven't finished reviewing yet but some initial thoughts in addition to my inline comments:

• Nit: please submit diffs with full context as it makes reviewing easier
• You need to address how enabling Hystart++ when using cc_newreno with the default FreeBSD stack will work
• The implementation pessimises the common case where newreno will be used without ABE or Hystart++ which is probably undesirable. The module's lack of a cb_init() hook implementation was very deliberate to avoid such pessimisation and also as noted inline, assumptions are made elsewhere that cc_newreno can always be used as an algo of last resort so we cannot allow init to ever fail.

Lawrence:

Some comments to your "late" review, after the item was closed.

1. First this item was opened Oct 8th and closed 2 weeks later after addressing all comments.

2. Comments put in after its closed I think are rather pointless but let me address what you put in: a) The only way I know to do this in the new git world order is git diff, and I don't know what options you can use to make git be more chatty. In the svn world there was a -x -up option that was suggested to me that I used, those do not work unfortunately. Let me know what options git diff will accept since that seems to be the tool for our new world order here. b) We do not have a CC fallback mechanism. If you set cc_default to say cc_cubic and it fails then the connection fails period. Making new-reno do similar is no different. If you want a cc of last resort then tcp_subr will need changes and we can create some cc_last non-module like we have in the freebsd stack. But thats way beyond the scope of this work. c) The SENT_AWND is only active when cwnd > ssthresh, which is not anywhere applicable when you are in slow-start so there is no round mechanism in the current cc. So no your ack-received cannot work as you state. d) If you pass in cwnd in-lieu of flight at send everything works as was before, this is what older version of rack at NF and also the freebsd stack here does. Its just you get a better more accurate result if you use fas.