Page MenuHomeFreeBSD

tcp: Add hystart++ to our cubic implementation.
Needs ReviewPublic

Authored by rrs on Wed, Nov 17, 4:46 PM.

Details

Reviewers
tuexen
rscheff
lstewart
Group Reviewers
transport
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.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

rrs requested review of this revision.Wed, Nov 17, 4:46 PM

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

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.

sys/netinet/tcp_stacks/rack.c
12856

another magic number here...

20727

Can we not have hard coded magic numbers for optval?

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.

sys/netinet/tcp_stacks/rack.c
12856

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

20727

sure I can do that ;)

Fix Richards magic numbers.

rrs marked 2 inline comments as done.Fri, Nov 19, 2:38 PM

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??

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?

This revision is now accepted and ready to land.Tue, Nov 30, 2:24 PM

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.

This revision now requires review to proceed.Tue, Nov 30, 2:40 PM

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.