In D47925#1095113, @glebius wrote:

Later I'd probably bring the input and output to same KBI, so that both return same signedness error for unlocked return. But this requires a sweeping change over stacks, so later.

Ok after a detailed analysis:

I agree with Michael here, macros badly hide things.. which is their intention :)

Fix what I thought I had fixed i.e. Unused_71 .. but for some reason I missed it ⭕

go through all the enum's and mark UNUSED those that are not used.

I am fine with this Drew, just went through and verified the departure code running earlier should have no impact...

This changes the enum, as discussed on a previous conf call to be "UNUSED" still taking the slot. I don't necessarily
agree with that but if everyone else wants it I am fine with it :)

Take out inadvertent space adds.

This gets rid of the defines mentioned by Michael in comments and as prescribed by Gleb on the last TCP conf call i.e.
we comment them out like in other code.

rcv-ack-finwait-1-ipv6 PASSED
rcv-fin-finwait-1-ipv4 PASSED
rcv-fin-finwait-1-ipv6 PASSED
rcv-fin-finwait-2-ipv4 PASSED
rcv-fin-finwait-2-ipv6 PASSED
rcv-ack-closing-ipv4 PASSED
rcv-ack-closing-ipv6 PASSED
rcv-ack-last-ack-ipv4 PASSED
rcv-ack-last-ack-ipv6 PASSED
rcv-ack-timewait-ipv4 PASSED
rcv-ack-timewait-ipv6 PASSED
rcv-fin-timewait-ipv4 PASSED
rcv-fin-timewait-ipv6 PASSED
rcv-syn-timewait-ipv4 PASSED

rcv-syn-timewait-ipv6 PASSED

Summary: Number of tests run: 881

Number of tests passed:                 849
Number of tests failed (expected):       32
Number of tests failed (unexpected):      0
Number of tests timed out (expected):     0
Number of tests timed out (unexpected):   0
Number of tests skipped:                  0

Committed it: fce03f85c5bf

Address all of Richards comments including the one I had to think about it was correct :)

One more update, need to use the tcp function call not t_maxseg - 12 to get the maxseg for filtering purposes

Add detailed comments so it is more clear what is going on inside sack_filter.h

One minor update my user space test did not show a new unused var that my kernel compile shows up. Fix that and
update the test information since I now validated the kernel version as well.

Updated the diff to fix some corner cases and to better handle compile and testing in user space.

Why are you removing the data after close checks in BBR and Rack. This is not related to getting a FIN (where you may
still be open). The point of that is so that if we have closed after dumping data into the SB, and then the peer sends in
a request (not realizing we have closed yet) to NOT send a RST until after all the data is through. This was useful for
NF since often times Nginx will close() a connection after filling the N requests and finishing dumping data into the sb.
It say has 1Meg in the buffer when it does close().

Add Drew's read_frequently compiler directive.

Update to Gleb and I's agreed upon reduction at the if in system.h

In D44420#1015031, @glebius wrote:

My bad, didn't understood proposed change. Of course if pointer stays stable since module load, there is no race.

In D44420#1014931, @glebius wrote:

In D44420#1014587, @rrs wrote:

Why not just use

if (hpts_that_need_softclock > 0)

tcp_hpts_softclock()\

?

But that would create the same race you mentioned above, wouldn't it? IMHO, using function pointer variable is the way to go and it potentially has a chance to get reduced to fewer instructions if compiler gets smarter.

Another issue we will need to address is multiple hpts threads trying to set the tcp_hpts_softclock pointer. One can envision
a case where you have one that is just lowering it to zero, and so goes into the "set" to NULL block. And another thread that
is going from 0 -> 1 and so goes into the set to function block.

Why not just use

In D44420#1013898, @glebius wrote:

Why can't we just clear and set the function pointer itself? That would reduce impact on userret() down to just one instruction.

I do believe the extern is not needed here since systm.h is included.

Adopt Drew's optimization suggestion and reverted back the sysctl Michael pointed out since the arg is unused (and here I thought
all these years it was the default).

Adopt Michael's suggestion to re-use one of the available bits for the flag.

Updated to use an atomic instead of the u64_counter as Gleb suggests.

and yeah Drew we could add a block with sysctl

I can easily change to an atomic.. I did not realize the fetch was expensive :)

Add in the additional MPASS as Gleb suggested.

Good catch Gleb, I missed the hpts.c changes.. I will update it :=)

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.

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

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

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

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