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_FASTOPEN option for Socket #336

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

zonyitoo
Copy link

- Supported platforms: Linux-like, FreeBSD, macOS, Windows
- ref rust-lang#49
Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

Can you add a smoke test for this?

If I call this with the value 5, does this work on non-Linux? I.e. enable it, or would it error and would users have to do if linux { 5 } else { 1 }? Depending on the answer can you also add a test for this?

@zonyitoo
Copy link
Author

thread 'tcp_fastopen' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 22, kind: InvalidInput, message: "Invalid argument" }', tests/socket.rs:1302:32

We have the answer, value must be 0 and 1 excepts Linux.

@zonyitoo
Copy link
Author

thread 'tcp_fastopen' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 1, kind: PermissionDenied, message: "Operation not permitted" }', tests/socket.rs:1306:36

Test failed on FreeBSD CI.

https://github.com/freebsd/freebsd-src/blob/e7d02be19d40063783d6b8f1ff2bc4c7170fd434/sys/netinet/tcp_usrreq.c#L2458-L2507

It seems that TFO is not enabled on CI's OS.

@zonyitoo
Copy link
Author

@Thomasdezeeuw Should I remove FreeBSD support?

@Thomasdezeeuw
Copy link
Collaborator

Can you change the method so that 0 means disable, and >= 1 means enable, mapping anything bigger than 1 to 1 on non-Linux systems? And update the docs accordingly.

For FreeBSD can you try add a new step to .cirrus.yml? With the following:

  enable_tcp_fastopen:
    - sysctl net.inet.tcp.fastopen.client_enabled=1
    - sysctl net.inet.tcp.fastopen.server_enabled=1

After build_script should work. This should enable it on FreeBSD.

src/socket.rs Outdated Show resolved Hide resolved
tests/socket.rs Outdated Show resolved Hide resolved
zonyitoo and others added 5 commits August 26, 2022 00:35
Co-authored-by: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Co-authored-by: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
@zonyitoo
Copy link
Author

Still not working on FreeBSD's CI test.

@Thomasdezeeuw
Copy link
Collaborator

@zonyitoo I'll work on FreeBSD

@Thomasdezeeuw
Copy link
Collaborator

@zonyitoo the sysctl names I gave you are incorrect. These should fix them:

-    - sysctl net.inet.tcp.fastopen.client_enabled=1
-    - sysctl net.inet.tcp.fastopen.server_enabled=1
+    - sudo sysctl net.inet.tcp.fastopen.client_enable=1
+    - sudo sysctl net.inet.tcp.fastopen.server_enable=1

However that doesn't seem to fix the problem (see my next comment).

@Thomasdezeeuw
Copy link
Collaborator

@asomers maybe you can help here. I got this to work on FreeBSD (the sysctl names are incorrect). But when retrieving the value of TCP_FASTOPEN isn't 1, but 2147483648, is this expected? I think the value comes from https://github.com/freebsd/freebsd-src/blob/e7d02be19d40063783d6b8f1ff2bc4c7170fd434/sys/netinet/tcp_usrreq.c#L2693-L2697, but I could be wrong here.

@asomers
Copy link

asomers commented Aug 26, 2022

@Thomasdezeeuw this socket option is a flag, so you should only interpret it as either zero or non-zero. TCP_NODELAY is similar, and I see that socket2 interprets it correctly. I suggest you change the Socket::tcp_fastopen signature to match Socket::nodelay.

@Thomasdezeeuw
Copy link
Collaborator

@Thomasdezeeuw this socket option is a flag, so you should only interpret it as either zero or non-zero. TCP_NODELAY is similar, and I see that socket2 interprets it correctly. I suggest you change the Socket::tcp_fastopen signature to match Socket::nodelay.

Thanks. That makes the API a little awkward as the value on Linux is the accept queue length, so we'll need to rethink the API.

.cirrus.yml 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants