[PATCH v2 2/2] utils/qemu-sockets: Introduce keep-alive-idle-period inet socket option

Juraj Marcin posted 2 patches 1 week, 6 days ago
[PATCH v2 2/2] utils/qemu-sockets: Introduce keep-alive-idle-period inet socket option
Posted by Juraj Marcin 1 week, 6 days ago
From: Juraj Marcin <jmarcin@redhat.com>

The default idle period for TCP connection could be even 2 hours.
However, in some cases, the application needs to be aware of a
connection issue much sooner.

This is the case, for example, for postcopy live migration. If there is
no traffic from the migration destination guest (server-side) to the
migration source guest (client-side), the destination keeps waiting for
pages indefinitely and does not switch to the postcopy-paused state.
This can happen, for example, if the destination QEMU instance is
started with '-S' command line option and the machine is not started yet
or if the machine is idle and produces no new page faults for
not-yet-migrated pages.

This patch introduces a new inet socket parameter on platforms where
TCP_KEEPIDLE is defined and passes the configured value to the
TCP_KEEPIDLE socket option when a client-side or server-side socket is
created.

The default value is 0, which means system configuration is used, and no
custom value is set.

Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
---
 io/dns-resolver.c   |  4 ++++
 meson.build         |  2 ++
 qapi/sockets.json   |  5 +++++
 util/qemu-sockets.c | 29 +++++++++++++++++++++++++++++
 4 files changed, 40 insertions(+)

diff --git a/io/dns-resolver.c b/io/dns-resolver.c
index ee7403b65b..03c59673f0 100644
--- a/io/dns-resolver.c
+++ b/io/dns-resolver.c
@@ -128,6 +128,10 @@ static int qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver,
 #endif
             .has_keep_alive = iaddr->has_keep_alive,
             .keep_alive = iaddr->keep_alive,
+#ifdef HAVE_TCP_KEEPIDLE
+            .has_keep_alive_idle_period = iaddr->has_keep_alive_idle_period,
+            .keep_alive_idle_period = iaddr->keep_alive_idle_period,
+#endif
         };
 
         (*addrs)[i] = newaddr;
diff --git a/meson.build b/meson.build
index 41f68d3806..e3440b09c8 100644
--- a/meson.build
+++ b/meson.build
@@ -2734,6 +2734,8 @@ if linux_io_uring.found()
   config_host_data.set('HAVE_IO_URING_PREP_WRITEV2',
                        cc.has_header_symbol('liburing.h', 'io_uring_prep_writev2'))
 endif
+config_host_data.set('HAVE_TCP_KEEPIDLE',
+                     cc.has_header_symbol('netinet/tcp.h', 'TCP_KEEPIDLE'))
 
 # has_member
 config_host_data.set('HAVE_SIGEV_NOTIFY_THREAD_ID',
diff --git a/qapi/sockets.json b/qapi/sockets.json
index eb50f64e3a..ddd82b1e66 100644
--- a/qapi/sockets.json
+++ b/qapi/sockets.json
@@ -59,6 +59,10 @@
 # @keep-alive: enable keep-alive when connecting to/listening on this socket.
 #     (Since 4.2, not supported for listening sockets until 10.0)
 #
+# @keep-alive-idle-period: time in seconds the connection needs to be idle
+#     before sending a keepalive packet.  Only supported for TCP sockets on
+#     systems where TCP_KEEPIDLE socket option is defined.  (Since 10.0)
+#
 # @mptcp: enable multi-path TCP.  (Since 6.1)
 #
 # Since: 1.3
@@ -71,6 +75,7 @@
     '*ipv4': 'bool',
     '*ipv6': 'bool',
     '*keep-alive': 'bool',
+    '*keep-alive-idle-period': { 'type': 'uint32', 'if': 'HAVE_TCP_KEEPIDLE' },
     '*mptcp': { 'type': 'bool', 'if': 'HAVE_IPPROTO_MPTCP' } } }
 
 ##
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 99357a4435..23c8a6cc2b 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -217,6 +217,19 @@ static int inet_set_sockopts(int sock, InetSocketAddress *saddr, Error **errp)
                              "Unable to set keep-alive option on socket");
             return -1;
         }
+#ifdef HAVE_TCP_KEEPIDLE
+        if (saddr->has_keep_alive_idle_period &&
+            saddr->keep_alive_idle_period) {
+            int keep_idle = saddr->has_keep_alive_idle_period;
+            ret = setsockopt(sock, IPPROTO_TCP, TCP_KEEPIDLE, &keep_idle,
+                             sizeof(keep_idle));
+            if (ret < 0) {
+                error_setg_errno(errp, errno,
+                                 "Unable to set TCP keep-alive idle option on socket");
+                return -1;
+            }
+        }
+#endif
     }
     return 0;
 }
@@ -697,6 +710,22 @@ int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
         }
         addr->has_keep_alive = true;
     }
+#ifdef HAVE_TCP_KEEPIDLE
+    begin = strstr(optstr, ",keep-alive-idle-period=");
+    if (begin) {
+        begin += strlen(",keep-alive-idle-period=");
+        if (sscanf(begin, "%" PRIu32 "%n", &addr->keep_alive_idle_period, &pos) != 1 ||
+            (begin[pos] != '\0' && begin[pos] != ',')) {
+            error_setg(errp, "error parsing keep-alive-idle-period argument");
+            return -1;
+        }
+        if (addr->keep_alive_idle_period > INT_MAX) {
+            error_setg(errp, "keep-alive-idle-period value is too large");
+            return -1;
+        }
+        addr->has_keep_alive_idle_period = true;
+    }
+#endif
 #ifdef HAVE_IPPROTO_MPTCP
     begin = strstr(optstr, ",mptcp");
     if (begin) {
-- 
2.48.1
Re: [PATCH v2 2/2] utils/qemu-sockets: Introduce keep-alive-idle-period inet socket option
Posted by Daniel P. Berrangé 1 week, 2 days ago
On Wed, Mar 19, 2025 at 05:36:20PM +0100, Juraj Marcin wrote:
> From: Juraj Marcin <jmarcin@redhat.com>
> 
> The default idle period for TCP connection could be even 2 hours.
> However, in some cases, the application needs to be aware of a
> connection issue much sooner.
> 
> This is the case, for example, for postcopy live migration. If there is
> no traffic from the migration destination guest (server-side) to the
> migration source guest (client-side), the destination keeps waiting for
> pages indefinitely and does not switch to the postcopy-paused state.
> This can happen, for example, if the destination QEMU instance is
> started with '-S' command line option and the machine is not started yet
> or if the machine is idle and produces no new page faults for
> not-yet-migrated pages.
> 
> This patch introduces a new inet socket parameter on platforms where
> TCP_KEEPIDLE is defined and passes the configured value to the
> TCP_KEEPIDLE socket option when a client-side or server-side socket is
> created.
> 
> The default value is 0, which means system configuration is used, and no
> custom value is set.
> 
> Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
> ---
>  io/dns-resolver.c   |  4 ++++
>  meson.build         |  2 ++
>  qapi/sockets.json   |  5 +++++
>  util/qemu-sockets.c | 29 +++++++++++++++++++++++++++++
>  4 files changed, 40 insertions(+)
> 
> diff --git a/io/dns-resolver.c b/io/dns-resolver.c
> index ee7403b65b..03c59673f0 100644
> --- a/io/dns-resolver.c
> +++ b/io/dns-resolver.c
> @@ -128,6 +128,10 @@ static int qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver,
>  #endif
>              .has_keep_alive = iaddr->has_keep_alive,
>              .keep_alive = iaddr->keep_alive,
> +#ifdef HAVE_TCP_KEEPIDLE
> +            .has_keep_alive_idle_period = iaddr->has_keep_alive_idle_period,
> +            .keep_alive_idle_period = iaddr->keep_alive_idle_period,
> +#endif
>          };
>  
>          (*addrs)[i] = newaddr;

I feel like this code is somewhat fragile. It is probably best to add a
commit that refactors it to do a struct copy, and then override the few
fields that need to be different.

     newaddr->u.inet = iaddr;
     newaddr->u.inet.host = g_strdup(uaddr);
     ...

that way we don't risk forgetting to copy fields as fixed in the previous
commit

> diff --git a/meson.build b/meson.build
> index 41f68d3806..e3440b09c8 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2734,6 +2734,8 @@ if linux_io_uring.found()
>    config_host_data.set('HAVE_IO_URING_PREP_WRITEV2',
>                         cc.has_header_symbol('liburing.h', 'io_uring_prep_writev2'))
>  endif
> +config_host_data.set('HAVE_TCP_KEEPIDLE',
> +                     cc.has_header_symbol('netinet/tcp.h', 'TCP_KEEPIDLE'))
>  
>  # has_member
>  config_host_data.set('HAVE_SIGEV_NOTIFY_THREAD_ID',
> diff --git a/qapi/sockets.json b/qapi/sockets.json
> index eb50f64e3a..ddd82b1e66 100644
> --- a/qapi/sockets.json
> +++ b/qapi/sockets.json
> @@ -59,6 +59,10 @@
>  # @keep-alive: enable keep-alive when connecting to/listening on this socket.
>  #     (Since 4.2, not supported for listening sockets until 10.0)
>  #
> +# @keep-alive-idle-period: time in seconds the connection needs to be idle
> +#     before sending a keepalive packet.  Only supported for TCP sockets on
> +#     systems where TCP_KEEPIDLE socket option is defined.  (Since 10.0)

There are three tunables for keepalive on TCP sockets:

   TCP_KEEPCNT (since Linux 2.4)
       The maximum number of keepalive probes TCP should send before
       dropping the connection.  This option should not be used in
       code intended to be portable.

   TCP_KEEPIDLE (since Linux 2.4)
       The time (in seconds) the connection needs to remain idle
       before TCP starts sending keepalive probes, if the socket
       option SO_KEEPALIVE has  been  set  on  this socket.  This
       option should not be used in code intended to be portable.

   TCP_KEEPINTVL (since Linux 2.4)
       The time (in seconds) between individual keepalive probes.
       This option should not be used in code intended to be portable.

Shouldn't we be supporting all of these, rather than just a subset ?

> +#
>  # @mptcp: enable multi-path TCP.  (Since 6.1)
>  #
>  # Since: 1.3
> @@ -71,6 +75,7 @@
>      '*ipv4': 'bool',
>      '*ipv6': 'bool',
>      '*keep-alive': 'bool',
> +    '*keep-alive-idle-period': { 'type': 'uint32', 'if': 'HAVE_TCP_KEEPIDLE' },
>      '*mptcp': { 'type': 'bool', 'if': 'HAVE_IPPROTO_MPTCP' } } }
>  
>  ##

> @@ -697,6 +710,22 @@ int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
>          }
>          addr->has_keep_alive = true;
>      }
> +#ifdef HAVE_TCP_KEEPIDLE
> +    begin = strstr(optstr, ",keep-alive-idle-period=");

A bit too verbose - make it just 'keep-alive-idle='

