On 01/28/2014 11:14 AM, e...@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.