Reduce latency caused by TCP delayed ACK

[faisal-shah]

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: https://github.com/hardbyte/python-can/issues/1684

[faisal-shah]

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

[hartkopp]

https://github.com/hardbyte/python-can/pull/1683#issuecomment-1779773192

[faisal-shah]

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]

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]

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]

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]

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

[faisal-shah]

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:

[marckleinebudde]

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]

looks good, please squash both patches and force push

[faisal-shah]

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

[faisal-shah]

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

[marckleinebudde]

Look good to me. Please squash both patches into one and force push the branch.

[faisal-shah]

Squashed.