Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tcp: add Delayed ACK #404

Merged
merged 1 commit into from
Jan 6, 2021
Merged

tcp: add Delayed ACK #404

merged 1 commit into from
Jan 6, 2021

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Jan 3, 2021

This adds Delayed ACK to TCP sockets. By default is on at 10ms. User can set another delay or turn it off, per-socket.

Some highlights of tests against a Linux stack:

Echo of a single character

Delayed ACK saves us from sending a packet, allowing the ACK to be send as part of the echo response.

Before:

2109	153.035063600	192.168.69.100	192.168.69.1	TCP	55	46560 → 6970 [PSH, ACK] Seq=2 Ack=2 Win=64256 Len=1
2110	153.035360221	192.168.69.1	192.168.69.100	TCP	54	6970 → 46560 [ACK] Seq=2 Ack=3 Win=63 Len=0
2111	153.035494863	192.168.69.1	192.168.69.100	TCP	55	6970 → 46560 [PSH, ACK] Seq=2 Ack=3 Win=64 Len=1
2112	153.056838360	192.168.69.100	192.168.69.1	TCP	54	46560 → 6970 [ACK] Seq=3 Ack=3 Win=64256 Len=0

After:

2123	303.804961760	192.168.69.100	192.168.69.1	TCP	55	46578 → 6970 [PSH, ACK] Seq=1 Ack=1 Win=64256 Len=1
2124	303.805361855	192.168.69.1	192.168.69.100	TCP	55	6970 → 46578 [PSH, ACK] Seq=1 Ack=2 Win=64 Len=1
2125	303.826386620	192.168.69.100	192.168.69.1	TCP	54	46578 → 6970 [ACK] Seq=2 Ack=2 Win=64256 Len=0

Big transfer

Here the ACK is sent every 2 received packets. Unfortunately it doesn't get coalesced with the window update, because every 2 packets we must send an immediate non-delayed ack.

Before:
2081	80.302120849	192.168.69.100	192.168.69.1	TCP	1078	47952 → 1235 [PSH, ACK] Seq=687105 Ack=1 Win=64256 Len=1024
2082	80.302160968	192.168.69.1	192.168.69.100	TCP	  54	1235 → 47952 [ACK] Seq=1 Ack=688129 Win=1024 Len=0
2083	80.302171172	192.168.69.1	192.168.69.100	TCP	  54	[TCP Window Update] 1235 → 47952 [ACK] Seq=1 Ack=688129 Win=2048 Len=0
2084	80.322883121	192.168.69.100	192.168.69.1	TCP	1078	47952 → 1235 [ACK] Seq=688129 Ack=1 Win=64256 Len=1024
2085	80.322924012	192.168.69.1	192.168.69.100	TCP	  54	1235 → 47952 [ACK] Seq=1 Ack=689153 Win=1024 Len=0
2086	80.322934248	192.168.69.1	192.168.69.100	TCP	  54	[TCP Window Update] 1235 → 47952 [ACK] Seq=1 Ack=689153 Win=2048 Len=0

After:
3169	348.618900317	192.168.69.100	192.168.69.1	TCP	1078	47976 → 1235 [ACK] Seq=530433 Ack=1 Win=64256 Len=1024
3170	348.627494172	192.168.69.100	192.168.69.1	TCP	1078	[TCP Window Full] 47976 → 1235 [PSH, ACK] Seq=531457 Ack=1 Win=64256 Len=1024
3171	348.627556721	192.168.69.1	192.168.69.100	TCP	54	1235 → 47976 [ACK] Seq=1 Ack=532481 Win=1024 Len=0
3172	348.627573109	192.168.69.1	192.168.69.100	TCP	54	[TCP Window Update] 1235 → 47976 [ACK] Seq=1 Ack=532481 Win=2048 Len=0

@Dirbaio
Copy link
Member Author

Dirbaio commented Jan 4, 2021

Clippy is failing but it's not on my code...

  • What do we do about this?
  • How is it possible, if it passes on master?

@Dirbaio
Copy link
Member Author

Dirbaio commented Jan 4, 2021

It broke spontaneously because CI runs clippy on latest stable, and 1.49 is just out which added some warnings. Maybe it's not a good idea to enforce "No warnings on latest stable"

@crawford
Copy link
Contributor

crawford commented Jan 4, 2021

@Dirbaio I figured that might happen. I've opened #407 to pin to 1.49.0 so that we can deal with the new lints on our own schedule.

Copy link
Contributor

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

LGTM. I did my best to double-check the new logic, but my experience here is limited.

src/socket/tcp.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants