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

[PATCH v3 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 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: Warn 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    | 4 ++++
 7 files changed, 28 insertions(+), 2 deletions(-)

-- 
2.36.1

[PATCH v3 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>
---
 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..698c086b70 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) {
+        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.36.1

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

[Leonardo Bras]

Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 qapi/migration.json   | 7 ++++++-
 migration/migration.c | 2 ++
 monitor/hmp-cmds.c    | 4 ++++
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index 7102e474a6..fed08b9b88 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 zero 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..5f3be9e405 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -307,6 +307,10 @@ 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, "missed zero-copy on: %" PRIu64 " iterations\n",
+                           info->ram->dirty_sync_missed_zero_copy);
+        }
     }
 
     if (info->has_disk) {
-- 
2.36.1

[PATCH v3 3/3] migration/multifd: Warn 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>
---
 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.36.1

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

[Markus Armbruster]

Leonardo Bras <leobras@redhat.com> writes:

> Signed-off-by: Leonardo Bras <leobras@redhat.com>

Acked-by: Markus Armbruster <armbru@redhat.com>

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

[Daniel P. Berrangé]

On Mon, Jul 04, 2022 at 05:23:13PM -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>
> ---
>  io/channel-socket.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

And if this merges via migration maintainers' tree

Acked-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index 4466bb1cd4..698c086b70 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) {
> +        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.36.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 v3 2/3] Add dirty-sync-missed-zero-copy migration stat

[Daniel P. Berrangé]

On Mon, Jul 04, 2022 at 05:23:14PM -0300, Leonardo Bras wrote:
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> ---
>  qapi/migration.json   | 7 ++++++-
>  migration/migration.c | 2 ++
>  monitor/hmp-cmds.c    | 4 ++++
>  3 files changed, 12 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 v3 3/3] migration/multifd: Warn user when zerocopy not working

[Daniel P. Berrangé]

On Mon, Jul 04, 2022 at 05:23:15PM -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>
> ---
>  migration/ram.h     | 2 ++
>  migration/multifd.c | 2 ++
>  migration/ram.c     | 5 +++++
>  3 files changed, 9 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 v3 1/3] QIOChannelSocket: Fix zero-copy flush returning code 1 when nothing sent

[Juan Quintela]

Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Mon, Jul 04, 2022 at 05:23:13PM -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>
>> ---
>>  io/channel-socket.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
> And if this merges via migration maintainers' tree
>
> Acked-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

I will add this on the next PULLL request.

Thanks.


>> 
>> diff --git a/io/channel-socket.c b/io/channel-socket.c
>> index 4466bb1cd4..698c086b70 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) {
>> +        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.36.1
>> 
>
> With regards,
> Daniel

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

[Peter Xu]

Hi, Leo,

On Mon, Jul 04, 2022 at 05:23:13PM -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>
> ---
>  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..698c086b70 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) {

I think I asked this in the downstream review but didn't get a
response.. shouldn't this check be "queued == 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.36.1
> 

-- 
Peter Xu

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

[Peter Xu]

On Mon, Jul 04, 2022 at 05:23:14PM -0300, Leonardo Bras wrote:
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> ---
>  qapi/migration.json   | 7 ++++++-
>  migration/migration.c | 2 ++
>  monitor/hmp-cmds.c    | 4 ++++
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 7102e474a6..fed08b9b88 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 zero pages.  This is between 0

Avoid copying zero pages?  Isn't this for counting MSG_ZEROCOPY fallbacks?

> +#                               and @dirty-sync-count * @multifd-channels.

I'd not name it as "dirty-sync-*" because fundamentally the accounting is
not doing like that (more in latter patch).  I also think we should squash
patch 2/3 as patch 3 only started to provide meaningful values.

> +#                               (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..5f3be9e405 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -307,6 +307,10 @@ 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, "missed zero-copy on: %" PRIu64 " iterations\n",
> +                           info->ram->dirty_sync_missed_zero_copy);

I suggest we don't call it "iterations" because it's not the generic mean
of iterations.

> +        }
>      }
>  
>      if (info->has_disk) {
> -- 
> 2.36.1
> 

-- 
Peter Xu

Re: [PATCH v3 3/3] migration/multifd: Warn user when zerocopy not working

[Peter Xu]

On Mon, Jul 04, 2022 at 05:23:15PM -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>
> ---
>  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();
>              }
>          }
>      }