> +    if (begin) {
> +        begin += strlen(",keep-alive-idle-period=");
> +        if (sscanf(begin, "%" PRIu32 "%n", &addr->keep_alive_idle_period, &pos) != 1 ||
> +            (begin[pos] != '\0' && begin[pos] != ',')) {
> +            error_setg(errp, "error parsing keep-alive-idle-period argument");
> +            return -1;
> +        }
> +        if (addr->keep_alive_idle_period > INT_MAX) {
> +            error_setg(errp, "keep-alive-idle-period value is too large");
> +            return -1;
> +        }
> +        addr->has_keep_alive_idle_period = true;
> +    }
> +#endif
>  #ifdef HAVE_IPPROTO_MPTCP
>      begin = strstr(optstr, ",mptcp");
>      if (begin) {
> -- 
> 2.48.1
> 

With 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 :|
Re: [PATCH v2 2/2] utils/qemu-sockets: Introduce keep-alive-idle-period inet socket option
Posted by Juraj Marcin 1 week, 1 day ago
On 2025-03-24 11:12, Daniel P. Berrangé wrote:
> On Wed, Mar 19, 2025 at 05:36:20PM +0100, Juraj Marcin wrote:
> > From: Juraj Marcin <jmarcin@redhat.com>
> > 
> > The default idle period for TCP connection could be even 2 hours.
> > However, in some cases, the application needs to be aware of a
> > connection issue much sooner.
> > 
> > This is the case, for example, for postcopy live migration. If there is
> > no traffic from the migration destination guest (server-side) to the
> > migration source guest (client-side), the destination keeps waiting for
> > pages indefinitely and does not switch to the postcopy-paused state.
> > This can happen, for example, if the destination QEMU instance is
> > started with '-S' command line option and the machine is not started yet
> > or if the machine is idle and produces no new page faults for
> > not-yet-migrated pages.
> > 
> > This patch introduces a new inet socket parameter on platforms where
> > TCP_KEEPIDLE is defined and passes the configured value to the
> > TCP_KEEPIDLE socket option when a client-side or server-side socket is
> > created.
> > 
> > The default value is 0, which means system configuration is used, and no
> > custom value is set.
> > 
> > Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
> > ---
> >  io/dns-resolver.c   |  4 ++++
> >  meson.build         |  2 ++
> >  qapi/sockets.json   |  5 +++++
> >  util/qemu-sockets.c | 29 +++++++++++++++++++++++++++++
> >  4 files changed, 40 insertions(+)
> > 
> > diff --git a/io/dns-resolver.c b/io/dns-resolver.c
> > index ee7403b65b..03c59673f0 100644
> > --- a/io/dns-resolver.c
> > +++ b/io/dns-resolver.c
> > @@ -128,6 +128,10 @@ static int qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver,
> >  #endif
> >              .has_keep_alive = iaddr->has_keep_alive,
> >              .keep_alive = iaddr->keep_alive,
> > +#ifdef HAVE_TCP_KEEPIDLE
> > +            .has_keep_alive_idle_period = iaddr->has_keep_alive_idle_period,
> > +            .keep_alive_idle_period = iaddr->keep_alive_idle_period,
> > +#endif
> >          };
> >  
> >          (*addrs)[i] = newaddr;
> 
> I feel like this code is somewhat fragile. It is probably best to add a
> commit that refactors it to do a struct copy, and then override the few
> fields that need to be different.
> 
>      newaddr->u.inet = iaddr;
>      newaddr->u.inet.host = g_strdup(uaddr);
>      ...
> 
> that way we don't risk forgetting to copy fields as fixed in the previous
> commit
> 
> > diff --git a/meson.build b/meson.build
> > index 41f68d3806..e3440b09c8 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -2734,6 +2734,8 @@ if linux_io_uring.found()
> >    config_host_data.set('HAVE_IO_URING_PREP_WRITEV2',
> >                         cc.has_header_symbol('liburing.h', 'io_uring_prep_writev2'))
> >  endif
> > +config_host_data.set('HAVE_TCP_KEEPIDLE',
> > +                     cc.has_header_symbol('netinet/tcp.h', 'TCP_KEEPIDLE'))
> >  
> >  # has_member
> >  config_host_data.set('HAVE_SIGEV_NOTIFY_THREAD_ID',
> > diff --git a/qapi/sockets.json b/qapi/sockets.json
> > index eb50f64e3a..ddd82b1e66 100644
> > --- a/qapi/sockets.json
> > +++ b/qapi/sockets.json
> > @@ -59,6 +59,10 @@
> >  # @keep-alive: enable keep-alive when connecting to/listening on this socket.
> >  #     (Since 4.2, not supported for listening sockets until 10.0)
> >  #
> > +# @keep-alive-idle-period: time in seconds the connection needs to be idle
> > +#     before sending a keepalive packet.  Only supported for TCP sockets on
> > +#     systems where TCP_KEEPIDLE socket option is defined.  (Since 10.0)
> 
> There are three tunables for keepalive on TCP sockets:
> 
>    TCP_KEEPCNT (since Linux 2.4)
>        The maximum number of keepalive probes TCP should send before
>        dropping the connection.  This option should not be used in
>        code intended to be portable.
> 
>    TCP_KEEPIDLE (since Linux 2.4)
>        The time (in seconds) the connection needs to remain idle
>        before TCP starts sending keepalive probes, if the socket
>        option SO_KEEPALIVE has  been  set  on  this socket.  This
>        option should not be used in code intended to be portable.
> 
>    TCP_KEEPINTVL (since Linux 2.4)
>        The time (in seconds) between individual keepalive probes.
>        This option should not be used in code intended to be portable.
> 
> Shouldn't we be supporting all of these, rather than just a subset ?

They were not strictly necessary for the Live Migration use case, but I
can also include them in the next version.

Thank you for your feedback, also for the first patch.

Best regards,

Juraj Marcin

> 
> > +#
> >  # @mptcp: enable multi-path TCP.  (Since 6.1)
> >  #
> >  # Since: 1.3
> > @@ -71,6 +75,7 @@
> >      '*ipv4': 'bool',
> >      '*ipv6': 'bool',
> >      '*keep-alive': 'bool',
> > +    '*keep-alive-idle-period': { 'type': 'uint32', 'if': 'HAVE_TCP_KEEPIDLE' },
> >      '*mptcp': { 'type': 'bool', 'if': 'HAVE_IPPROTO_MPTCP' } } }
> >  
> >  ##
> 
> > @@ -697,6 +710,22 @@ int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> >          }
> >          addr->has_keep_alive = true;
> >      }
> > +#ifdef HAVE_TCP_KEEPIDLE
> > +    begin = strstr(optstr, ",keep-alive-idle-period=");
> 
> A bit too verbose - make it just 'keep-alive-idle='
> 
> > +    if (begin) {
> > +        begin += strlen(",keep-alive-idle-period=");
> > +        if (sscanf(begin, "%" PRIu32 "%n", &addr->keep_alive_idle_period, &pos) != 1 ||
> > +            (begin[pos] != '\0' && begin[pos] != ',')) {
> > +            error_setg(errp, "error parsing keep-alive-idle-period argument");
> > +            return -1;
> > +        }
> > +        if (addr->keep_alive_idle_period > INT_MAX) {
> > +            error_setg(errp, "keep-alive-idle-period value is too large");
> > +            return -1;
> > +        }
> > +        addr->has_keep_alive_idle_period = true;
> > +    }
> > +#endif
> >  #ifdef HAVE_IPPROTO_MPTCP
> >      begin = strstr(optstr, ",mptcp");
> >      if (begin) {
> > -- 
> > 2.48.1
> > 
> 
> With 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 :|
> 
Re: [PATCH v2 2/2] utils/qemu-sockets: Introduce keep-alive-idle-period inet socket option
Posted by Vladimir Sementsov-Ogievskiy 1 week, 6 days ago
On 19.03.25 19:36, Juraj Marcin wrote:
> From: Juraj Marcin<jmarcin@redhat.com>
> 
> The default idle period for TCP connection could be even 2 hours.
> However, in some cases, the application needs to be aware of a
> connection issue much sooner.
> 
> This is the case, for example, for postcopy live migration. If there is
> no traffic from the migration destination guest (server-side) to the
> migration source guest (client-side), the destination keeps waiting for
> pages indefinitely and does not switch to the postcopy-paused state.
> This can happen, for example, if the destination QEMU instance is
> started with '-S' command line option and the machine is not started yet
> or if the machine is idle and produces no new page faults for
> not-yet-migrated pages.
> 
> This patch introduces a new inet socket parameter on platforms where
> TCP_KEEPIDLE is defined and passes the configured value to the
> TCP_KEEPIDLE socket option when a client-side or server-side socket is
> created.
> 
> The default value is 0, which means system configuration is used, and no
> custom value is set.
> 
> Signed-off-by: Juraj Marcin<jmarcin@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir