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, 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)?

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.
Received on Tue Jan 28 2014 - 21:53:03 MST