I know that Juan is working on some patch to only do
multifd_send_sync_main() for each dirty sync, but that's not landed, right?

Can we name it without "dirty-sync" at all (so it'll work before/after
Juan's patch will be applied)?  Something like "zero-copy-send-fallbacks"?

The other thing is the subject may need to be touched up as right now with
the field we don't warn the user anymore on zero-copy-send fallbacks.

Thanks,

> 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.36.1
> 

-- 
Peter Xu

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

[Leonardo Bras Soares Passos]

Hello Peter,

On Thu, Jul 7, 2022 at 2:47 PM Peter Xu <peterx@redhat.com> wrote:
>
> Hi, Leo,
>
> On Mon, Jul 04, 2022 at 05:23:13PM -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>
> > ---
> >  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..698c086b70 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) {
>
> I think I asked this in the downstream review but didn't get a
> response.. shouldn't this check be "queued == sent"?

This is just supposed to skip flush if nothing was queued for sending.
queued == sent is tested bellow in the while part.

Without this, the function could return 1 if nothing was sent with zero-copy,
and it would be confusing, because the QIOChannel API says 1 should be
returned only if all zero-copy sends fell back to copying.

Best regards,
Leo

>
> > +        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.36.1
> >
>
> --
> Peter Xu
>

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

[Leonardo Bras Soares Passos]

Hello Peter,

On Thu, Jul 7, 2022 at 2:54 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Mon, Jul 04, 2022 at 05:23:14PM -0300, Leonardo Bras wrote:
> > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> > ---
> >  qapi/migration.json   | 7 ++++++-
> >  migration/migration.c | 2 ++
> >  monitor/hmp-cmds.c    | 4 ++++
> >  3 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index 7102e474a6..fed08b9b88 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 zero pages.  This is between 0
>
> Avoid copying zero pages?  Isn't this for counting MSG_ZEROCOPY fallbacks?

Yes, sorry, I think I got confused at some point between some cuts & pastes.
It should be "not avoid copying dirty pages." I will fix that on a V4.


>
> > +#                               and @dirty-sync-count * @multifd-channels.
>
> I'd not name it as "dirty-sync-*" because fundamentally the accounting is
> not doing like that (more in latter patch).

Ok, I will take a look & answer there.

> I also think we should squash
> patch 2/3 as patch 3 only started to provide meaningful values.

IIRC Previously in zero-copy-send implementation, I was asked to keep the
property/capability in a separated patch in order to make it easier to review.
So I thought it would be helpful now.

>
> > +#                               (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..5f3be9e405 100644
> > --- a/monitor/hmp-cmds.c
> > +++ b/monitor/hmp-cmds.c
> > @@ -307,6 +307,10 @@ 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, "missed zero-copy on: %" PRIu64 " iterations\n",
> > +                           info->ram->dirty_sync_missed_zero_copy);
>
> I suggest we don't call it "iterations" because it's not the generic mean
> of iterations.

Yeah, I thought that too, but I could not think on anything better.
What do you suggest instead?

Best regards,
Leo

