[PATCH v4 0/3] Zero copy improvements (QIOChannel + multifd)

[PATCH v4 0/3] Zero copy improvements (QIOChannel + multifd)

[Leonardo Bras]

The first patch avoid spuriously returning 1 [*] when zero-copy flush is
attempted before any buffer was sent using MSG_ZEROCOPY.

[*] zero-copy not being used, even though it's enabled and supported
by kernel

The second patch introduces a new migration stat
(dirty-sync-missed-zero-copy) that will be used to keep track of [*]. 

The third patch keeps track of how many zero-copy flushes retured 1 [*]

Changes since v3:
- Patch#1: Also checks if no packet was queued after the last flush
- Patch#2: Improve dirty-sync-missed-zero-copy doc and hmp print message

Changes since v2:
- Documentation release number changed from 7.2 to 7.1
- migration stat renamed from zero-copy-copied to 
  dirty-sync-missed-zero-copy
- Updated documentation to make it more user-friendly

Changes since v1:
- Idea of using a warning replaced by using a migration stat counter



Leonardo Bras (3):
  QIOChannelSocket: Fix zero-copy flush returning code 1 when nothing
    sent
  Add dirty-sync-missed-zero-copy migration stat
  migration/multifd: Report to user when zerocopy not working

 qapi/migration.json   | 7 ++++++-
 migration/ram.h       | 2 ++
 io/channel-socket.c   | 8 +++++++-
 migration/migration.c | 2 ++
 migration/multifd.c   | 2 ++
 migration/ram.c       | 5 +++++
 monitor/hmp-cmds.c    | 5 +++++
 7 files changed, 29 insertions(+), 2 deletions(-)

-- 
2.37.0

[PATCH v4 1/3] QIOChannelSocket: Fix zero-copy flush returning code 1 when nothing sent

[Leonardo Bras]

If flush is called when no buffer was sent with MSG_ZEROCOPY, it currently
returns 1. This return code should be used only when Linux fails to use
MSG_ZEROCOPY on a lot of sendmsg().

Fix this by returning early from flush if no sendmsg(...,MSG_ZEROCOPY)
was attempted.

Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX")
Signed-off-by: Leonardo Bras <leobras@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Acked-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 io/channel-socket.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/io/channel-socket.c b/io/channel-socket.c
index 4466bb1cd4..74a936cc1f 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -716,12 +716,18 @@ static int qio_channel_socket_flush(QIOChannel *ioc,
     struct cmsghdr *cm;
     char control[CMSG_SPACE(sizeof(*serr))];
     int received;
-    int ret = 1;
+    int ret;
+
+    if (sioc->zero_copy_queued == sioc->zero_copy_sent) {
+        return 0;
+    }
 
     msg.msg_control = control;
     msg.msg_controllen = sizeof(control);
     memset(control, 0, sizeof(control));
 
+    ret = 1;
+
     while (sioc->zero_copy_sent < sioc->zero_copy_queued) {
         received = recvmsg(sioc->fd, &msg, MSG_ERRQUEUE);
         if (received < 0) {
-- 
2.37.0

Re: [PATCH v4 1/3] QIOChannelSocket: Fix zero-copy flush returning code 1 when nothing sent

[Peter Xu]

On Mon, Jul 11, 2022 at 06:11:11PM -0300, Leonardo Bras wrote:
> If flush is called when no buffer was sent with MSG_ZEROCOPY, it currently
> returns 1. This return code should be used only when Linux fails to use
> MSG_ZEROCOPY on a lot of sendmsg().
> 
> Fix this by returning early from flush if no sendmsg(...,MSG_ZEROCOPY)
> was attempted.
> 
> Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX")
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Acked-by: Daniel P. Berrangé <berrange@redhat.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH v4 2/3] Add dirty-sync-missed-zero-copy migration stat

[Leonardo Bras]

Signed-off-by: Leonardo Bras <leobras@redhat.com>
Acked-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qapi/migration.json   | 7 ++++++-
 migration/migration.c | 2 ++
 monitor/hmp-cmds.c    | 5 +++++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index 7102e474a6..4a03e8f173 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -55,6 +55,10 @@
 # @postcopy-bytes: The number of bytes sent during the post-copy phase
 #                  (since 7.0).
 #
+# @dirty-sync-missed-zero-copy: Number of times dirty RAM synchronization could
+#                               not avoid copying dirty pages. This is between
+#                               0 and @dirty-sync-count * @multifd-channels.
+#                               (since 7.1)
 # Since: 0.14
 ##
 { 'struct': 'MigrationStats',
@@ -65,7 +69,8 @@
            'postcopy-requests' : 'int', 'page-size' : 'int',
            'multifd-bytes' : 'uint64', 'pages-per-second' : 'uint64',
            'precopy-bytes' : 'uint64', 'downtime-bytes' : 'uint64',
-           'postcopy-bytes' : 'uint64' } }
+           'postcopy-bytes' : 'uint64',
+           'dirty-sync-missed-zero-copy' : 'uint64' } }
 
 ##
 # @XBZRLECacheStats:
diff --git a/migration/migration.c b/migration/migration.c
index 78f5057373..048f7f8bdb 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1027,6 +1027,8 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
     info->ram->normal_bytes = ram_counters.normal * page_size;
     info->ram->mbps = s->mbps;
     info->ram->dirty_sync_count = ram_counters.dirty_sync_count;
+    info->ram->dirty_sync_missed_zero_copy =
+            ram_counters.dirty_sync_missed_zero_copy;
     info->ram->postcopy_requests = ram_counters.postcopy_requests;
     info->ram->page_size = page_size;
     info->ram->multifd_bytes = ram_counters.multifd_bytes;
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index ca98df0495..a6dc79e0d5 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -307,6 +307,11 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
             monitor_printf(mon, "postcopy ram: %" PRIu64 " kbytes\n",
                            info->ram->postcopy_bytes >> 10);
         }
+        if (info->ram->dirty_sync_missed_zero_copy) {
+            monitor_printf(mon,
+                           "Zero-copy-send fallbacks happened: %" PRIu64 " times\n",
+                           info->ram->dirty_sync_missed_zero_copy);
+        }
     }
 
     if (info->has_disk) {
-- 
2.37.0

Re: [PATCH v4 2/3] Add dirty-sync-missed-zero-copy migration stat

[Peter Xu]

On Mon, Jul 11, 2022 at 06:11:12PM -0300, Leonardo Bras wrote:
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> Acked-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  qapi/migration.json   | 7 ++++++-
>  migration/migration.c | 2 ++
>  monitor/hmp-cmds.c    | 5 +++++
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 7102e474a6..4a03e8f173 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -55,6 +55,10 @@
>  # @postcopy-bytes: The number of bytes sent during the post-copy phase
>  #                  (since 7.0).
>  #
> +# @dirty-sync-missed-zero-copy: Number of times dirty RAM synchronization could
> +#                               not avoid copying dirty pages. This is between
> +#                               0 and @dirty-sync-count * @multifd-channels.
> +#                               (since 7.1)
>  # Since: 0.14
>  ##
>  { 'struct': 'MigrationStats',
> @@ -65,7 +69,8 @@
>             'postcopy-requests' : 'int', 'page-size' : 'int',
>             'multifd-bytes' : 'uint64', 'pages-per-second' : 'uint64',
>             'precopy-bytes' : 'uint64', 'downtime-bytes' : 'uint64',
> -           'postcopy-bytes' : 'uint64' } }
> +           'postcopy-bytes' : 'uint64',
> +           'dirty-sync-missed-zero-copy' : 'uint64' } }
>  
>  ##
>  # @XBZRLECacheStats:
> diff --git a/migration/migration.c b/migration/migration.c
> index 78f5057373..048f7f8bdb 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1027,6 +1027,8 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
>      info->ram->normal_bytes = ram_counters.normal * page_size;
>      info->ram->mbps = s->mbps;
>      info->ram->dirty_sync_count = ram_counters.dirty_sync_count;
> +    info->ram->dirty_sync_missed_zero_copy =
> +            ram_counters.dirty_sync_missed_zero_copy;
>      info->ram->postcopy_requests = ram_counters.postcopy_requests;
>      info->ram->page_size = page_size;
>      info->ram->multifd_bytes = ram_counters.multifd_bytes;
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index ca98df0495..a6dc79e0d5 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -307,6 +307,11 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>              monitor_printf(mon, "postcopy ram: %" PRIu64 " kbytes\n",
>                             info->ram->postcopy_bytes >> 10);
>          }
> +        if (info->ram->dirty_sync_missed_zero_copy) {
> +            monitor_printf(mon,
> +                           "Zero-copy-send fallbacks happened: %" PRIu64 " times\n",
> +                           info->ram->dirty_sync_missed_zero_copy);

Thanks, this looks better.  Though I think all the "dirty-sync" wordings
are still kept there, assuming flush() is bound to dirty sync even it's not
yet.  Not a big deal, but let's still keep an eye on the follow up patches..

Acked-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH v4 3/3] migration/multifd: Report to user when zerocopy not working

[Leonardo Bras]

Some errors, like the lack of Scatter-Gather support by the network
interface(NETIF_F_SG) may cause sendmsg(...,MSG_ZEROCOPY) to fail on using
zero-copy, which causes it to fall back to the default copying mechanism.

After each full dirty-bitmap scan there should be a zero-copy flush
happening, which checks for errors each of the previous calls to
sendmsg(...,MSG_ZEROCOPY). If all of them failed to use zero-copy, then
increment dirty_sync_missed_zero_copy migration stat to let the user know
about it.

Signed-off-by: Leonardo Bras <leobras@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 migration/ram.h     | 2 ++
 migration/multifd.c | 2 ++
 migration/ram.c     | 5 +++++
 3 files changed, 9 insertions(+)

diff --git a/migration/ram.h b/migration/ram.h
index ded0a3a086..d3c7eb96f5 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -87,4 +87,6 @@ void ram_write_tracking_prepare(void);
 int ram_write_tracking_start(void);
 void ram_write_tracking_stop(void);
 
+void dirty_sync_missed_zero_copy(void);
+
 #endif
diff --git a/migration/multifd.c b/migration/multifd.c
index 684c014c86..3909b34967 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -624,6 +624,8 @@ int multifd_send_sync_main(QEMUFile *f)
             if (ret < 0) {
                 error_report_err(err);
                 return -1;
+            } else if (ret == 1) {
+                dirty_sync_missed_zero_copy();
             }
         }
     }
diff --git a/migration/ram.c b/migration/ram.c
index 01f9cc1d72..db948c4787 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -407,6 +407,11 @@ static void ram_transferred_add(uint64_t bytes)
     ram_counters.transferred += bytes;
 }
 
+void dirty_sync_missed_zero_copy(void)
+{
+    ram_counters.dirty_sync_missed_zero_copy++;
+}
+
 /* used by the search for pages to send */
 struct PageSearchStatus {
     /* Current block being searched */
-- 
2.37.0

Re: [PATCH v4 3/3] migration/multifd: Report to user when zerocopy not working

[Peter Xu]

On Mon, Jul 11, 2022 at 06:11:13PM -0300, Leonardo Bras wrote:
> Some errors, like the lack of Scatter-Gather support by the network
> interface(NETIF_F_SG) may cause sendmsg(...,MSG_ZEROCOPY) to fail on using
> zero-copy, which causes it to fall back to the default copying mechanism.
> 
> After each full dirty-bitmap scan there should be a zero-copy flush
> happening, which checks for errors each of the previous calls to
> sendmsg(...,MSG_ZEROCOPY). If all of them failed to use zero-copy, then
> increment dirty_sync_missed_zero_copy migration stat to let the user know
> about it.
> 
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Acked-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v4 3/3] migration/multifd: Report to user when zerocopy not working

[Leonardo Bras Soares Passos]

On Tue, Jul 12, 2022 at 7:42 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Mon, Jul 11, 2022 at 06:11:13PM -0300, Leonardo Bras wrote:
> > Some errors, like the lack of Scatter-Gather support by the network
> > interface(NETIF_F_SG) may cause sendmsg(...,MSG_ZEROCOPY) to fail on using
> > zero-copy, which causes it to fall back to the default copying mechanism.
> >
> > After each full dirty-bitmap scan there should be a zero-copy flush
> > happening, which checks for errors each of the previous calls to
> > sendmsg(...,MSG_ZEROCOPY). If all of them failed to use zero-copy, then
> > increment dirty_sync_missed_zero_copy migration stat to let the user know
> > about it.
> >
> > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
> Acked-by: Peter Xu <peterx@redhat.com>

Thanks Peter!

> --
> Peter Xu
>

Re: [PATCH v4 0/3] Zero copy improvements (QIOChannel + multifd)

[Dr. David Alan Gilbert]

* Leonardo Bras (leobras@redhat.com) wrote:
> The first patch avoid spuriously returning 1 [*] when zero-copy flush is
> attempted before any buffer was sent using MSG_ZEROCOPY.
> 
> [*] zero-copy not being used, even though it's enabled and supported
> by kernel
> 
> The second patch introduces a new migration stat
> (dirty-sync-missed-zero-copy) that will be used to keep track of [*]. 
> 
> The third patch keeps track of how many zero-copy flushes retured 1 [*]
> 
> Changes since v3:
> - Patch#1: Also checks if no packet was queued after the last flush
> - Patch#2: Improve dirty-sync-missed-zero-copy doc and hmp print message
> 
> Changes since v2:
> - Documentation release number changed from 7.2 to 7.1
> - migration stat renamed from zero-copy-copied to 
>   dirty-sync-missed-zero-copy
> - Updated documentation to make it more user-friendly
> 
> Changes since v1:
> - Idea of using a warning replaced by using a migration stat counter

Queued
> 
> 
> 
> Leonardo Bras (3):
>   QIOChannelSocket: Fix zero-copy flush returning code 1 when nothing
>     sent
>   Add dirty-sync-missed-zero-copy migration stat
>   migration/multifd: Report to user when zerocopy not working
> 
>  qapi/migration.json   | 7 ++++++-
>  migration/ram.h       | 2 ++
>  io/channel-socket.c   | 8 +++++++-
>  migration/migration.c | 2 ++
>  migration/multifd.c   | 2 ++
>  migration/ram.c       | 5 +++++
>  monitor/hmp-cmds.c    | 5 +++++
>  7 files changed, 29 insertions(+), 2 deletions(-)
> 
> -- 
> 2.37.0
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK