Re: [PATCH] Setting SO_KEEPALIVE on outbound connections

On 01/28/2014 11:14 AM, eam_at_frap.net wrote:

> Below I include a patch to enable configurable SO_KEEPALIVE on outbound
> connections from Squid.

> +NAME: tcp_keepalive

If the new directive is specific to outbound connections, should it be
renamed to reflect that fact? How about tcp_outgoing_keepalive (to mimic
tcp_outgoing_address)?

> +DOC_START
> + This directive enables TCP keepalive for outbound connections.
> +DOC_END

Please document what happens by default -- Squid will not do that for
you when generating squid.conf.documented, unfortunately.

Please document what happens when the directive is explicitly set to
off. I think some admins will be surprised to learn that "tcp_keepalive
off" does not actually disable keepalives in the current implementation
but relies on system-wide settings instead (AFAICT).

Should this directive implementation be adjusted to also work for
disabling TCP keepalive for outbound connections if the admin wants to
overwrite a system-wide setting?

> 2) I did not use commSetTcpKeepalive() as I believe it may have an issue
> in its handling of TCP_KEEPCNT.

That is not a valid reason because you can call commSetTcpKeepalive()
with parameters that bypass TCP_KEEPCNT code inside
commSetTcpKeepalive(). Please do use that existing function instead of
partially duplicating its code:

   commSetTcpKeepalive(fd, 0, 0, 0)

> There are four relevant keepalive values:
>
> a) SO_KEEPALIVE, boolean. The only portable value.
>
> b) TCP_KEEPIDLE, idle time in seconds before the socket is placed in
> keepalive mode. AKA tcp_keepalive_time on linux
>
> c) TCP_KEEPINTVL, seconds between probes, once keepalive mode is
> engaged. AKA tcp_keepalive_intvl on linux
>
> d) TCP_KEEPCNT, the maximum number of keepalive probes sent before
> dropping the connection.
>
> commSetTcpKeepalive() appears to treat TCP_KEEPCNT as a time value, rather
> than a count value:

What makes you think that? Rounding math aside, the code below appears
to divide timeout by interval, to count the number of intervals in a
timeout, no?

>> if (timeout && interval) {
>> int count = (timeout + interval - 1) / interval;
>> if (setsockopt(fd, IPPROTO_TCP, TCP_KEEPCNT, &count, sizeof(on)) < 0)
>
> I would suggest a larger patch modifying commSetTcpKeepalive() to replace
> "int timeout" with "int keep_count", and replace the above three lines
> with only:
>
>> if (setsockopt(fd, IPPROTO_TCP, TCP_KEEPCNT, &keep_count, sizeof(on)) < 0)

> If squid-dev is amenable to this change I will submit another patch for
> this issue. It's a behavior change so I wanted to keep it separate from
> the feature patch I include below. I would recommend:
>
> * Introduce new keep_count= parameter
> * Retain current behavior if keep_count= is not present and timeout= is
> set to avoid changing existing behavior
> * Document deprecation of timeout= parameter

Why would you like to replace timeout with keep_count? A seconds-based
timeout seems to be a more natural way to configure this feature than a
counter IMHO.

> 3) I only include a boolean tcp_keepalive, rather than passing in args for
> the remaining 3 parameters. I'm a bit confused as to how I should use
> cf.data.pre to pass config parameters. If someone could point me at how to
> supply something like this I'd appreciate it:
>
> tcp_keepalive[=idle,interval,keep_count]

For a stand-alone directive, you should use a different syntax IMO:

tcp_keepalive off
tcp_keepalive on [idle=seconds] [interval=seconds] [timeout=seconds]

For parsing examples, see sslcrtd_children, external_acl_type,
http_port, and many other directives that use similar syntax. Hint: You
will have to write code to parse such an option. Squid does not have a
configurable reusable parser for complex options like this one.

> diff -ur a/src/comm.cc b/src/comm.cc
> --- a/src/comm.cc 2013-12-30 11:33:27.000000000 +0000
> +++ b/src/comm.cc 2014-01-28 05:50:23.242620051 +0000
> @@ -702,10 +702,14 @@
> commSetTcpNoDelay(new_socket);
>
> #endif
> -
> if (Config.tcpRcvBufsz > 0 && sock_type == SOCK_STREAM)
> commSetTcpRcvbuf(new_socket, Config.tcpRcvBufsz);

Please avoid adding non-changes.

Please send future patches as attachments to avoid copy-paste errors and
whitespace changes when the patch is committed.

Thank you,

Alex.
Received on Tue Jan 28 2014 - 20:03:46 MST