Page MenuHomeFreeBSD

Update to bring the rack stack with all its fixes in.
ClosedPublic

Authored by rrs on Feb 20 2024, 3:47 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 6, 3:14 PM
Unknown Object (File)
Wed, Nov 6, 3:14 PM
Unknown Object (File)
Wed, Nov 6, 3:14 PM
Unknown Object (File)
Tue, Nov 5, 3:09 AM
Unknown Object (File)
Tue, Oct 22, 5:33 AM
Unknown Object (File)
Oct 4 2024, 3:17 AM
Unknown Object (File)
Sep 30 2024, 4:32 PM
Unknown Object (File)
Sep 30 2024, 4:27 PM

Details

Summary

This brings the rack stack up to the current level used at NF. Many fixes
and improvements have been added. I also add in a fix to BBR to deal with
the changes that have been in hpts for a while i.e. only one call no matter
if mbuf queue or tcp_output.

Note there is a new file that I can't figure out how to get in rack_pcm.c

It basically does little except BBlogs and is a placemark for future work on
doing path capacity measurements.

Test Plan

run the rack stack after fixes on my machine.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

rrs requested review of this revision.Feb 20 2024, 3:47 PM

Did you try git add tcp_pcm.c? Please note that this week is stabilization week, meaning only bug fixes should go in...

glebius requested changes to this revision.Feb 20 2024, 5:21 PM
glebius added inline comments.
sys/netinet/tcp_stacks/rack_bbr_common.c
421 ↗(On Diff #134732)

This definitely doesn't look correct to me, cause it was just removed in f30c7d56546b.

This revision now requires changes to proceed.Feb 20 2024, 5:21 PM

Fix a bug I found in testing (and I think Gleb just pointed out)

Did you try git add tcp_pcm.c? Please note that this week is stabilization week, meaning only bug fixes should go in...

No I did not, but once I do that will it show up in the diff?

I will give it a try and see.

In D43986#1003519, @rrs wrote:

Did you try git add tcp_pcm.c? Please note that this week is stabilization week, meaning only bug fixes should go in...

No I did not, but once I do that will it show up in the diff?

I will give it a try and see.

Ok I tried it and it now shows rack_pcm.c as green (changes to be committed) and a git diff does not include that
it only shows the other files... unless there is some arg I am missing to have it show the "new file" under changes to be committed :(

Normally I have always just dit git diff -U999999 > xx

to get everything that is red (unstaged). maybe there is an arg I need?

In D43986#1003524, @rrs wrote:
In D43986#1003519, @rrs wrote:

Did you try git add tcp_pcm.c? Please note that this week is stabilization week, meaning only bug fixes should go in...

No I did not, but once I do that will it show up in the diff?

I will give it a try and see.

Ok I tried it and it now shows rack_pcm.c as green (changes to be committed) and a git diff does not include that
it only shows the other files... unless there is some arg I am missing to have it show the "new file" under changes to be committed :(

Normally I have always just dit git diff -U999999 > xx

to get everything that is red (unstaged). maybe there is an arg I need?

I think you actually need git add -N . Then the new should show up in the diff.

Ok update to after the sync. Also I forgot to add in the inherit calls in the syncache and the tcp_usr_attach

sys/netinet/tcp_hpts.h
116–121 ↗(On Diff #135008)

Can you please commit this change separately from the bulk update, so that git log/blame explains why. The original message here was:

Ok lets fix up the tcp_in_hpts() so that it also says yes if you
are in the race state moving and you are scheduled to be put in.
This also requires changing the MPASS to be the old version non
inline function of tcp_in_hpts().

This changed the tcp_in_hpts() and one assertion in tcp_hpts_insert_diag(). And that change to tcp_hpts_insert_diag() is missing in this review!

I still need to re-test this after the last sync I did.. will do that and then split out the hpts change and update the diff before landing..

sys/netinet/tcp_hpts.h
116–121 ↗(On Diff #135008)

Yeah thats a good idea. I will pull this out of this diff and save it for a subsequent commit. I will
clip out the above and use that as the commit message :)

This separates out the hpts fix. Will add that as a new review after I drop this in. I have
tested the combined hpts fix and this with the current head and everything works as
expected :)

So in testing I found a slight bug in that we were on the connect() side setting up the PCM max seg to
early. This means we end up with 10 x 512 not 10 x mss. Lets fix it so just like the listening side things get
deferred until we have gotten to the established state.

des added a subscriber: des.

This review was committed while still incomplete and broke the LINT build. Please provide the contents of netinet/tcp_stacks/rack_pcm.c asap so I can fix it.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 15 2024, 5:15 PM
This revision was automatically updated to reflect the committed changes.