Re: [PATCH] Setting SO_KEEPALIVE on outbound connections
Do you have an updated patch on this? Amos
Re: [PATCH] Setting SO_KEEPALIVE on outbound connections
On Mon, Feb 10, 2014 at 09:35:02PM +1300, Amos Jeffries wrote: Do you have an updated patch on this? Sorry, I haven't had time to dig into the configuration end of this. I do intend to when I get a chance, but I won't be offended if someone else does it first.
Re: [PATCH] Setting SO_KEEPALIVE on outbound connections
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.0 +
+++ b/src/comm.cc 2014-01-28 05:50:23.242620051 +
@@ -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.
Re: [PATCH] Setting SO_KEEPALIVE on outbound connections
On Tue, Jan 28, 2014 at 01:03:24PM -0700, Alex Rousskov wrote:
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)?
Good idea.
+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).
Will do.
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?
I think this is a good idea. I don't want to disable system settings by
default, however. Would this preclude using an onoff config type? I'd need
a third not set state.
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)
Roger. Will do.
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)
Ah, thanks for the explanation, I see the intent there is to provide a
more user friendly input than what the underlying implementation exposes.
* 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.
As a user who understands tcp well enough to be aware of these settings
I'm going to be looking for knobs to manipulate the relevant options
directly. It's somewhat confusing to see timeout in seconds when I know
the underlying implementation doesn't provide such a thing. I just want to
set it to X, where X is the same as the sockopts in all my other
applications. I end up needing to read source to verify it is actually
touching TCP_KEEPCNT after some calculation.
Maybe putting the formula for calculating the probe count in the docs
would've headed off this confusion on my part? Or how about adding
keep_count withouit deprecating timeout, as a conflicting parameter?
(Now that I understand the intent here I care much less about it)
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.
I was afraid of that. I'll give it a shot, thanks for the pointers.
Please avoid adding non-changes.
Please send future patches as attachments to avoid copy-paste errors and
whitespace changes when the patch is committed.
Oops, sorry about that. Will do.
Re: [PATCH] Setting SO_KEEPALIVE on outbound connections
On 01/28/2014 02:52 PM, e...@frap.net wrote: On Tue, Jan 28, 2014 at 01:03:24PM -0700, Alex Rousskov wrote: 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? I think this is a good idea. I don't want to disable system settings by default, however. Would this preclude using an onoff config type? I'd need a third not set state. See memory_cache_shared in src/cf.data.pre and class YesNoNone. Maybe putting the formula for calculating the probe count in the docs would've headed off this confusion on my part? Or how about adding keep_count withouit deprecating timeout, as a conflicting parameter? (Now that I understand the intent here I care much less about it) Matching an existing OS interface certainly has some merit. If you want to do it, adding an option to specify the probe count directly is acceptable IMHO, provided the new code rejects self-conflicting configurations. If you would rather not deal with coding this up, documentation improvements are always welcome. You can, for example, submit a patch to describe the mapping between Squid and popular sysctl or /proc settings. Thank you, Alex.
