Skip to content

gnrc_tcp: experimental feature "dynamic msl"#16764

Merged
jeandudey merged 1 commit intoRIOT-OS:masterfrom
brummer-simon:gnrc_tcp-experimental_dynamic_MSL
Sep 13, 2021
Merged

gnrc_tcp: experimental feature "dynamic msl"#16764
jeandudey merged 1 commit intoRIOT-OS:masterfrom
brummer-simon:gnrc_tcp-experimental_dynamic_MSL

Conversation

@brummer-simon
Copy link
Member

@brummer-simon brummer-simon commented Aug 21, 2021

Contribution description

This PR adds an experimental feature to gnrc_tcp called "dynamic MSL".
It calculates the otherwise static MSL value based on the last calculated re-transmission timeout value times
a configurable constant factor. This leads to a much shorter MSL causing gnrc_tcp_close to return faster if the TCP teardown sequence causes GNRC_TCP to wait in state TIME-WAIT.

Although this feature is a deviation from the TCP Standard, it should not cause any Issues. If a packet is received after the shorter TIME-WAIT timer times out, a reset packet is sent in return.

Testing procedure

This PR is not really testable in a deterministic way. It depends on heavily the scheduling of the
test runner and the packet transmission order between host system and riot node.
I enabled this feature in the gnrc_tcp test suite located under tests/gnrc_tcp.
If you run the test suite tests multiple times and no test takes longer than a minute, chances are good that its working.

Issues/PRs references

#16494 contains a long explanation regarding MSL length and the core mechanism this PR allows to tweak.

@github-actions github-actions bot added Area: Kconfig Area: Kconfig integration Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework labels Aug 21, 2021
@brummer-simon
Copy link
Member Author

@benpicco : You could use this in your telnet application to speedup gnrc_tcp_close. Using this causes gnrc_tcp_close to wait 4 seconds in the best case since the rto used in the calculation has a minimal value of 1s.

Copy link
Contributor

@jeandudey jeandudey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

However, would want some input from @miri64 on this first :-)

@benpicco
Copy link
Contributor

benpicco commented Sep 6, 2021

I can confirm that with this I only have to wait a few seconds before I can re-connect to my telnet server.

Still I don't really like that concept of 'broken defaults'. What speaks against enabling this always?
It is certainly surprising that a connection still lingers around after sock_tcp_disconnect() and you can't accept new connections for a couple minutes. To me this is a bug.

Just imagine a HTTP server that also serves an image - you would have to sit there waiting several minutes for the image to load, let alone a separate .js or .css file.

@brummer-simon
Copy link
Member Author

brummer-simon commented Sep 7, 2021

I can confirm that with this I only have to wait a few seconds before I can re-connect to my telnet server.

Still I don't really like that concept of 'broken defaults'. What speaks against enabling this always?
It is certainly surprising that a connection still lingers around after sock_tcp_disconnect() and you can't accept new connections for a couple minutes. To me this is a bug.

Just imagine a HTTP server that also serves an image - you would have to sit there waiting several minutes for the image to load, let alone a separate .js or .css file.

The issue with enabling this on default is that it is my brainchild and therefore not in the TCP standard. The funny thing is that the lingering connections are part of TCP. Googles webservers for googles search did a hack a while ago terminating connections with a reset instead of normal connection teardown because they had issues with single maschines running out of available handles during times of heavy load.

Thats one of the reasons why they are pushing and developing QUIC as a TCP replacement...

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 7, 2021
@jeandudey jeandudey merged commit cd728e0 into RIOT-OS:master Sep 13, 2021
@benpicco benpicco added this to the Release 2021.10 milestone Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Kconfig Area: Kconfig integration Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants