-
Notifications
You must be signed in to change notification settings - Fork 51
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
Reduce latency caused by TCP delayed ACK #28
Reduce latency caused by TCP delayed ACK #28
Conversation
Sorry, just noticed the formatting issues, let me fix and force push |
3c93a56
to
b34940e
Compare
Thanks for your comment. So, how would you like me to approach this. Shall I add a command line option to socketcand? If so, you want something high level like 'low latency', or specific like 'set TCP_QUICKACK on socket' ? |
Yes, a command line option something like: And maybe you can also create a (global) variable to simplify the sockopt() calls like:
Many thanks! |
Cool, will do. No delay is set by default currently. Keep it that way? Your description of the option is slightly imprecise. Maybe something like enable quickack in addition to nodelay? |
Ugh! Sorry. I wasn't aware that TCP_NODELAY is already enabled in socketcand. Then I would suggest to only add TCP_QUICKACK depending on the command line.
|
b34940e
to
513ff97
Compare
I messed up the push, ignore, will fix and notify when ready. |
513ff97
to
cfc49e2
Compare
That's a lot of code duplication. What about adding a function like this:
|
looks good, please squash both patches and force push |
a2d27dc
to
b07ca79
Compare
Let me know if anything else needs to be done. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you don't need the additional includes, right?
Sorry, not familiar with the github review feature. I went ahead and submitted another commit to address the code review changes (unnecessary includes). |
Look good to me. Please squash both patches into one and force push the branch. |
Default in linux is ~40ms. This can and does introduce latency in the data stream. Protocols sensitive to this type of timing result in timeouts. Setting TCP_QUICKACK in sock options disables the delayed ACK. The catch is that it resets itself after every send/recv - so it needs to be reenabled upon each call.
cdddd1e
to
c06ff30
Compare
Squashed. |
Co-authored-by: Marc Kleine-Budde <[email protected]>
Default in linux is ~40ms. This can and does introduce latency in the data stream. Protocols sensitive to this type of timing result in timeouts.
Setting TCP_QUICKACK in sock options disables the delayed ACK. The catch is that it resets itself after every send/recv - so it needs to be reenabled upon each call.
Related:
hardbyte/python-can#1684