I agree with Peter :)

old code :)

Are you sure this condition cannot be reached?

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.