>
> > +        }
> >      }
> >
> >      if (info->has_disk) {
> > --
> > 2.36.1
> >
>
> --
> Peter Xu
>

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

[Peter Xu]

On Thu, Jul 07, 2022 at 04:44:21PM -0300, Leonardo Bras Soares Passos wrote:
> Hello Peter,
> 
> On Thu, Jul 7, 2022 at 2:47 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > Hi, Leo,
> >
> > On Mon, Jul 04, 2022 at 05:23:13PM -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>
> > > ---
> > >  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..698c086b70 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) {
> >
> > I think I asked this in the downstream review but didn't get a
> > response.. shouldn't this check be "queued == sent"?
> 
> This is just supposed to skip flush if nothing was queued for sending.
> queued == sent is tested bellow in the while part.
> 
> Without this, the function could return 1 if nothing was sent with zero-copy,
> and it would be confusing, because the QIOChannel API says 1 should be
> returned only if all zero-copy sends fell back to copying.

I know it won't happen in practise, but.. what if we call flush() without
sending anything zero-copy-wise at all (so zero_copy_sent > 0,
zero_copy_queued > 0, meanwhile zero_copy_sent == zero_copy_queued)?  Then
IIUC we'll return 1 even if we didn't do any fallback, or am I wrong?

> 
> Best regards,
> Leo
> 
> >
> > > +        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.36.1
> > >
> >
> > --
> > Peter Xu
> >
> 

-- 
Peter Xu

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

[Peter Xu]

On Thu, Jul 07, 2022 at 04:50:47PM -0300, Leonardo Bras Soares Passos wrote:
> > I also think we should squash
> > patch 2/3 as patch 3 only started to provide meaningful values.
> 
> IIRC Previously in zero-copy-send implementation, I was asked to keep the
> property/capability in a separated patch in order to make it easier to review.
> So I thought it would be helpful now.

Ah, that's fine then.

> > > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> > > index ca98df0495..5f3be9e405 100644
> > > --- a/monitor/hmp-cmds.c
> > > +++ b/monitor/hmp-cmds.c
> > > @@ -307,6 +307,10 @@ 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, "missed zero-copy on: %" PRIu64 " iterations\n",
> > > +                           info->ram->dirty_sync_missed_zero_copy);
> >
> > I suggest we don't call it "iterations" because it's not the generic mean
> > of iterations.
> 
> Yeah, I thought that too, but I could not think on anything better.
> What do you suggest instead?

"Zero-copy-send fallbacks happened: xxx times\n"?

-- 
Peter Xu

Re: [PATCH v3 3/3] migration/multifd: Warn user when zerocopy not working

[Leonardo Bras Soares Passos]

Hello Peter,

On Thu, Jul 7, 2022 at 2:56 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Mon, Jul 04, 2022 at 05:23:15PM -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>
> > ---
> >  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();
> >              }
> >          }
> >      }
>
> I know that Juan is working on some patch to only do
> multifd_send_sync_main() for each dirty sync, but that's not landed, right?

That's correct, but I am hoping it should land before the release, so
the numbers will match.


