Page MenuHomeFreeBSD
Differential D45410

Non-tested experimental code removal.
ClosedPublic
Actions

Authored by rrs on May 30 2024, 11:04 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 24, 6:08 PM
Unknown Object (File)
Fri, Mar 20, 5:09 AM
Unknown Object (File)
Fri, Mar 20, 5:04 AM
Unknown Object (File)
Thu, Mar 19, 7:44 AM
Unknown Object (File)
Thu, Mar 19, 7:38 AM
Unknown Object (File)
Thu, Mar 19, 7:38 AM
Unknown Object (File)
Thu, Mar 19, 7:38 AM
Unknown Object (File)
Thu, Mar 19, 6:55 AM
View All Files
Subscribers
glebius
imp
melifaro
tuexen

Details

Reviewers
tuexen
rscheff
rrs
Group Reviewers
transport
Commits
rG872164f559d2: Non-tested experimental code removal.
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

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

rrs created this revision.May 30 2024, 11:04 AM
Herald added a reviewer: transport. · View Herald TranscriptMay 30 2024, 11:04 AM
Herald added subscribers: glebius, melifaro. · View Herald Transcript
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
tuexen added a subscriber: tuexen.May 30 2024, 1:14 PM

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

rrs added reviewers: tuexen, rscheff.May 31 2024, 11:42 AM
rrs updated this revision to Diff 139843.Jun 14 2024, 8:59 AM

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.

tuexen added inline comments.Jun 21 2024, 5:11 AM
sys/netinet/tcp_log_buf.h
271

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
11999

Why do you add an empty line?

sys/netinet/tcp_stacks/tcp_rack.h
1

Why do you add an empty line?

rrs added inline comments.Jun 27 2024, 10:26 AM
sys/netinet/tcp_log_buf.h
271

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
11999

Inadvertent ... I will update this out :)

sys/netinet/tcp_stacks/tcp_rack.h
1

ditto

rrs updated this revision to Diff 140303.Jun 27 2024, 10:27 AM

Take out inadvertent space adds.

tuexen added inline comments.Jun 27 2024, 11:50 AM
sys/netinet/tcp_log_buf.h
271

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

rrs updated this revision to Diff 141435.Jul 26 2024, 11:12 AM

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

rrs accepted this revision.Jul 30 2024, 6:10 PM
This revision is now accepted and ready to land.Jul 30 2024, 6:10 PM
rrs updated this revision to Diff 141915.Aug 8 2024, 2:43 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
tuexen added a comment.Aug 8 2024, 3:57 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
269–270

Please use TCP_UNUSED_71 instead of TCP_UNUSED_1 for consistency.

rrs updated this revision to Diff 141921.Aug 8 2024, 5:07 PM

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

tuexen accepted this revision.Aug 8 2024, 5:27 PM
This revision is now accepted and ready to land.Aug 8 2024, 5:27 PM
Closed by commit rG872164f559d2: Non-tested experimental code removal. (authored by rrs). · Explain WhyAug 9 2024, 1:03 PM
This revision was automatically updated to reflect the committed changes.
rrs added a commit: rG872164f559d2: Non-tested experimental code removal..
Herald added a subscriber: imp. · View Herald TranscriptAug 9 2024, 1:03 PM

Revision Contents
Changeset List

PathSize
sys/
netinet/
5 lines
4 lines
tcp_stacks/
1048 lines
25 lines

Diff 139843

View Options

sys/netinet/tcp.h

Loading...
View Options

sys/netinet/tcp_log_buf.h

Loading...
View Options

sys/netinet/tcp_stacks/rack.c

Loading...
View Options

sys/netinet/tcp_stacks/tcp_rack.h

Loading...