On Wed, Jul 08, 2020 at 10:15:38PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Introduce keep-alive settings (TCP_KEEPCNT, TCP_KEEPIDLE,
> TCP_KEEPINTVL) and chose some defaults.
>
> The linux default of 2 hours for /proc/tcp_keepalive_time
> (corresponding to TCP_KEEPIDLE) makes keep-alive option almost
> superfluous. Let's add a possibility to set the options by hand
> and specify some defaults resulting in smaller total time to terminate
> idle connection.
As you say, 2 hours just a default. The sysadmin can override that
as they wish to change the behaviour globally on the system, so using
the global settings is quite reasonable IMHO.
> Do not document the default values in QAPI as they may be altered in
> future (careful user will use explicit values).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> ---
>
> Suggested default numbers are RFC, any better suggestion is welcome.
> I just looked at /etc/libvirt/qemu.conf in my system and take values of
> keepalive_interval and keepalive_count.
> The only thing I'm sure in is that 2 hours is too long.
>
> qapi/sockets.json | 33 +++++++++++++++++++-
> util/qemu-sockets.c | 76 ++++++++++++++++++++++++++++++++++++++-------
> 2 files changed, 97 insertions(+), 12 deletions(-)
>
> diff --git a/qapi/sockets.json b/qapi/sockets.json
> index cbd6ef35d0..73ff66a5d5 100644
> --- a/qapi/sockets.json
> +++ b/qapi/sockets.json
> @@ -37,6 +37,37 @@
> 'host': 'str',
> 'port': 'str' } }
>
> +##
> +# @KeepAliveSettings:
> +#
> +# @idle: The time (in seconds) the connection needs to remain idle
> +# before TCP starts sending keepalive probes (sets TCP_KEEPIDLE).
> +# @interval: The time (in seconds) between individual keepalive probes
> +# (sets TCP_KEEPINTVL).
> +# @count: The maximum number of keepalive probes TCP should send before
> +# dropping the connection (sets TCP_KEEPCNT).
> +#
> +# Since: 5.2
> +##
> +{ 'struct': 'KeepAliveSettings',
> + 'data': {
> + 'idle': 'int',
> + 'interval': 'int',
> + 'count': 'int' } }
> +
> +##
> +# @KeepAliveField:
> +#
> +# @enabled: If true, enable keep-alive with some default settings
> +# @settings: Enable keep-alive and use explicit settings
> +#
> +# Since: 5.2
> +##
> +{ 'alternate': 'KeepAliveField',
> + 'data': {
> + 'enabled': 'bool',
> + 'settings': 'KeepAliveSettings' } }
> +
> ##
> # @InetSocketAddress:
> #
> @@ -65,7 +96,7 @@
> '*to': 'uint16',
> '*ipv4': 'bool',
> '*ipv6': 'bool',
> - '*keep-alive': 'bool' } }
> + '*keep-alive': 'KeepAliveField' } }
>
> ##
> # @UnixSocketAddress:
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index b37d288866..b961963472 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -433,6 +433,57 @@ static struct addrinfo
> *inet_parse_connect_saddr(InetSocketAddress *saddr,
> return res;
> }
>
> +/*
> + * inet_set_keepalive
> + *
> + * Handle keep_alive settings. If user specified settings explicitly, fail if
> + * can't set the settings. If user just enabled keep-alive, not specifying
> the
> + * settings, try to set defaults but ignore failures.
> + */
> +static int inet_set_keepalive(int sock, bool has_keep_alive,
> + KeepAliveField *keep_alive, Error **errp)
> +{
> + int ret;
> + int val;
> + bool has_settings = has_keep_alive && keep_alive->type == QTYPE_QDICT;
> +
> + if (!has_keep_alive || (keep_alive->type == QTYPE_QBOOL &&
> + !keep_alive->u.enabled))
> + {
> + return 0;
> + }
> +
> + val = 1;
> + ret = qemu_setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE, &val, sizeof(val));
> + if (ret < 0) {
> + error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
> + return -1;
> + }
> +
> + val = has_settings ? keep_alive->u.settings.idle : 30;
> + ret = qemu_setsockopt(sock, IPPROTO_TCP, TCP_KEEPIDLE, &val,
> sizeof(val));
> + if (has_settings && ret < 0) {
> + error_setg_errno(errp, errno, "Unable to set TCP_KEEPIDLE");
> + return -1;
> + }
> +
> + val = has_settings ? keep_alive->u.settings.interval : 30;
> + ret = qemu_setsockopt(sock, IPPROTO_TCP, TCP_KEEPINTVL, &val,
> sizeof(val));
> + if (has_settings && ret < 0) {
> + error_setg_errno(errp, errno, "Unable to set TCP_KEEPINTVL");
> + return -1;
> + }
> +
> + val = has_settings ? keep_alive->u.settings.count : 20;
> + ret = qemu_setsockopt(sock, IPPROTO_TCP, TCP_KEEPCNT, &val, sizeof(val));
> + if (has_settings && ret < 0) {
> + error_setg_errno(errp, errno, "Unable to set TCP_KEEPCNT");
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> /**
> * Create a socket and connect it to an address.
> *
> @@ -468,16 +519,11 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error
> **errp)
> return sock;
> }
>
> - if (saddr->keep_alive) {
> - int val = 1;
> - int ret = qemu_setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE,
> - &val, sizeof(val));
> -
> - if (ret < 0) {
> - error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
> - close(sock);
> - return -1;
> - }
> + if (inet_set_keepalive(sock, saddr->has_keep_alive, saddr->keep_alive,
> + errp) < 0)
> + {
> + close(sock);
> + return -1;
> }
>
> return sock;
> @@ -677,12 +723,20 @@ int inet_parse(InetSocketAddress *addr, const char
> *str, Error **errp)
> }
> begin = strstr(optstr, ",keep-alive");
> if (begin) {
> + bool val;
> +
> if (inet_parse_flag("keep-alive", begin + strlen(",keep-alive"),
> - &addr->keep_alive, errp) < 0)
> + &val, errp) < 0)
> {
> return -1;
> }
> +
> addr->has_keep_alive = true;
> + addr->keep_alive = g_new(KeepAliveField, 1);
> + *addr->keep_alive = (KeepAliveField) {
> + .type = QTYPE_QBOOL,
> + .u.enabled = val
> + };
> }
> return 0;
> }
> --
> 2.21.0
>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
