Page MenuHomeFreeBSD
Differential D43986

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

Authored by rrs on Feb 20 2024, 3:47 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 23, 4:08 PM
Unknown Object (File)
Mon, Mar 23, 6:25 AM
Unknown Object (File)
Mon, Mar 23, 6:18 AM
Unknown Object (File)
Sun, Mar 22, 6:54 PM
Unknown Object (File)
Wed, Mar 18, 1:36 PM
Unknown Object (File)
Wed, Mar 18, 10:49 AM
Unknown Object (File)
Wed, Mar 18, 5:20 AM
Unknown Object (File)
Wed, Mar 18, 5:17 AM
View All Files
Subscribers
des
glebius
imp
melifaro

Details

Reviewers
tuexen
rscheff
glebius
rrs
Group Reviewers
transport
Commits
rGe18b97bd63a8: Update to bring the rack stack with all its fixes in.
rGc112243f6bd3: Revert "Update to bring the rack stack with all its fixes in."
rGf6d489f402c3: Update to bring the rack stack with all its fixes in.
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

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

rrs created this revision.Feb 20 2024, 3:47 PM
Herald added 1 blocking reviewer(s): transport. · View Herald TranscriptFeb 20 2024, 3:47 PM
Herald added subscribers: glebius, melifaro. · View Herald Transcript
rrs requested review of this revision.Feb 20 2024, 3:47 PM
tuexen added a comment.Feb 20 2024, 4:12 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
rrs updated this revision to Diff 134741.Feb 20 2024, 5:44 PM

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

rrs added a comment.Feb 20 2024, 5:46 PM
In D43986#1003451, @tuexen 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.

rrs added a comment.Feb 20 2024, 5:50 PM
In D43986#1003519, @rrs wrote:
In D43986#1003451, @tuexen 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?

tuexen added a comment.Feb 20 2024, 6:01 PM
In D43986#1003524, @rrs wrote:
In D43986#1003519, @rrs wrote:
In D43986#1003451, @tuexen 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.

rrs updated this revision to Diff 135008.Feb 26 2024, 2:25 PM

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

rrs accepted this revision.Feb 28 2024, 6:56 PM
glebius added inline comments.Feb 28 2024, 10:14 PM
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!

rrs added a comment.Feb 29 2024, 3:10 PM

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 :)

rrs updated this revision to Diff 135223.Feb 29 2024, 4:05 PM

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 :)

rrs added a comment.Feb 29 2024, 4:07 PM

The hpts change is now in https://reviews.freebsd.org/D44157

rrs updated this revision to Diff 135228.Feb 29 2024, 4:31 PM

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.

tuexen accepted this revision.Mar 10 2024, 7:31 AM
rrs accepted this revision.Mar 11 2024, 3:46 PM
des added a commit: rGf6d489f402c3: Update to bring the rack stack with all its fixes in..Mar 11 2024, 7:36 PM
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.

brooks added a reverting change: rGc112243f6bd3: Revert "Update to bring the rack stack with all its fixes in.".Mar 11 2024, 8:29 PM
des added a commit: rGc112243f6bd3: Revert "Update to bring the rack stack with all its fixes in.".Mar 11 2024, 8:31 PM
This revision was not accepted when it landed; it landed in state Needs Review.Mar 15 2024, 5:15 PM
Closed by commit rGf6d489f402c3: Update to bring the rack stack with all its fixes in. (authored by rrs). · Explain Why
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: imp. · View Herald TranscriptMar 15 2024, 5:15 PM
rrs added a commit: rGe18b97bd63a8: Update to bring the rack stack with all its fixes in..Mar 16 2024, 5:28 PM

Revision Contents
Changeset List

PathSize
sys/
modules/
tcp/
rack/
2 lines
netinet/
38 lines
9 lines
tcp_stacks/
4 lines
4443 lines
5 lines
8 lines
33 lines
135 lines
57 lines
5 lines
7 lines
12 lines

Diff 135228

View Options

sys/modules/tcp/rack/Makefile

Loading...
View Options

sys/netinet/tcp.h

Loading...
View Options

sys/netinet/tcp_log_buf.h

Loading...
View Options

sys/netinet/tcp_stacks/bbr.c

Loading...
View Options

sys/netinet/tcp_stacks/rack.c

Loading...
View Options

sys/netinet/tcp_stacks/sack_filter.h

Loading...
View Options

sys/netinet/tcp_stacks/tailq_hash.h

Loading...
View Options

sys/netinet/tcp_stacks/tailq_hash.c

Loading...
View Options

sys/netinet/tcp_stacks/tcp_rack.h

Loading...
View Options

sys/netinet/tcp_subr.c

Loading...
View Options

sys/netinet/tcp_syncache.c

Loading...
View Options

sys/netinet/tcp_usrreq.c

Loading...
View Options

sys/netinet/tcp_var.h

Loading...