[PATCH v4 0/4] MSG_ZEROCOPY fixes & improvements

[PATCH v4 0/4] MSG_ZEROCOPY fixes & improvements

[Leonardo Bras]

Patches 1 & 2 are about a fix needed to make zero-copy flush work.
The code that incremented the sendmsg counter ended up missing in
the last versions of the patchset, causing the flush to never
happen.

Patch 3 is about an improvement on flushing that improves 
sending performance.

Patch 4 is about making zero-copy-send a migration capability,
instead of a migration parameter. Which actually makes more sense
and helps the implementation of the libvirt code.

Leonardo Bras (4):
  QIOChannelSocket: Introduce assert and reduce ifdefs to improve
    readability
  QIOChannelSocket: Fix zero-copy send so socket flush works
  migration: zero-copy flush only at the end of bitmap scanning
  migration: Change zero_copy_send from migration parameter to migration
    capability

 qapi/migration.json   | 33 +++++++-----------------
 migration/multifd.h   |  1 +
 io/channel-socket.c   | 19 ++++++++++----
 migration/migration.c | 52 ++++++++++++++------------------------
 migration/multifd.c   | 58 ++++++++++++++++++++++++-------------------
 migration/ram.c       |  7 ++++++
 monitor/hmp-cmds.c    |  6 -----
 7 files changed, 83 insertions(+), 93 deletions(-)

-- 
2.36.1

[PATCH v4 1/4] QIOChannelSocket: Introduce assert and reduce ifdefs to improve readability

[Leonardo Bras]

During implementation of MSG_ZEROCOPY feature, a lot of #ifdefs were
introduced, particularly at qio_channel_socket_writev().

Rewrite some of those changes so it's easier to read.

Also, introduce an assert to help detect incorrect zero-copy usage is when
it's disabled on build.

Signed-off-by: Leonardo Bras <leobras@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 io/channel-socket.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/io/channel-socket.c b/io/channel-socket.c
index dc9c165de1..dac9e60c20 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -578,11 +578,17 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
         memcpy(CMSG_DATA(cmsg), fds, fdsize);
     }
 
-#ifdef QEMU_MSG_ZEROCOPY
     if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
+#ifdef QEMU_MSG_ZEROCOPY
         sflags = MSG_ZEROCOPY;
-    }
+#else
+        /*
+         * We expect QIOChannel class entry point to have
+         * blocked this code path already
+         */
+        g_assert_unreachable();
 #endif
+    }
 
  retry:
     ret = sendmsg(sioc->fd, &msg, sflags);
@@ -592,15 +598,13 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
             return QIO_CHANNEL_ERR_BLOCK;
         case EINTR:
             goto retry;
-#ifdef QEMU_MSG_ZEROCOPY
         case ENOBUFS:
-            if (sflags & MSG_ZEROCOPY) {
+            if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
                 error_setg_errno(errp, errno,
                                  "Process can't lock enough memory for using MSG_ZEROCOPY");
                 return -1;
             }
             break;
-#endif
         }
 
         error_setg_errno(errp, errno,
-- 
2.36.1

Re: [PATCH v4 1/4] QIOChannelSocket: Introduce assert and reduce ifdefs to improve readability

[Juan Quintela]

Leonardo Bras <leobras@redhat.com> wrote:
> During implementation of MSG_ZEROCOPY feature, a lot of #ifdefs were
> introduced, particularly at qio_channel_socket_writev().
>
> Rewrite some of those changes so it's easier to read.
>
> Also, introduce an assert to help detect incorrect zero-copy usage is when
> it's disabled on build.
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

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

Re: [PATCH v4 1/4] QIOChannelSocket: Introduce assert and reduce ifdefs to improve readability

[Peter Xu]

On Mon, Jun 20, 2022 at 02:39:42AM -0300, Leonardo Bras wrote:
> During implementation of MSG_ZEROCOPY feature, a lot of #ifdefs were
> introduced, particularly at qio_channel_socket_writev().
> 
> Rewrite some of those changes so it's easier to read.
> 
> Also, introduce an assert to help detect incorrect zero-copy usage is when
> it's disabled on build.
> 
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

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

-- 
Peter Xu

[PATCH v4 2/4] QIOChannelSocket: Fix zero-copy send so socket flush works

[Leonardo Bras]

Somewhere between v6 and v7 the of the zero-copy-send patchset a crucial
part of the flushing mechanism got missing: incrementing zero_copy_queued.

Without that, the flushing interface becomes a no-op, and there is no
guarantee the buffer is really sent.

This can go as bad as causing a corruption in RAM during migration.

Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX")
Reported-by: 徐闯 <xuchuangxclwt@bytedance.com>
Signed-off-by: Leonardo Bras <leobras@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 io/channel-socket.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/io/channel-socket.c b/io/channel-socket.c
index dac9e60c20..4fa0402f54 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -611,6 +611,11 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
                          "Unable to write to socket");
         return -1;
     }
+
+    if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
+        sioc->zero_copy_queued++;
+    }
+
     return ret;
 }
 #else /* WIN32 */
-- 
2.36.1

Re: [PATCH v4 2/4] QIOChannelSocket: Fix zero-copy send so socket flush works

[Juan Quintela]

Leonardo Bras <leobras@redhat.com> wrote:
> Somewhere between v6 and v7 the of the zero-copy-send patchset a crucial
> part of the flushing mechanism got missing: incrementing zero_copy_queued.
>
> Without that, the flushing interface becomes a no-op, and there is no
> guarantee the buffer is really sent.
>
> This can go as bad as causing a corruption in RAM during migration.
>
> Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy
> flag & io_flush for CONFIG_LINUX")
> Reported-by: 徐闯 <xuchuangxclwt@bytedance.com>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

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

Re: [PATCH v4 2/4] QIOChannelSocket: Fix zero-copy send so socket flush works

[Peter Xu]

On Mon, Jun 20, 2022 at 02:39:43AM -0300, Leonardo Bras wrote:
> Somewhere between v6 and v7 the of the zero-copy-send patchset a crucial
> part of the flushing mechanism got missing: incrementing zero_copy_queued.
> 
> Without that, the flushing interface becomes a no-op, and there is no
> guarantee the buffer is really sent.
> 
> This can go as bad as causing a corruption in RAM during migration.
> 
> Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX")
> Reported-by: 徐闯 <xuchuangxclwt@bytedance.com>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

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

-- 
Peter Xu

[PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning

[Leonardo Bras]

When sending memory pages with MSG_ZEROCOPY, it's necessary to flush
to make sure all dirty pages are sent before a future version of them
happens to be sent.

Currently, the flush happens every time at the end of ram_save_iterate(),
which usually happens around 20x per second, due to a timeout.

Change so it flushes only after a whole scanning of the dirty bimap,
so it never sends a newer version of a page before an older one, while
avoiding unnecessary overhead.

Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 migration/multifd.h |  1 +
 migration/multifd.c | 58 ++++++++++++++++++++++++++-------------------
 migration/ram.c     |  7 ++++++
 3 files changed, 41 insertions(+), 25 deletions(-)

diff --git a/migration/multifd.h b/migration/multifd.h
index 4d8d89e5e5..e7cbdf1fb4 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -22,6 +22,7 @@ bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp);
 void multifd_recv_sync_main(void);
 int multifd_send_sync_main(QEMUFile *f);
 int multifd_queue_page(QEMUFile *f, RAMBlock *block, ram_addr_t offset);
+int multifd_zero_copy_flush(void);
 
 /* Multifd Compression flags */
 #define MULTIFD_FLAG_SYNC (1 << 0)
diff --git a/migration/multifd.c b/migration/multifd.c
index 9282ab6aa4..ce4220a97d 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -566,10 +566,42 @@ void multifd_save_cleanup(void)
     multifd_send_state = NULL;
 }
 
