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

Reduce latency caused by TCP delayed ACK #28

Merged

Conversation

faisal-shah
Copy link
Contributor

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

@faisal-shah
Copy link
Contributor Author

faisal-shah commented Oct 19, 2023

Sorry, just noticed the formatting issues, let me fix and force push

@hartkopp
Copy link
Member

@faisal-shah
Copy link
Contributor Author

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' ?

@hartkopp
Copy link
Member

Yes, a command line option something like:
[-t| --tune-tcp]
-t (enable TCP_NODELAY and TCP_QUICKACK socket options)

And maybe you can also create a (global) variable to simplify the sockopt() calls like:

if (tune_tcp)
    setsockopt(client_socket, IPPROTO_TCP, TCP_QUICKACK, &tune_tcp, sizeof(tune_tcp));

Many thanks!

@faisal-shah
Copy link
Contributor Author

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?

@hartkopp
Copy link
Member

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.

[-q | --quick-ack]
-q (enable TCP_QUICKACK socket option)

@faisal-shah
Copy link
Contributor Author

I messed up the push, ignore, will fix and notify when ready.

@faisal-shah
Copy link
Contributor Author

faisal-shah commented Oct 25, 2023

Ok, I believe the patch is ready to go. Please review.

I took the liberty to remove some superfluous tab characters as well.

Tried to insert the option where it made sense, and re-wrapped the usage help. Looks like this on my terminal:
image

@marckleinebudde
Copy link
Member

That's a lot of code duplication. What about adding a function like this:

void tcp_quickack(int s)
{
	int i = 1;

	if (tcp_quickack_flag)
		setsockopt(s, IPPROTO_TCP, TCP_QUICKACK, &i, sizeof(int));
}

@marckleinebudde
Copy link
Member

looks good, please squash both patches and force push

@faisal-shah
Copy link
Contributor Author

Let me know if anything else needs to be done. Thanks!

Copy link
Member

@marckleinebudde marckleinebudde left a 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?

statistics.c Outdated Show resolved Hide resolved
state_raw.c Outdated Show resolved Hide resolved
state_isotp.c Outdated Show resolved Hide resolved
state_control.c Outdated Show resolved Hide resolved
state_bcm.c Outdated Show resolved Hide resolved
@faisal-shah
Copy link
Contributor Author

Sorry, not familiar with the github review feature. I went ahead and submitted another commit to address the code review changes (unnecessary includes).

@marckleinebudde
Copy link
Member

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.
@faisal-shah
Copy link
Contributor Author

Squashed.

socketcand.h Outdated Show resolved Hide resolved
Co-authored-by: Marc Kleine-Budde <[email protected]>
@marckleinebudde marckleinebudde merged commit 02ad0f5 into linux-can:master Dec 6, 2023
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