Re: [PATCH 1/2] sockets: keep-alive settings

["Daniel P. Berrangé" <berrange@redhat.com>]

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 <vsementsov@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 :|