+/*
+ * Set zero_copy_flush = true for every multifd channel
+ *
+ * When using zero-copy, it's necessary to flush the pages before any of
+ * the pages can be sent again, so we'll make sure the new version of the
+ * pages will always arrive _later_ than the old pages.
+ *
+ * Should be called only after we finished one whole scanning of
+ * all the dirty bitmaps.
+ */
+int multifd_zero_copy_flush(void)
+{
+    int i;
+    Error *local_err = NULL;
+
+    if (!migrate_use_multifd()) {
+        return 0;
+    }
+
+    for (i = 0; i < migrate_multifd_channels(); i++) {
+        MultiFDSendParams *p = &multifd_send_state->params[i];
+        int ret;
+
+        ret = qio_channel_flush(p->c, &local_err);
+        if (ret < 0) {
+            error_report_err(local_err);
+            return ret;
+        }
+    }
+
+    return 0;
+}
+
 int multifd_send_sync_main(QEMUFile *f)
 {
     int i;
-    bool flush_zero_copy;
 
     if (!migrate_use_multifd()) {
         return 0;
@@ -581,19 +613,6 @@ int multifd_send_sync_main(QEMUFile *f)
         }
     }
 
-    /*
-     * When using zero-copy, it's necessary to flush the pages before any of
-     * the pages can be sent again, so we'll make sure the new version of the
-     * pages will always arrive _later_ than the old pages.
-     *
-     * Currently we achieve this by flushing the zero-page requested writes
-     * per ram iteration, but in the future we could potentially optimize it
-     * to be less frequent, e.g. only after we finished one whole scanning of
-     * all the dirty bitmaps.
-     */
-
-    flush_zero_copy = migrate_use_zero_copy_send();
-
     for (i = 0; i < migrate_multifd_channels(); i++) {
         MultiFDSendParams *p = &multifd_send_state->params[i];
 
@@ -615,17 +634,6 @@ int multifd_send_sync_main(QEMUFile *f)
         ram_counters.transferred += p->packet_len;
         qemu_mutex_unlock(&p->mutex);
         qemu_sem_post(&p->sem);
-
-        if (flush_zero_copy && p->c) {
-            int ret;
-            Error *err = NULL;
-
-            ret = qio_channel_flush(p->c, &err);
-            if (ret < 0) {
-                error_report_err(err);
-                return -1;
-            }
-        }
     }
     for (i = 0; i < migrate_multifd_channels(); i++) {
         MultiFDSendParams *p = &multifd_send_state->params[i];
diff --git a/migration/ram.c b/migration/ram.c
index 5f5e37f64d..514584e44f 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2288,6 +2288,13 @@ static int ram_find_and_save_block(RAMState *rs)
     rs->last_seen_block = pss.block;
     rs->last_page = pss.page;
 
+    if (pss.complete_round && migrate_use_zero_copy_send()) {
+        int ret = multifd_zero_copy_flush();
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
     return pages;
 }
 
-- 
2.36.1

Re: [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning

[Juan Quintela]

Leonardo Bras <leobras@redhat.com> wrote:
> When sending memory pages with MSG_ZEROCOPY, it's necessary to flush
> to make sure all dirty pages are sent before a future version of them
> happens to be sent.
>
> Currently, the flush happens every time at the end of ram_save_iterate(),
> which usually happens around 20x per second, due to a timeout.
>
> Change so it flushes only after a whole scanning of the dirty bimap,
> so it never sends a newer version of a page before an older one, while
> avoiding unnecessary overhead.
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

I agree that previous one is too many flushes, but this one changes to too
much memory to be uncommitted, and that is important because otherwise we
have unlimited amount of dirty memory.

> +/*
> + * Set zero_copy_flush = true for every multifd channel
> + *
> + * When using zero-copy, it's necessary to flush the pages before any of
> + * the pages can be sent again, so we'll make sure the new version of the
> + * pages will always arrive _later_ than the old pages.
> + *
> + * Should be called only after we finished one whole scanning of
> + * all the dirty bitmaps.
> + */
> +int multifd_zero_copy_flush(void)
> +{
> +    int i;
> +    Error *local_err = NULL;
> +
> +    if (!migrate_use_multifd()) {
> +        return 0;
> +    }
> +
> +    for (i = 0; i < migrate_multifd_channels(); i++) {
> +        MultiFDSendParams *p = &multifd_send_state->params[i];
> +        int ret;
> +
> +        ret = qio_channel_flush(p->c, &local_err);
> +        if (ret < 0) {
> +            error_report_err(local_err);
> +            return ret;
> +        }
> +    }
> +
> +    return 0;
> +}


Here you flush every channel, Only at the end of a range you want to do this.


>  int multifd_send_sync_main(QEMUFile *f)
>  {
>      int i;
> -    bool flush_zero_copy;
>  
>      if (!migrate_use_multifd()) {
>          return 0;
> @@ -581,19 +613,6 @@ int multifd_send_sync_main(QEMUFile *f)
>          }
>      }
>  
> -    /*
> -     * When using zero-copy, it's necessary to flush the pages before any of
> -     * the pages can be sent again, so we'll make sure the new version of the
> -     * pages will always arrive _later_ than the old pages.
> -     *
> -     * Currently we achieve this by flushing the zero-page requested writes
> -     * per ram iteration, but in the future we could potentially optimize it
> -     * to be less frequent, e.g. only after we finished one whole scanning of
> -     * all the dirty bitmaps.
> -     */
> -
> -    flush_zero_copy = migrate_use_zero_copy_send();
> -
>      for (i = 0; i < migrate_multifd_channels(); i++) {
>          MultiFDSendParams *p = &multifd_send_state->params[i];
>  
> @@ -615,17 +634,6 @@ int multifd_send_sync_main(QEMUFile *f)
>          ram_counters.transferred += p->packet_len;
>          qemu_mutex_unlock(&p->mutex);
>          qemu_sem_post(&p->sem);
> -
> -        if (flush_zero_copy && p->c) {
> -            int ret;
> -            Error *err = NULL;
> -
> -            ret = qio_channel_flush(p->c, &err);
> -            if (ret < 0) {
> -                error_report_err(err);
> -                return -1;
> -            }
> -        }

This synchronization already happens once every iteration through all
ram.

</me checks how>

And low and behold, it doesn't.

The problem here is that we are calling multifd_send_sync_main() in the
wrong place, i.e. we are being too conservative.

We need to call multifd_send_sync_main() just before doing
migration_bitmap_sync().  The reason that we need to call that function
is exactly the same that we need to flush for zero_copy.

So, what we need to change is remove the call to
multifd_send_sync_main(), not how it handles zero_copy.

>      }
>      for (i = 0; i < migrate_multifd_channels(); i++) {
>          MultiFDSendParams *p = &multifd_send_state->params[i];
> diff --git a/migration/ram.c b/migration/ram.c
> index 5f5e37f64d..514584e44f 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2288,6 +2288,13 @@ static int ram_find_and_save_block(RAMState *rs)
>      rs->last_seen_block = pss.block;
>      rs->last_page = pss.page;
>  
> +    if (pss.complete_round && migrate_use_zero_copy_send()) {
> +        int ret = multifd_zero_copy_flush();
> +        if (ret < 0) {
> +            return ret;
> +        }
> +    }
> +

This place is not right either, because we can have a sync in the middle
for other reasons.

We call migration_bitmap_sync() in save_live_pending(), and that is not
when we finish the complete_round.

Once discussed this, what I asked in the past is that you are having too
much dirty memory on zero_copy.  When you have a Multiterabyte guest, in
a single round you have a "potentially" dirty memory on each channel of:

   total_amount_memory / number of channels.

In a Multiterabyte guest, this is going to be more that probably in the
dozens of gigabytes.  As far as I know there is no card/driver that will
benefit for so many pages in zero_copy, and kernel will move to
synchronous copy at some point.  (In older threads, daniel showed how to
test for this case).

What I proposed is that you check in the migration_send_thread() how
much memory you have written since last synchronization.  Once that it
is big enough (I don't know the limits for card, in the docs that you
showed suggested the size is a few megabytes), you just sync at that
point and continue.

You still need to synchronize all threads at bitmap sync, but as said,
that is handled by multifd_send_sync_main(), and we should fix that
instead of changing where zero_copy flushes.

/* Removed not relevant bits of the function */

static void *multifd_send_thread(void *opaque)
{
    size_t zero_copy_bytes_sent = 0;

    ...

    while (true) {

            ....

            trace_multifd_send(p->id, packet_num, p->normal_num, p->zero_num,
                               p->flags, p->next_packet_size);

            if (use_zero_copy_send) {
                /* Send header first, without zerocopy */
                ret = qio_channel_write_all(p->c, (void *)p->packet,
                                            p->packet_len, &local_err);
                if (ret != 0) {
                    break;
                }

****** Note aside

Did you answered my question here of what happens when you do:

write_with_zero_copy(1MB); write_without_zero_copy(4KB); write_with_zero_copy(1MB); write_without_zero_copy(4KB);

My guess is that write_without_zero_copy(4KB) will just flush the
socket.  I can't think how it can work without doing that.
If so, we are not getting what we want.

            } else {
                /* Send header using the same writev call */
                p->iov[0].iov_len = p->packet_len;
                p->iov[0].iov_base = p->packet;
            }

            ret = qio_channel_writev_full_all(p->c, p->iov, p->iovs_num, NULL,
                                              0, p->write_flags, &local_err);
            if (ret != 0) {
                break;
            }

            qemu_mutex_lock(&p->mutex);
            p->num_packets++;
            p->total_normal_pages += p->normal_num;
            p->total_zero_pages += p->zero_num;
            p->pages->num = 0;
            p->pages->block = NULL;
            p->sent_bytes += p->packet_len;;
            p->sent_bytes += p->next_packet_size;

**** Addition
            zero_copy_bytes_sent += p->packet_len + p->next_packet_size;

            p->pending_job--;
            qemu_mutex_unlock(&p->mutex);
***** Addition
            if (zero_copy_bytes_sent > Threshold) // 2MB/4MB?  I don't know
                    ret = qio_channel_flush(p->c, &local_err);
                    // Handle error somehow
                    //  If you want to be a pro, just change the
                    // Threshold depending on what the kernel answers.
                    // If it has to revert to synchronous sent, just
                    // decrease the threshold, otherwise increase it.

            if (p->flags & MULTIFD_FLAG_SYNC) {
                qemu_sem_post(&p->sem_sync);
            }
            qemu_sem_post(&multifd_send_state->channels_ready);
        } else if (p->quit) {
            qemu_mutex_unlock(&p->mutex);
            break;
        } else {
            qemu_mutex_unlock(&p->mutex);
            /* sometimes there are spurious wakeups */
        }
    }
    .............
}

What do you think?

Later, Juan.

Re: [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning

[Peter Xu]

On Mon, Jun 20, 2022 at 11:23:53AM +0200, Juan Quintela wrote:
> Once discussed this, what I asked in the past is that you are having too
> much dirty memory on zero_copy.  When you have a Multiterabyte guest, in
> a single round you have a "potentially" dirty memory on each channel of:
> 
>    total_amount_memory / number of channels.
> 
> In a Multiterabyte guest, this is going to be more that probably in the
> dozens of gigabytes.  As far as I know there is no card/driver that will
> benefit for so many pages in zero_copy, and kernel will move to
> synchronous copy at some point.  (In older threads, daniel showed how to
> test for this case).

I was wondering whether the kernel needs to cache a lot of messages for
zero copy if we don't flush it for a long time, as recvmsg(MSG_ERRQUEUE)
seems to be fetching one message from the kernel one at a time.  And,
whether that queue has a limit in length or something.

Does it mean that when the kernel could have cached enough of these
messages then it'll fallback to the no-zero-copy mode?  And probably that's
the way how kernel protects itself from using too much buffer for the error
msgs?

This reminded me - Leo, have you considered adding the patch altogether to
detect the "fallback to non-zero-copy" condition?  Because when with it and
when the fallback happens at some point (e.g. when the guest memory is
larger than some value) we'll know.

Thanks,

-- 
Peter Xu

Re: [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning

[Leonardo Brás]

On Mon, 2022-06-20 at 11:44 -0400, Peter Xu wrote:
> On Mon, Jun 20, 2022 at 11:23:53AM +0200, Juan Quintela wrote:
> > Once discussed this, what I asked in the past is that you are having too
> > much dirty memory on zero_copy.  When you have a Multiterabyte guest, in
> > a single round you have a "potentially" dirty memory on each channel of:
> > 
> >    total_amount_memory / number of channels.
> > 
> > In a Multiterabyte guest, this is going to be more that probably in the
> > dozens of gigabytes.  As far as I know there is no card/driver that will
> > benefit for so many pages in zero_copy, and kernel will move to
> > synchronous copy at some point.  (In older threads, daniel showed how to
> > test for this case).
> 
> I was wondering whether the kernel needs to cache a lot of messages for
> zero copy if we don't flush it for a long time, as recvmsg(MSG_ERRQUEUE)
> seems to be fetching one message from the kernel one at a time.  And,
> whether that queue has a limit in length or something.

IIRC, if all messages look the same, it 'merges' them in a single message, like,
'this range has these flags and output'.

So, if no issue happens, we should have a single message with the confirmation
of all sent buffers, meaning just a little memory is used for that.

> 
> Does it mean that when the kernel could have cached enough of these
> messages then it'll fallback to the no-zero-copy mode?  And probably that's
> the way how kernel protects itself from using too much buffer for the error
> msgs?

Since it merges the messages, I don't think it uses a lot of space for that.

IIRC, the kernel will fall back to copying only if the network adapter / driver
does not support MSG_ZEROCOPY, like when it does not support scatter-gather.

> 
> This reminded me - Leo, have you considered adding the patch altogether to
> detect the "fallback to non-zero-copy" condition?  Because when with it and
> when the fallback happens at some point (e.g. when the guest memory is
> larger than some value) we'll know.

I still did not consider that, but sure, how do you see that working?

We can't just disable zero-copy-send because the user actually opted in, so we
could instead add a one time error message for when it falls back to copying, as
it should happen in the first try of zero-copy send.

Or we could fail the migration, stating the interface does not support
MSG_ZEROCOPY, since it should happen in the first sendmsg().

I would personally opt for the last option.

What do you think?

> 
> Thanks,
> 

Thanks Peter!

Best regards,
Leo

Re: [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning

[Juan Quintela]

Leonardo Brás <leobras@redhat.com> wrote:
> On Mon, 2022-06-20 at 11:44 -0400, Peter Xu wrote:
>> On Mon, Jun 20, 2022 at 11:23:53AM +0200, Juan Quintela wrote:
>> > Once discussed this, what I asked in the past is that you are having too
>> > much dirty memory on zero_copy.  When you have a Multiterabyte guest, in
>> > a single round you have a "potentially" dirty memory on each channel of:
>> > 
>> >    total_amount_memory / number of channels.
>> > 
>> > In a Multiterabyte guest, this is going to be more that probably in the
>> > dozens of gigabytes.  As far as I know there is no card/driver that will
>> > benefit for so many pages in zero_copy, and kernel will move to
>> > synchronous copy at some point.  (In older threads, daniel showed how to
>> > test for this case).
>> 
>> I was wondering whether the kernel needs to cache a lot of messages for
>> zero copy if we don't flush it for a long time, as recvmsg(MSG_ERRQUEUE)
>> seems to be fetching one message from the kernel one at a time.  And,
>> whether that queue has a limit in length or something.
>
> IIRC, if all messages look the same, it 'merges' them in a single message, like,
> 'this range has these flags and output'.
>
> So, if no issue happens, we should have a single message with the confirmation
> of all sent buffers, meaning just a little memory is used for that.
>
>> 
>> Does it mean that when the kernel could have cached enough of these
>> messages then it'll fallback to the no-zero-copy mode?  And probably that's
>> the way how kernel protects itself from using too much buffer for the error
>> msgs?
>
> Since it merges the messages, I don't think it uses a lot of space for that.
>
> IIRC, the kernel will fall back to copying only if the network adapter / driver
> does not support MSG_ZEROCOPY, like when it does not support scatter-gather.

My understanding is that it will fallback when you have too much stuff
inflight.

>> 
>> This reminded me - Leo, have you considered adding the patch altogether to
>> detect the "fallback to non-zero-copy" condition?  Because when with it and
>> when the fallback happens at some point (e.g. when the guest memory is
>> larger than some value) we'll know.
>
> I still did not consider that, but sure, how do you see that working?

send with zero_copy(1MB)
send with zero_copy(1MB)
.... (repeat)
at some point kernel decides:
sync all queue()
send synchronously next package.

we are not wondering if the kernel does this (it does).  What we are
wondering is when it does it, i.e. after 1MB worth of writes, 2MB, 10MB
....
That is the thing that depends on kernel/network card/driver.


> We can't just disable zero-copy-send because the user actually opted in, so we
> could instead add a one time error message for when it falls back to copying, as
> it should happen in the first try of zero-copy send.

On your 1st (or second) series, Dan Berrange explained hew to use the
error message interface to detect it.

> Or we could fail the migration, stating the interface does not support
> MSG_ZEROCOPY, since it should happen in the first sendmsg().

> I would personally opt for the last option.
>
> What do you think?

Later, Juan.

Re: [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning

[Peter Xu]

On Tue, Jun 21, 2022 at 04:51:39PM +0200, Juan Quintela wrote:
> > IIRC, the kernel will fall back to copying only if the network adapter / driver
> > does not support MSG_ZEROCOPY, like when it does not support scatter-gather.
> 
> My understanding is that it will fallback when you have too much stuff
> inflight.

I think we'd better figure this out soon, because if so then IMHO we can't
simply fail the migration when the fallback happens..  If we're not sure
about that, we can always be on the safe side to dump an error only.

-- 
Peter Xu

Re: [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning

[Peter Xu]

On Tue, Jun 21, 2022 at 12:35:54AM -0300, Leonardo Brás wrote:
> > This reminded me - Leo, have you considered adding the patch altogether to
> > detect the "fallback to non-zero-copy" condition?  Because when with it and
> > when the fallback happens at some point (e.g. when the guest memory is
> > larger than some value) we'll know.
> 
> I still did not consider that, but sure, how do you see that working?
> 
> We can't just disable zero-copy-send because the user actually opted in, so we
> could instead add a one time error message for when it falls back to copying, as
> it should happen in the first try of zero-copy send.
> 
> Or we could fail the migration, stating the interface does not support
> MSG_ZEROCOPY, since it should happen in the first sendmsg().
> 
> I would personally opt for the last option.
> 
> What do you think?

I don't have a very strong feeling on it, but yes that sounds proper to me.
If one day we'll like zero-copy send be on by default then we'll consider
the other way round, but maybe not necessary for now.  Thanks.

-- 
Peter Xu

Re: [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning

[Leonardo Brás]

On Mon, 2022-06-20 at 11:23 +0200, Juan Quintela wrote:
> Leonardo Bras <leobras@redhat.com> wrote:
> > When sending memory pages with MSG_ZEROCOPY, it's necessary to flush
> > to make sure all dirty pages are sent before a future version of them
> > happens to be sent.
> > 
> > Currently, the flush happens every time at the end of ram_save_iterate(),
> > which usually happens around 20x per second, due to a timeout.
> > 
> > Change so it flushes only after a whole scanning of the dirty bimap,
> > so it never sends a newer version of a page before an older one, while
> > avoiding unnecessary overhead.
> > 
> > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> 
> I agree that previous one is too many flushes, but this one changes to too
> much memory to be uncommitted, and that is important because otherwise we
> have unlimited amount of dirty memory.

I don't quite understand what you meant here.
What does it mean to be uncommitted at this context?
I don't see how we get unlimited amounts of dirty memory here. 

Zero-copy is not supposed to copy memory pages, so let's say all pages are dirty
and enqueued to send, but our network interface is stalling the send:
All memory we should have allocated is VM ram pagecount x sizeof(iov), because
then we flush.

Unless... are you referring to locked memory (as dirty enqueued memory -> locked
memory)? 
That would be a point: If we enqueue more memory than the locked memory amount
our user support, the migration will fail. 

But that would mean a very fast CPU (lots of sendmsgs) and a very slow network
interface. 

> 
> > +/*
> > + * Set zero_copy_flush = true for every multifd channel
> > + *
> > + * When using zero-copy, it's necessary to flush the pages before any of
> > + * the pages can be sent again, so we'll make sure the new version of the
> > + * pages will always arrive _later_ than the old pages.
> > + *
> > + * Should be called only after we finished one whole scanning of
> > + * all the dirty bitmaps.
> > + */
> > +int multifd_zero_copy_flush(void)
> > +{
> > +    int i;
> > +    Error *local_err = NULL;
> > +
> > +    if (!migrate_use_multifd()) {
> > +        return 0;
> > +    }
> > +
> > +    for (i = 0; i < migrate_multifd_channels(); i++) {
> > +        MultiFDSendParams *p = &multifd_send_state->params[i];
> > +        int ret;
> > +
> > +        ret = qio_channel_flush(p->c, &local_err);
> > +        if (ret < 0) {
> > +            error_report_err(local_err);
> > +            return ret;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> 
> 
> Here you flush every channel, Only at the end of a range you want to do this.

Yes, the idea is to flush after a full scan of the dirty-bitmap.

> 
> 
> >  int multifd_send_sync_main(QEMUFile *f)
> >  {
> >      int i;
> > -    bool flush_zero_copy;
> >  
> >      if (!migrate_use_multifd()) {
> >          return 0;
> > @@ -581,19 +613,6 @@ int multifd_send_sync_main(QEMUFile *f)
> >          }
> >      }
> >  
> > -    /*
> > -     * When using zero-copy, it's necessary to flush the pages before any
> > of
> > -     * the pages can be sent again, so we'll make sure the new version of
> > the
> > -     * pages will always arrive _later_ than the old pages.
> > -     *
> > -     * Currently we achieve this by flushing the zero-page requested writes
> > -     * per ram iteration, but in the future we could potentially optimize
> > it
> > -     * to be less frequent, e.g. only after we finished one whole scanning
> > of
> > -     * all the dirty bitmaps.
> > -     */
> > -
> > -    flush_zero_copy = migrate_use_zero_copy_send();
> > -
> >      for (i = 0; i < migrate_multifd_channels(); i++) {
> >          MultiFDSendParams *p = &multifd_send_state->params[i];
> >  
> > @@ -615,17 +634,6 @@ int multifd_send_sync_main(QEMUFile *f)
> >          ram_counters.transferred += p->packet_len;
> >          qemu_mutex_unlock(&p->mutex);
> >          qemu_sem_post(&p->sem);
> > -
> > -        if (flush_zero_copy && p->c) {
> > -            int ret;
> > -            Error *err = NULL;
> > -
> > -            ret = qio_channel_flush(p->c, &err);
> > -            if (ret < 0) {
> > -                error_report_err(err);
> > -                return -1;
> > -            }
> > -        }
> 
> This synchronization already happens once every iteration through all
> ram.
> 
> </me checks how>
> 
> And low and behold, it doesn't.
> 
> The problem here is that we are calling multifd_send_sync_main() in the
> wrong place, i.e. we are being too conservative.
> 
> We need to call multifd_send_sync_main() just before doing
> migration_bitmap_sync().  The reason that we need to call that function
> is exactly the same that we need to flush for zero_copy.
> 
> So, what we need to change is remove the call to
> multifd_send_sync_main(), not how it handles zero_copy.

So, IIUC, multifd have been syncing in a conservative way (i.e. more than
actually needed), and we need to do the same improvement (sync less) for multifd
too, instead of just changing it for zero-copy. Is that correct?

> 
> >      }
> >      for (i = 0; i < migrate_multifd_channels(); i++) {
> >          MultiFDSendParams *p = &multifd_send_state->params[i];
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 5f5e37f64d..514584e44f 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -2288,6 +2288,13 @@ static int ram_find_and_save_block(RAMState *rs)
> >      rs->last_seen_block = pss.block;
> >      rs->last_page = pss.page;
> >  
> > +    if (pss.complete_round && migrate_use_zero_copy_send()) {
> > +        int ret = multifd_zero_copy_flush();
> > +        if (ret < 0) {
> > +            return ret;
> > +        }
> > +    }
> > +
> 
> This place is not right either, because we can have a sync in the middle
> for other reasons.

What do you mean by "in the middle" here? 
Does it mean we should not have multifd code in this function?

> 
> We call migration_bitmap_sync() in save_live_pending(), and that is not
> when we finish the complete_round.

Agree. 
We could add/set a flag in multifd, say in the above area, and then sync/flush
in the correct place (in the future).

But at my experience debugging my code, I found that the loop at
ram_save_iterate() will not stop/break at "bitmap scan end", so we may end up
sending the same page twice before a flush, which is what we want to avoid.

> 
> Once discussed this, what I asked in the past is that you are having too
> much dirty memory on zero_copy.  When you have a Multiterabyte guest, in
> a single round you have a "potentially" dirty memory on each channel of:
> 
>    total_amount_memory / number of channels.

If dirty memory -> locked memory, yes, max_locked_memory == guest_ram.

> 
> In a Multiterabyte guest, this is going to be more that probably in the
> dozens of gigabytes.  As far as I know there is no card/driver that will
> benefit for so many pages in zero_copy, and kernel will move to
> synchronous copy at some point.  (In older threads, daniel showed how to
> test for this case).

The idea of MSG_ZEROCOPY is to avoid copying the pages content over to the
kernel space. I don't recall any reason a network card would drop to copying
instead of keep using zero-copy, unless it does not support it (scatter-gather
not supported, for example), but in this case it would never use zero-copy from
the start. 

IIRC, as long as we have enough locked memory, there should be no problem for
the driver to keep using zero-copy send.

Well, unless you mean the gain of using zero-copy should be irrelevant compared
to the cost of locked memory in some scenarios, say very powerful CPU with slow
network interface, where using zero-copy should not interfere with migration
speed. But that is not a scenario for zero-copy anyway

> 
> What I proposed is that you check in the migration_send_thread() how
> much memory you have written since last synchronization.  Once that it
> is big enough (I don't know the limits for card, in the docs that you
> showed suggested the size is a few megabytes), you just sync at that
> point and continue.

This would help us keep the locked memory under control? yes.
But it would come with the cost of flushing *much* more often than needed.

I mean, it would only be useful if the network card is completely stalling the
send, or if the cpu is scheduling more zero-copy sends than the network card can
process.

The point here is: we don't have to keep track of how much has been sent, but
instead of how much is enqueued (i.e. locked memory). Maybe there is cheap way
to track this value, and we could flush when we detect it's too high.

> 
> You still need to synchronize all threads at bitmap sync, but as said,
> that is handled by multifd_send_sync_main(), and we should fix that
> instead of changing where zero_copy flushes.

Ok, so we keep the zero-copy-flush inside the multifd_send_sync_main() and
change where it's called in order to match the end of the dirty-bitmap scan.
Is that correct?

> 
> /* Removed not relevant bits of the function */
> 
> static void *multifd_send_thread(void *opaque)
> {
>     size_t zero_copy_bytes_sent = 0;
> 
>     ...
> 
>     while (true) {
> 
>             ....
> 
>             trace_multifd_send(p->id, packet_num, p->normal_num, p->zero_num,
>                                p->flags, p->next_packet_size);
> 
>             if (use_zero_copy_send) {
>                 /* Send header first, without zerocopy */
>                 ret = qio_channel_write_all(p->c, (void *)p->packet,
>                                             p->packet_len, &local_err);
>                 if (ret != 0) {
>                     break;
>                 }
> 
> ****** Note aside
> 
> Did you answered my question here of what happens when you do:
> 
> write_with_zero_copy(1MB); write_without_zero_copy(4KB);
> write_with_zero_copy(1MB); write_without_zero_copy(4KB);
> 
> My guess is that write_without_zero_copy(4KB) will just flush the
> socket.  I can't think how it can work without doing that.

To be honest, I did not follow the MSG_ZEROCOPY code to this extent. 

But it depends. If write_without_zero_copy() has MSG_DONTWAIT we could have
everything being sent in a non-blocking way.

But, of course, taking it into account that in multifd we are using a blocking
sendmsg(), I also agree that it is probably flushing the packets previously sent
with MSG_ZEROCOPY (or we could have issues in the receiving part).

But that's an assumption. I still need to dig the code in that aspect to be
sure. I will work on that asap.

> If so, we are not getting what we want.

In fact yes, we are.
We are avoiding copying the buffer, which is what we intend to do, and avoiding
the cost of the 'flush' syscall happening too often. 

If we think multifd is supposed to have blocking sendmsg(), then we are doing
exactly that here, with zero-copy for the heavy part, which is ideal IMHO.

We could try adding MSG_DONTWAIT to the sendmsg() in multifd (which would take
the blocking / synchronous send away), but I have no idea on the impact it would
have at multifd inner workings.

> 
>             } else {
>                 /* Send header using the same writev call */
>                 p->iov[0].iov_len = p->packet_len;
>                 p->iov[0].iov_base = p->packet;
>             }
> 
>             ret = qio_channel_writev_full_all(p->c, p->iov, p->iovs_num, NULL,
>                                               0, p->write_flags, &local_err);
>             if (ret != 0) {
>                 break;
>             }
> 
>             qemu_mutex_lock(&p->mutex);
>             p->num_packets++;
>             p->total_normal_pages += p->normal_num;
>             p->total_zero_pages += p->zero_num;
>             p->pages->num = 0;
>             p->pages->block = NULL;
>             p->sent_bytes += p->packet_len;;
>             p->sent_bytes += p->next_packet_size;
> 
> **** Addition
>             zero_copy_bytes_sent += p->packet_len + p->next_packet_size;
> 
>             p->pending_job--;
>             qemu_mutex_unlock(&p->mutex);
> ***** Addition
>             if (zero_copy_bytes_sent > Threshold) // 2MB/4MB?  I don't know
>                     ret = qio_channel_flush(p->c, &local_err);
>                     // Handle error somehow
>                     //  If you want to be a pro, just change the
>                     // Threshold depending on what the kernel answers.
>                     // If it has to revert to synchronous sent, just
>                     // decrease the threshold, otherwise increase it.
> 
>             if (p->flags & MULTIFD_FLAG_SYNC) {
>                 qemu_sem_post(&p->sem_sync);
>             }
>             qemu_sem_post(&multifd_send_state->channels_ready);
>         } else if (p->quit) {
>             qemu_mutex_unlock(&p->mutex);
>             break;
>         } else {
>             qemu_mutex_unlock(&p->mutex);
>             /* sometimes there are spurious wakeups */
>         }
>     }
>     .............
> }
> 
> What do you think?

It's a good idea to keep locked memory at bay, but I think flushing after N
bytes sent is too often, and will kill the performance. Even more if we assume
the non-zerocopy blocking sendmsg() is already flushing for us. 

> 
> Later, Juan.
> 

Thanks Juan!

Best regards,
Leo

[PATCH v4 4/4] migration: Change zero_copy_send from migration parameter to migration capability

[Leonardo Bras]

When originally implemented, zero_copy_send was designed as a Migration
paramenter.

But taking into account how is that supposed to work, and how
the difference between a capability and a parameter, it only makes sense
that zero-copy-send would work better as a capability.

Taking into account how recently the change got merged, it was decided
that it's still time to make it right, and convert zero_copy_send into
a Migration capability.

Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 qapi/migration.json   | 33 ++++++++-------------------
 migration/migration.c | 52 ++++++++++++++++---------------------------
 monitor/hmp-cmds.c    |  6 -----
 3 files changed, 28 insertions(+), 63 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index 6130cd9fae..baf8d734de 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -461,6 +461,13 @@
 #                       procedure starts. The VM RAM is saved with running VM.
 #                       (since 6.0)
 #
+# @zero-copy-send: Controls behavior on sending memory pages on migration.
+#                  When true, enables a zero-copy mechanism for sending
+#                  memory pages, if host supports it.
+#                  Requires that QEMU be permitted to use locked memory
+#                  for guest RAM pages.
+#                  (since 7.1)
+#
 # Features:
 # @unstable: Members @x-colo and @x-ignore-shared are experimental.
 #
@@ -474,7 +481,8 @@
            'block', 'return-path', 'pause-before-switchover', 'multifd',
            'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
            { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
-           'validate-uuid', 'background-snapshot'] }
+           'validate-uuid', 'background-snapshot',
+           { 'name': 'zero-copy-send', 'if' : 'CONFIG_LINUX'}] }
 
 ##
 # @MigrationCapabilityStatus:
@@ -738,12 +746,6 @@
 #                      will consume more CPU.
 #                      Defaults to 1. (Since 5.0)
 #
-# @zero-copy-send: Controls behavior on sending memory pages on migration.
-#                  When true, enables a zero-copy mechanism for sending
-#                  memory pages, if host supports it.
-#                  Requires that QEMU be permitted to use locked memory
-#                  for guest RAM pages.
-#                  Defaults to false. (Since 7.1)
 #
 # @block-bitmap-mapping: Maps block nodes and bitmaps on them to
 #                        aliases for the purpose of dirty bitmap migration.  Such
@@ -784,7 +786,6 @@
            'xbzrle-cache-size', 'max-postcopy-bandwidth',
            'max-cpu-throttle', 'multifd-compression',
            'multifd-zlib-level' ,'multifd-zstd-level',
-           { 'name': 'zero-copy-send', 'if' : 'CONFIG_LINUX'},
            'block-bitmap-mapping' ] }
 
 ##
@@ -911,13 +912,6 @@
 #                      will consume more CPU.
 #                      Defaults to 1. (Since 5.0)
 #
-# @zero-copy-send: Controls behavior on sending memory pages on migration.
-#                  When true, enables a zero-copy mechanism for sending
-#                  memory pages, if host supports it.
-#                  Requires that QEMU be permitted to use locked memory
-#                  for guest RAM pages.
-#                  Defaults to false. (Since 7.1)
-#
 # @block-bitmap-mapping: Maps block nodes and bitmaps on them to
 #                        aliases for the purpose of dirty bitmap migration.  Such
 #                        aliases may for example be the corresponding names on the
@@ -972,7 +966,6 @@
             '*multifd-compression': 'MultiFDCompression',
             '*multifd-zlib-level': 'uint8',
             '*multifd-zstd-level': 'uint8',
-            '*zero-copy-send': { 'type': 'bool', 'if': 'CONFIG_LINUX' },
             '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
 
 ##
@@ -1119,13 +1112,6 @@
 #                      will consume more CPU.
 #                      Defaults to 1. (Since 5.0)
 #
-# @zero-copy-send: Controls behavior on sending memory pages on migration.
-#                  When true, enables a zero-copy mechanism for sending
-#                  memory pages, if host supports it.
-#                  Requires that QEMU be permitted to use locked memory
-#                  for guest RAM pages.
-#                  Defaults to false. (Since 7.1)
-#
 # @block-bitmap-mapping: Maps block nodes and bitmaps on them to
 #                        aliases for the purpose of dirty bitmap migration.  Such
 #                        aliases may for example be the corresponding names on the
@@ -1178,7 +1164,6 @@
             '*multifd-compression': 'MultiFDCompression',
             '*multifd-zlib-level': 'uint8',
             '*multifd-zstd-level': 'uint8',
-            '*zero-copy-send': { 'type': 'bool', 'if': 'CONFIG_LINUX' },
             '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
 
 ##
diff --git a/migration/migration.c b/migration/migration.c
index 31739b2af9..cc253d66e3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -163,7 +163,8 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
     MIGRATION_CAPABILITY_COMPRESS,
     MIGRATION_CAPABILITY_XBZRLE,
     MIGRATION_CAPABILITY_X_COLO,
-    MIGRATION_CAPABILITY_VALIDATE_UUID);
+    MIGRATION_CAPABILITY_VALIDATE_UUID,
+    MIGRATION_CAPABILITY_ZERO_COPY_SEND);
 
 /* When we add fault tolerance, we could have several
    migrations at once.  For now we don't need to add
@@ -910,10 +911,6 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->multifd_zlib_level = s->parameters.multifd_zlib_level;
     params->has_multifd_zstd_level = true;
     params->multifd_zstd_level = s->parameters.multifd_zstd_level;
-#ifdef CONFIG_LINUX
-    params->has_zero_copy_send = true;
-    params->zero_copy_send = s->parameters.zero_copy_send;
-#endif
     params->has_xbzrle_cache_size = true;
     params->xbzrle_cache_size = s->parameters.xbzrle_cache_size;
     params->has_max_postcopy_bandwidth = true;
@@ -1275,6 +1272,18 @@ static bool migrate_caps_check(bool *cap_list,
         }
     }
 
+#ifdef CONFIG_LINUX
+    if (cap_list[MIGRATION_CAPABILITY_ZERO_COPY_SEND] &&
+        (!cap_list[MIGRATION_CAPABILITY_MULTIFD] ||
+         migrate_use_compression() ||
+         migrate_use_tls())) {
+        error_setg(errp,
+                   "Zero copy only available for non-compressed non-TLS multifd migration");
+        return false;
+    }
+#endif
+
+
     /* incoming side only */
     if (runstate_check(RUN_STATE_INMIGRATE) &&
         !migrate_multi_channels_is_allowed() &&
@@ -1497,16 +1506,6 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
         error_prepend(errp, "Invalid mapping given for block-bitmap-mapping: ");
         return false;
     }
-#ifdef CONFIG_LINUX
-    if (params->zero_copy_send &&
-        (!migrate_use_multifd() ||
-         params->multifd_compression != MULTIFD_COMPRESSION_NONE ||
-         (params->tls_creds && *params->tls_creds))) {
-        error_setg(errp,
-                   "Zero copy only available for non-compressed non-TLS multifd migration");
-        return false;
-    }
-#endif
     return true;
 }
 
@@ -1580,11 +1579,6 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
     if (params->has_multifd_compression) {
         dest->multifd_compression = params->multifd_compression;
     }
-#ifdef CONFIG_LINUX
-    if (params->has_zero_copy_send) {
-        dest->zero_copy_send = params->zero_copy_send;
-    }
-#endif
     if (params->has_xbzrle_cache_size) {
         dest->xbzrle_cache_size = params->xbzrle_cache_size;
     }
@@ -1697,11 +1691,6 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
     if (params->has_multifd_compression) {
         s->parameters.multifd_compression = params->multifd_compression;
     }
-#ifdef CONFIG_LINUX
-    if (params->has_zero_copy_send) {
-        s->parameters.zero_copy_send = params->zero_copy_send;
-    }
-#endif
     if (params->has_xbzrle_cache_size) {
         s->parameters.xbzrle_cache_size = params->xbzrle_cache_size;
         xbzrle_cache_resize(params->xbzrle_cache_size, errp);
@@ -2593,7 +2582,7 @@ bool migrate_use_zero_copy_send(void)
 
     s = migrate_get_current();
 
-    return s->parameters.zero_copy_send;
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_ZERO_COPY_SEND];
 }
 #endif
 
@@ -4249,10 +4238,6 @@ static Property migration_properties[] = {
     DEFINE_PROP_UINT8("multifd-zstd-level", MigrationState,
                       parameters.multifd_zstd_level,
                       DEFAULT_MIGRATE_MULTIFD_ZSTD_LEVEL),
-#ifdef CONFIG_LINUX
-    DEFINE_PROP_BOOL("zero_copy_send", MigrationState,
-                      parameters.zero_copy_send, false),
-#endif
     DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState,
                       parameters.xbzrle_cache_size,
                       DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE),
@@ -4290,6 +4275,10 @@ static Property migration_properties[] = {
     DEFINE_PROP_MIG_CAP("x-multifd", MIGRATION_CAPABILITY_MULTIFD),
     DEFINE_PROP_MIG_CAP("x-background-snapshot",
             MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT),
+#ifdef CONFIG_LINUX
+    DEFINE_PROP_MIG_CAP("x-zero-copy-send",
+            MIGRATION_CAPABILITY_ZERO_COPY_SEND),
+#endif
 
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -4350,9 +4339,6 @@ static void migration_instance_init(Object *obj)
     params->has_multifd_compression = true;
     params->has_multifd_zlib_level = true;
     params->has_multifd_zstd_level = true;
-#ifdef CONFIG_LINUX
-    params->has_zero_copy_send = true;
-#endif
     params->has_xbzrle_cache_size = true;
     params->has_max_postcopy_bandwidth = true;
     params->has_max_cpu_throttle = true;
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 47a27326ee..ca98df0495 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1311,12 +1311,6 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         p->has_multifd_zstd_level = true;
         visit_type_uint8(v, param, &p->multifd_zstd_level, &err);
         break;
-#ifdef CONFIG_LINUX
-    case MIGRATION_PARAMETER_ZERO_COPY_SEND:
-        p->has_zero_copy_send = true;
-        visit_type_bool(v, param, &p->zero_copy_send, &err);
-        break;
-#endif
     case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
         p->has_xbzrle_cache_size = true;
         if (!visit_type_size(v, param, &cache_size, &err)) {
-- 
2.36.1

Re: [PATCH v4 4/4] migration: Change zero_copy_send from migration parameter to migration capability

[Leonardo Bras Soares Passos]

CC: Jiri Denemark <jdenemar@redhat.com>

On Mon, Jun 20, 2022 at 2:40 AM Leonardo Bras <leobras@redhat.com> wrote:
>
> When originally implemented, zero_copy_send was designed as a Migration
> paramenter.
>
> But taking into account how is that supposed to work, and how
> the difference between a capability and a parameter, it only makes sense
> that zero-copy-send would work better as a capability.
>
> Taking into account how recently the change got merged, it was decided
> that it's still time to make it right, and convert zero_copy_send into
> a Migration capability.
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> ---
>  qapi/migration.json   | 33 ++++++++-------------------
>  migration/migration.c | 52 ++++++++++++++++---------------------------
>  monitor/hmp-cmds.c    |  6 -----
>  3 files changed, 28 insertions(+), 63 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 6130cd9fae..baf8d734de 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -461,6 +461,13 @@
>  #                       procedure starts. The VM RAM is saved with running VM.
>  #                       (since 6.0)
>  #
> +# @zero-copy-send: Controls behavior on sending memory pages on migration.
> +#                  When true, enables a zero-copy mechanism for sending
> +#                  memory pages, if host supports it.
> +#                  Requires that QEMU be permitted to use locked memory
> +#                  for guest RAM pages.
> +#                  (since 7.1)
> +#
>  # Features:
>  # @unstable: Members @x-colo and @x-ignore-shared are experimental.
>  #
> @@ -474,7 +481,8 @@
>             'block', 'return-path', 'pause-before-switchover', 'multifd',
>             'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
>             { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
> -           'validate-uuid', 'background-snapshot'] }
> +           'validate-uuid', 'background-snapshot',
> +           { 'name': 'zero-copy-send', 'if' : 'CONFIG_LINUX'}] }
>
>  ##
>  # @MigrationCapabilityStatus:
> @@ -738,12 +746,6 @@
>  #                      will consume more CPU.
>  #                      Defaults to 1. (Since 5.0)
>  #
> -# @zero-copy-send: Controls behavior on sending memory pages on migration.
> -#                  When true, enables a zero-copy mechanism for sending
> -#                  memory pages, if host supports it.
> -#                  Requires that QEMU be permitted to use locked memory
> -#                  for guest RAM pages.
> -#                  Defaults to false. (Since 7.1)
>  #
>  # @block-bitmap-mapping: Maps block nodes and bitmaps on them to
>  #                        aliases for the purpose of dirty bitmap migration.  Such
> @@ -784,7 +786,6 @@
>             'xbzrle-cache-size', 'max-postcopy-bandwidth',
>             'max-cpu-throttle', 'multifd-compression',
>             'multifd-zlib-level' ,'multifd-zstd-level',
> -           { 'name': 'zero-copy-send', 'if' : 'CONFIG_LINUX'},
>             'block-bitmap-mapping' ] }
>
>  ##
> @@ -911,13 +912,6 @@
>  #                      will consume more CPU.
>  #                      Defaults to 1. (Since 5.0)
>  #
> -# @zero-copy-send: Controls behavior on sending memory pages on migration.
> -#                  When true, enables a zero-copy mechanism for sending
> -#                  memory pages, if host supports it.
> -#                  Requires that QEMU be permitted to use locked memory
> -#                  for guest RAM pages.
> -#                  Defaults to false. (Since 7.1)
> -#
>  # @block-bitmap-mapping: Maps block nodes and bitmaps on them to
>  #                        aliases for the purpose of dirty bitmap migration.  Such
>  #                        aliases may for example be the corresponding names on the
> @@ -972,7 +966,6 @@
>              '*multifd-compression': 'MultiFDCompression',
>              '*multifd-zlib-level': 'uint8',
>              '*multifd-zstd-level': 'uint8',
> -            '*zero-copy-send': { 'type': 'bool', 'if': 'CONFIG_LINUX' },
>              '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
>
>  ##
> @@ -1119,13 +1112,6 @@
>  #                      will consume more CPU.
>  #                      Defaults to 1. (Since 5.0)
>  #
> -# @zero-copy-send: Controls behavior on sending memory pages on migration.
> -#                  When true, enables a zero-copy mechanism for sending
> -#                  memory pages, if host supports it.
> -#                  Requires that QEMU be permitted to use locked memory
> -#                  for guest RAM pages.
> -#                  Defaults to false. (Since 7.1)
> -#
>  # @block-bitmap-mapping: Maps block nodes and bitmaps on them to
>  #                        aliases for the purpose of dirty bitmap migration.  Such
>  #                        aliases may for example be the corresponding names on the
> @@ -1178,7 +1164,6 @@
>              '*multifd-compression': 'MultiFDCompression',
>              '*multifd-zlib-level': 'uint8',
>              '*multifd-zstd-level': 'uint8',
> -            '*zero-copy-send': { 'type': 'bool', 'if': 'CONFIG_LINUX' },
>              '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
>
>  ##
> diff --git a/migration/migration.c b/migration/migration.c
> index 31739b2af9..cc253d66e3 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -163,7 +163,8 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
>      MIGRATION_CAPABILITY_COMPRESS,
>      MIGRATION_CAPABILITY_XBZRLE,
>      MIGRATION_CAPABILITY_X_COLO,
> -    MIGRATION_CAPABILITY_VALIDATE_UUID);
> +    MIGRATION_CAPABILITY_VALIDATE_UUID,
> +    MIGRATION_CAPABILITY_ZERO_COPY_SEND);
>
>  /* When we add fault tolerance, we could have several
>     migrations at once.  For now we don't need to add
> @@ -910,10 +911,6 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>      params->multifd_zlib_level = s->parameters.multifd_zlib_level;
>      params->has_multifd_zstd_level = true;
>      params->multifd_zstd_level = s->parameters.multifd_zstd_level;
> -#ifdef CONFIG_LINUX
> -    params->has_zero_copy_send = true;
> -    params->zero_copy_send = s->parameters.zero_copy_send;
> -#endif
>      params->has_xbzrle_cache_size = true;
>      params->xbzrle_cache_size = s->parameters.xbzrle_cache_size;
>      params->has_max_postcopy_bandwidth = true;
> @@ -1275,6 +1272,18 @@ static bool migrate_caps_check(bool *cap_list,
>          }
>      }
>
> +#ifdef CONFIG_LINUX
> +    if (cap_list[MIGRATION_CAPABILITY_ZERO_COPY_SEND] &&
> +        (!cap_list[MIGRATION_CAPABILITY_MULTIFD] ||
> +         migrate_use_compression() ||
> +         migrate_use_tls())) {
> +        error_setg(errp,
> +                   "Zero copy only available for non-compressed non-TLS multifd migration");
> +        return false;
> +    }
> +#endif
> +
> +
>      /* incoming side only */
>      if (runstate_check(RUN_STATE_INMIGRATE) &&
>          !migrate_multi_channels_is_allowed() &&
> @@ -1497,16 +1506,6 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
>          error_prepend(errp, "Invalid mapping given for block-bitmap-mapping: ");
>          return false;
>      }
> -#ifdef CONFIG_LINUX
> -    if (params->zero_copy_send &&
> -        (!migrate_use_multifd() ||
> -         params->multifd_compression != MULTIFD_COMPRESSION_NONE ||
> -         (params->tls_creds && *params->tls_creds))) {
> -        error_setg(errp,
> -                   "Zero copy only available for non-compressed non-TLS multifd migration");
> -        return false;
> -    }
> -#endif
>      return true;
>  }
>
> @@ -1580,11 +1579,6 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>      if (params->has_multifd_compression) {
>          dest->multifd_compression = params->multifd_compression;
>      }
> -#ifdef CONFIG_LINUX
> -    if (params->has_zero_copy_send) {
> -        dest->zero_copy_send = params->zero_copy_send;
> -    }
> -#endif
>      if (params->has_xbzrle_cache_size) {
>          dest->xbzrle_cache_size = params->xbzrle_cache_size;
>      }
> @@ -1697,11 +1691,6 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>      if (params->has_multifd_compression) {
>          s->parameters.multifd_compression = params->multifd_compression;
>      }
> -#ifdef CONFIG_LINUX
> -    if (params->has_zero_copy_send) {
> -        s->parameters.zero_copy_send = params->zero_copy_send;
> -    }
> -#endif
>      if (params->has_xbzrle_cache_size) {
>          s->parameters.xbzrle_cache_size = params->xbzrle_cache_size;
>          xbzrle_cache_resize(params->xbzrle_cache_size, errp);
> @@ -2593,7 +2582,7 @@ bool migrate_use_zero_copy_send(void)
>
>      s = migrate_get_current();
>
> -    return s->parameters.zero_copy_send;
> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_ZERO_COPY_SEND];
>  }
>  #endif
>
> @@ -4249,10 +4238,6 @@ static Property migration_properties[] = {
>      DEFINE_PROP_UINT8("multifd-zstd-level", MigrationState,
>                        parameters.multifd_zstd_level,
>                        DEFAULT_MIGRATE_MULTIFD_ZSTD_LEVEL),
> -#ifdef CONFIG_LINUX
> -    DEFINE_PROP_BOOL("zero_copy_send", MigrationState,
> -                      parameters.zero_copy_send, false),
> -#endif
>      DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState,
>                        parameters.xbzrle_cache_size,
>                        DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE),
> @@ -4290,6 +4275,10 @@ static Property migration_properties[] = {
>      DEFINE_PROP_MIG_CAP("x-multifd", MIGRATION_CAPABILITY_MULTIFD),
>      DEFINE_PROP_MIG_CAP("x-background-snapshot",
>              MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT),
> +#ifdef CONFIG_LINUX
> +    DEFINE_PROP_MIG_CAP("x-zero-copy-send",
> +            MIGRATION_CAPABILITY_ZERO_COPY_SEND),
> +#endif
>
>      DEFINE_PROP_END_OF_LIST(),
>  };
> @@ -4350,9 +4339,6 @@ static void migration_instance_init(Object *obj)
>      params->has_multifd_compression = true;
>      params->has_multifd_zlib_level = true;
>      params->has_multifd_zstd_level = true;
> -#ifdef CONFIG_LINUX
> -    params->has_zero_copy_send = true;
> -#endif
>      params->has_xbzrle_cache_size = true;
>      params->has_max_postcopy_bandwidth = true;
>      params->has_max_cpu_throttle = true;
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 47a27326ee..ca98df0495 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1311,12 +1311,6 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>          p->has_multifd_zstd_level = true;
>          visit_type_uint8(v, param, &p->multifd_zstd_level, &err);
>          break;
> -#ifdef CONFIG_LINUX
> -    case MIGRATION_PARAMETER_ZERO_COPY_SEND:
> -        p->has_zero_copy_send = true;
> -        visit_type_bool(v, param, &p->zero_copy_send, &err);
> -        break;
> -#endif
>      case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
>          p->has_xbzrle_cache_size = true;
>          if (!visit_type_size(v, param, &cache_size, &err)) {
> --
> 2.36.1
>

Re: [PATCH v4 4/4] migration: Change zero_copy_send from migration parameter to migration capability

[Juan Quintela]

Leonardo Bras <leobras@redhat.com> wrote:
> When originally implemented, zero_copy_send was designed as a Migration
> paramenter.
>
> But taking into account how is that supposed to work, and how
> the difference between a capability and a parameter, it only makes sense
> that zero-copy-send would work better as a capability.
>
> Taking into account how recently the change got merged, it was decided
> that it's still time to make it right, and convert zero_copy_send into
> a Migration capability.
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

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


Note to myself: I would really love not to have to write so much
boilerplate code each time that we want to add a parameter/capability.

Adding the name, the documentation, and the check to set it up should be
all that be necesary.

A parameter needs to be defined twiced in qapi, 6 times in migration.c
and another one in hmp.  Capabilities are a better, but still room to improvement.

Later, Juan.

Re: [PATCH v4 4/4] migration: Change zero_copy_send from migration parameter to migration capability

[Peter Xu]

On Mon, Jun 20, 2022 at 02:39:45AM -0300, Leonardo Bras wrote:
> When originally implemented, zero_copy_send was designed as a Migration
> paramenter.
> 
> But taking into account how is that supposed to work, and how
> the difference between a capability and a parameter, it only makes sense
> that zero-copy-send would work better as a capability.
> 
> Taking into account how recently the change got merged, it was decided
> that it's still time to make it right, and convert zero_copy_send into
> a Migration capability.
> 
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

I assume this is a request from Libvirt? We don't have a release yet so
yeah probably we still have time..

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

-- 
Peter Xu

Re: [PATCH v4 4/4] migration: Change zero_copy_send from migration parameter to migration capability

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Mon, Jun 20, 2022 at 02:39:45AM -0300, Leonardo Bras wrote:
>> When originally implemented, zero_copy_send was designed as a Migration
>> paramenter.
>> 
>> But taking into account how is that supposed to work, and how
>> the difference between a capability and a parameter, it only makes sense
>> that zero-copy-send would work better as a capability.
>> 
>> Taking into account how recently the change got merged, it was decided
>> that it's still time to make it right, and convert zero_copy_send into
>> a Migration capability.
>> 
>> Signed-off-by: Leonardo Bras <leobras@redhat.com>
>
> I assume this is a request from Libvirt? We don't have a release yet so
> yeah probably we still have time..
>
> Acked-by: Peter Xu <peterx@redhat.com>

Livirt already have a concept of migration capabilities (bools) that
lets layered products to check/discover/etc.  Putting it there instead
of one parameter makes their life much easier (i.e. more common code).

As this code hasn't been in a stable release yet, I think it is ok.

Later, Juan.

Re: [PATCH v4 4/4] migration: Change zero_copy_send from migration parameter to migration capability

[Markus Armbruster]

Leonardo Bras <leobras@redhat.com> writes:

> When originally implemented, zero_copy_send was designed as a Migration
> paramenter.
>
> But taking into account how is that supposed to work, and how
> the difference between a capability and a parameter, it only makes sense
> that zero-copy-send would work better as a capability.
>
> Taking into account how recently the change got merged, it was decided
> that it's still time to make it right, and convert zero_copy_send into
> a Migration capability.
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

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