Page MenuHomeFreeBSD

Non-tested experimental code removal.
ClosedPublic

Authored by rrs on May 30 2024, 11:04 AM.
Tags
None
Referenced Files
F102586848: D45410.id141945.diff
Thu, Nov 14, 10:40 AM
Unknown Object (File)
Mon, Nov 11, 6:07 AM
Unknown Object (File)
Sun, Nov 10, 4:23 PM
Unknown Object (File)
Wed, Nov 6, 6:28 PM
Unknown Object (File)
Wed, Oct 30, 2:19 AM
Unknown Object (File)
Tue, Oct 22, 5:13 PM
Unknown Object (File)
Mon, Oct 21, 10:18 AM
Unknown Object (File)
Mon, Oct 21, 10:18 AM

Details

Summary

There is a new feature that came in with the last sync to the rack stack that should not have
been released. It is untested and may not well work. It currently is off by default, which is good
but it is best to remove it until such time that it can be vetted and tuned to actually work :)

This change removes just the experimental feature for now. It can make a appearance in the future
when it is proofed out.

Test Plan

Run the tcp-test suite and make sure we have not taken out too much. It should all pass.

Diff Detail

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

Event Timeline

rrs requested review of this revision.May 30 2024, 11:04 AM

\

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

Can't we also remove TCP_POLICER_DET from sys/netinet/tcp_log_buf.h:270?
Can't we also remove TCP_POLICER_DETECT from /sys/netinet/tcp.h:340 and keeping a comment there that it was used?
I'm going to remove unused defines there in the not so distant future...

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.

sys/netinet/tcp_log_buf.h
270

Since this is an enum: do we want to keep some dummy item like TCP_LOG_UNUSED_71 or so to avoid that later log events get reassigned?

sys/netinet/tcp_stacks/rack.c
12035–12036

Why do you add an empty line?

sys/netinet/tcp_stacks/tcp_rack.h
-23

Why do you add an empty line?

sys/netinet/tcp_log_buf.h
270

I thought about that, but is not the purpose of an ENUM so that you don't care what its number is?

If we do care, then why not just change these to defines?

We for sure can add a "NOT_USED" but it seems to me counter to the purpose of an ENUM.....

sys/netinet/tcp_stacks/rack.c
12035–12036

Inadvertent ... I will update this out :)

sys/netinet/tcp_stacks/tcp_rack.h
-23

ditto

Take out inadvertent space adds.

sys/netinet/tcp_log_buf.h
270

I'll bring this up at the transport call...

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

This revision is now accepted and ready to land.Jul 30 2024, 6:10 PM

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

This revision now requires review to proceed.Aug 8 2024, 2:43 PM

If you address both minor issues above, I'm fine with the patch.

sys/netinet/tcp.h
340

We need to get in D46244 first. That removes the usage of TCP_POLICER_DETECT in tcpsso. Please review...

sys/netinet/tcp_log_buf.h
270

Please use TCP_UNUSED_71 instead of TCP_UNUSED_1 for consistency.

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

This revision is now accepted and ready to land.Aug 8 2024, 5:27 PM
This revision was automatically updated to reflect the committed changes.