tcp: Add hystart-plus to cc_newreno and rack.
ClosedPublic
Actions

Authored by rrs on Oct 8 2021, 4:03 PM.

Details

Reviewers

tuexen

Group Reviewers

transport

Commits

rG4e4c84f8d101: tcp: Add hystart-plus to cc_newreno and rack.

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.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

rrs created this revision.Oct 8 2021, 4:03 PM

Herald added 1 blocking reviewer(s): transport. · View Herald TranscriptOct 8 2021, 4:03 PM

Herald added a subscriber: melifaro. · View Herald Transcript

rrs requested review of this revision.Oct 8 2021, 4:03 PM

tuexen added inline comments.Oct 19 2021, 5:01 PM

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`?
64	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 added a subscriber: rscheff.Oct 19 2021, 5:48 PM

rscheff added inline comments.

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 marked 12 inline comments as done.Oct 20 2021, 6:18 AM

rrs added inline comments.

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

Address Michael's comments.

tuexen accepted this revision.Oct 20 2021, 8:01 AM

This revision is now accepted and ready to land.Oct 20 2021, 8:01 AM

Closed by commit rG4e4c84f8d101: tcp: Add hystart-plus to cc_newreno and rack. (authored by rrs). · Explain WhyOct 22 2021, 11:12 AM

This revision was automatically updated to reflect the committed changes.

rrs added a commit: rG4e4c84f8d101: tcp: Add hystart-plus to cc_newreno and rack..

Herald added a subscriber: imp. · View Herald TranscriptOct 22 2021, 11:12 AM

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.

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.

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

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.