>
> Can we name it without "dirty-sync" at all (so it'll work before/after
> Juan's patch will be applied)?  Something like "zero-copy-send-fallbacks"?

It initially was something like that, but on the v2 thread there was
some discussion on
the topic, and it was suggested the number would not mean much to the
user, unless
it was connected to something else.

Markus suggested the connection to @dirty-sync-count right in the
name, and Daniel suggested the above name, which sounds fine to me.

>
> The other thing is the subject may need to be touched up as right now with
> the field we don't warn the user anymore on zero-copy-send fallbacks.

Ok, Warning sounds misleading here.
What do you think about 'report' instead?

Best regards,
Leo

>
> Thanks,
>
> > 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.36.1
> >
>
> --
> Peter Xu
>

Re: [PATCH v3 3/3] migration/multifd: Warn user when zerocopy not working

[Peter Xu]

On Thu, Jul 07, 2022 at 04:59:22PM -0300, Leonardo Bras Soares Passos wrote:
> Hello Peter,
> 
> On Thu, Jul 7, 2022 at 2:56 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Mon, Jul 04, 2022 at 05:23:15PM -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>
> > > ---
> > >  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();
> > >              }
> > >          }
> > >      }
> >
> > I know that Juan is working on some patch to only do
> > multifd_send_sync_main() for each dirty sync, but that's not landed, right?
> 
> That's correct, but I am hoping it should land before the release, so
> the numbers will match.
> 
> 
> >
> > Can we name it without "dirty-sync" at all (so it'll work before/after
> > Juan's patch will be applied)?  Something like "zero-copy-send-fallbacks"?
> 
> It initially was something like that, but on the v2 thread there was
> some discussion on
> the topic, and it was suggested the number would not mean much to the
> user, unless
> it was connected to something else.
> 
> Markus suggested the connection to @dirty-sync-count right in the
> name, and Daniel suggested the above name, which sounds fine to me.

Ah okay.

But then I suggest we make sure it lands only after Juan's.. or it won't
really match.  Also when Juan's patch ready, we'd also double check it will
be exactly called once per iteration, or we can get confusing numbers.  I
assume Juan will take care of that then.

> 
> >
> > The other thing is the subject may need to be touched up as right now with
> > the field we don't warn the user anymore on zero-copy-send fallbacks.
> 
> Ok, Warning sounds misleading here.
> What do you think about 'report' instead?

Looks good.  Thanks,

-- 
Peter Xu

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

[Leonardo Brás]

On Thu, 2022-07-07 at 15:52 -0400, Peter Xu wrote:
> On Thu, Jul 07, 2022 at 04:44:21PM -0300, Leonardo Bras Soares Passos wrote:
> > Hello Peter,
> > 
> > On Thu, Jul 7, 2022 at 2:47 PM Peter Xu <peterx@redhat.com> wrote:
> > > 
> > > Hi, Leo,
> > > 
> > > On Mon, Jul 04, 2022 at 05:23:13PM -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>
> > > > ---
> > > >  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..698c086b70 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) {
> > > 
> > > I think I asked this in the downstream review but didn't get a
> > > response.. shouldn't this check be "queued == sent"?
> > 
> > This is just supposed to skip flush if nothing was queued for sending.
> > queued == sent is tested bellow in the while part.
> > 
> > Without this, the function could return 1 if nothing was sent with zero-
> > copy,
> > and it would be confusing, because the QIOChannel API says 1 should be
> > returned only if all zero-copy sends fell back to copying.
> 
> I know it won't happen in practise, but.. what if we call flush() without
> sending anything zero-copy-wise at all (so zero_copy_sent > 0,
> zero_copy_queued > 0, 

On a no-bug scenario:
- If we don't send anything zero-copy wise, zero_copy_queued will never get
incremented. 
- If we don't get inside the while loop in flush, zero_copy_sent will never be
incremented.


But sure, suppose it does get incremented by bug / mistake: 
-  zero_copy_queued incremented, zero_copy_sent untouched : 
 On (queued == 0) it will go past the 'if', and get stuck 'forever' in the while
loop, since sent will 'never' get incremented.
 On (queued == sent), same.
 On (queued <= sent), same.

-  zero_copy_queued untouched, zero_copy_sent incremented :
 On 'if (queued == 0)' it will not go past the 'if'
 On 'if (queued == sent)', will go past the 'if', but will exit on the 'while',
falsely returning 1.
 On 'if (queued <= sent)', it will not go past the 'if'.

-  zero_copy_queued incremented, zero_copy_sent incremented : 
 On 'if (queued == 0)',  infinite loop as above.
 On 'if (queued == sent)', 
	if sent < queued :  infinite loop , 
	if sent == queued : won't go past 'if' ,
       	if sent > queued :  will go past the 'if', but will exit on the
'while', falsely returning 1.  
 On (queued <= sent), infinite loop if sent < queued, not past if otherwise

BTW, I consider it 'infinite loop', but it could also end up returning -1 on any
error.

> meanwhile zero_copy_sent == zero_copy_queued)?  Then
> IIUC we'll return 1 even if we didn't do any fallback, or am I wrong?

Having 'if(queued == sent)' will cause us to falsely return '1' in two buggy
cases, while 'if queued == 0) will either skip early or go into 'infinite' loop.

Best regards,
Leo

> 
> > 
> > Best regards,
> > Leo
> > 
> > > 
> > > > +        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.36.1
> > > > 
> > > 
> > > --
> > > Peter Xu
> > > 
> > 
> 

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

[Leonardo Brás]

On Thu, 2022-07-07 at 15:56 -0400, Peter Xu wrote:
> On Thu, Jul 07, 2022 at 04:50:47PM -0300, Leonardo Bras Soares Passos wrote:
> > > I also think we should squash
> > > patch 2/3 as patch 3 only started to provide meaningful values.
> > 
> > IIRC Previously in zero-copy-send implementation, I was asked to keep the
> > property/capability in a separated patch in order to make it easier to
> > review.
> > So I thought it would be helpful now.
> 
> Ah, that's fine then.
> 
> > > > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> > > > index ca98df0495..5f3be9e405 100644
> > > > --- a/monitor/hmp-cmds.c
> > > > +++ b/monitor/hmp-cmds.c
> > > > @@ -307,6 +307,10 @@ 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, "missed zero-copy on: %" PRIu64 "
> > > > iterations\n",
> > > > +                           info->ram->dirty_sync_missed_zero_copy);
> > > 
> > > I suggest we don't call it "iterations" because it's not the generic mean
> > > of iterations.
> > 
> > Yeah, I thought that too, but I could not think on anything better.
> > What do you suggest instead?
> 
> "Zero-copy-send fallbacks happened: xxx times\n"?

Oh, yeah, that will work.
I was thinking on keeping the pattern and ended up thinking what was the correct
unit. But this is much simpler and work better.

Best regards,
Leo

> 

Re: [PATCH v3 3/3] migration/multifd: Warn user when zerocopy not working

[Leonardo Brás]

On Thu, 2022-07-07 at 16:06 -0400, Peter Xu wrote:
> On Thu, Jul 07, 2022 at 04:59:22PM -0300, Leonardo Bras Soares Passos wrote:
> > Hello Peter,
> > 
> > On Thu, Jul 7, 2022 at 2:56 PM Peter Xu <peterx@redhat.com> wrote:
> > > 
> > > On Mon, Jul 04, 2022 at 05:23:15PM -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>
> > > > ---
> > > >  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();
> > > >              }
> > > >          }
> > > >      }
> > > 
> > > I know that Juan is working on some patch to only do
> > > multifd_send_sync_main() for each dirty sync, but that's not landed,
> > > right?
> > 
> > That's correct, but I am hoping it should land before the release, so
> > the numbers will match.
> > 
> > 
> > > 
> > > Can we name it without "dirty-sync" at all (so it'll work before/after
> > > Juan's patch will be applied)?  Something like "zero-copy-send-fallbacks"?
> > 
> > It initially was something like that, but on the v2 thread there was
> > some discussion on
> > the topic, and it was suggested the number would not mean much to the
> > user, unless
> > it was connected to something else.
> > 
> > Markus suggested the connection to @dirty-sync-count right in the
> > name, and Daniel suggested the above name, which sounds fine to me.
> 
> Ah okay.
> 
> But then I suggest we make sure it lands only after Juan's.. or it won't
> really match.  Also when Juan's patch ready, we'd also double check it will
> be exactly called once per iteration, or we can get confusing numbers.  I
> assume Juan will take care of that then.
> 
> > 
> > > 
> > > The other thing is the subject may need to be touched up as right now with
> > > the field we don't warn the user anymore on zero-copy-send fallbacks.
> > 
> > Ok, Warning sounds misleading here.
> > What do you think about 'report' instead?
> 
> Looks good.  Thanks,

Thank you for reviewing, Peter!

Leo

> 

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

[Peter Xu]

On Thu, Jul 07, 2022 at 06:14:17PM -0300, Leonardo Brás wrote:
> Having 'if(queued == sent)' will cause us to falsely return '1' in two buggy
> cases, while 'if queued == 0) will either skip early or go into 'infinite' loop.

I'm not sure I strictly follow here..

Imagine the case we do flush() twice without sending anything, then in the
1st flush we'll see queued>sent, we'll finish flush() until queued==sent.
Then in the 2nd (continuous) flush() we'll see queued==sent immediately.

IIUC with the current patch we'll return 1 which I think is wrong because
fallback didn't happen, and if with the change to "if (queued==sent) return
0" it'll fix it?

-- 
Peter Xu

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

[Leonardo Bras Soares Passos]

On Thu, Jul 7, 2022 at 7:18 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Thu, Jul 07, 2022 at 06:14:17PM -0300, Leonardo Brás wrote:
> > Having 'if(queued == sent)' will cause us to falsely return '1' in two buggy
> > cases, while 'if queued == 0) will either skip early or go into 'infinite' loop.
>
> I'm not sure I strictly follow here..
>

Sorry, I was thinking of a different scenario.

> Imagine the case we do flush() twice without sending anything, then in the
> 1st flush we'll see queued>sent, we'll finish flush() until queued==sent.
> Then in the 2nd (continuous) flush() we'll see queued==sent immediately.
>
> IIUC with the current patch we'll return 1 which I think is wrong because
> fallback didn't happen, and if with the change to "if (queued==sent) return
> 0" it'll fix it?

Yes, you are correct.
It's a possible scenario to have a flush happen just after another
without any sending in between.

I will fix it as suggested.

Best regards,
Leo

>
> --
> Peter Xu
>