* [PATCH v4 0/4] MSG_ZEROCOPY fixes & improvements @ 2022-06-20 5:39 Leonardo Bras 2022-06-20 5:39 ` [PATCH v4 1/4] QIOChannelSocket: Introduce assert and reduce ifdefs to improve readability Leonardo Bras ` (3 more replies) 0 siblings, 4 replies; 21+ messages in thread From: Leonardo Bras @ 2022-06-20 5:39 UTC (permalink / raw) To: Daniel P. Berrangé, Juan Quintela, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, Peter Xu, 徐闯 Cc: Leonardo Bras, qemu-devel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v4 1/4] QIOChannelSocket: Introduce assert and reduce ifdefs to improve readability 2022-06-20 5:39 [PATCH v4 0/4] MSG_ZEROCOPY fixes & improvements Leonardo Bras @ 2022-06-20 5:39 ` Leonardo Bras 2022-06-20 8:47 ` Juan Quintela 2022-06-20 15:27 ` Peter Xu 2022-06-20 5:39 ` [PATCH v4 2/4] QIOChannelSocket: Fix zero-copy send so socket flush works Leonardo Bras ` (2 subsequent siblings) 3 siblings, 2 replies; 21+ messages in thread From: Leonardo Bras @ 2022-06-20 5:39 UTC (permalink / raw) To: Daniel P. Berrangé, Juan Quintela, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, Peter Xu, 徐闯 Cc: Leonardo Bras, qemu-devel 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v4 1/4] QIOChannelSocket: Introduce assert and reduce ifdefs to improve readability 2022-06-20 5:39 ` [PATCH v4 1/4] QIOChannelSocket: Introduce assert and reduce ifdefs to improve readability Leonardo Bras @ 2022-06-20 8:47 ` Juan Quintela 2022-06-20 15:27 ` Peter Xu 1 sibling, 0 replies; 21+ messages in thread From: Juan Quintela @ 2022-06-20 8:47 UTC (permalink / raw) To: Leonardo Bras Cc: Daniel P. Berrangé, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, Peter Xu, 徐闯, qemu-devel 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> ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 1/4] QIOChannelSocket: Introduce assert and reduce ifdefs to improve readability 2022-06-20 5:39 ` [PATCH v4 1/4] QIOChannelSocket: Introduce assert and reduce ifdefs to improve readability Leonardo Bras 2022-06-20 8:47 ` Juan Quintela @ 2022-06-20 15:27 ` Peter Xu 1 sibling, 0 replies; 21+ messages in thread From: Peter Xu @ 2022-06-20 15:27 UTC (permalink / raw) To: Leonardo Bras Cc: Daniel P. Berrangé, Juan Quintela, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, 徐闯, qemu-devel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v4 2/4] QIOChannelSocket: Fix zero-copy send so socket flush works 2022-06-20 5:39 [PATCH v4 0/4] MSG_ZEROCOPY fixes & improvements Leonardo Bras 2022-06-20 5:39 ` [PATCH v4 1/4] QIOChannelSocket: Introduce assert and reduce ifdefs to improve readability Leonardo Bras @ 2022-06-20 5:39 ` Leonardo Bras 2022-06-20 8:48 ` Juan Quintela 2022-06-20 15:27 ` Peter Xu 2022-06-20 5:39 ` [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning Leonardo Bras 2022-06-20 5:39 ` [PATCH v4 4/4] migration: Change zero_copy_send from migration parameter to migration capability Leonardo Bras 3 siblings, 2 replies; 21+ messages in thread From: Leonardo Bras @ 2022-06-20 5:39 UTC (permalink / raw) To: Daniel P. Berrangé, Juan Quintela, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, Peter Xu, 徐闯 Cc: Leonardo Bras, qemu-devel 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/4] QIOChannelSocket: Fix zero-copy send so socket flush works 2022-06-20 5:39 ` [PATCH v4 2/4] QIOChannelSocket: Fix zero-copy send so socket flush works Leonardo Bras @ 2022-06-20 8:48 ` Juan Quintela 2022-06-20 15:27 ` Peter Xu 1 sibling, 0 replies; 21+ messages in thread From: Juan Quintela @ 2022-06-20 8:48 UTC (permalink / raw) To: Leonardo Bras Cc: Daniel P. Berrangé, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, Peter Xu, 徐闯, qemu-devel 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> ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/4] QIOChannelSocket: Fix zero-copy send so socket flush works 2022-06-20 5:39 ` [PATCH v4 2/4] QIOChannelSocket: Fix zero-copy send so socket flush works Leonardo Bras 2022-06-20 8:48 ` Juan Quintela @ 2022-06-20 15:27 ` Peter Xu 1 sibling, 0 replies; 21+ messages in thread From: Peter Xu @ 2022-06-20 15:27 UTC (permalink / raw) To: Leonardo Bras Cc: Daniel P. Berrangé, Juan Quintela, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, 徐闯, qemu-devel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning 2022-06-20 5:39 [PATCH v4 0/4] MSG_ZEROCOPY fixes & improvements Leonardo Bras 2022-06-20 5:39 ` [PATCH v4 1/4] QIOChannelSocket: Introduce assert and reduce ifdefs to improve readability Leonardo Bras 2022-06-20 5:39 ` [PATCH v4 2/4] QIOChannelSocket: Fix zero-copy send so socket flush works Leonardo Bras @ 2022-06-20 5:39 ` Leonardo Bras 2022-06-20 9:23 ` Juan Quintela 2022-06-20 5:39 ` [PATCH v4 4/4] migration: Change zero_copy_send from migration parameter to migration capability Leonardo Bras 3 siblings, 1 reply; 21+ messages in thread From: Leonardo Bras @ 2022-06-20 5:39 UTC (permalink / raw) To: Daniel P. Berrangé, Juan Quintela, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, Peter Xu, 徐闯 Cc: Leonardo Bras, qemu-devel 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning 2022-06-20 5:39 ` [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning Leonardo Bras @ 2022-06-20 9:23 ` Juan Quintela 2022-06-20 15:44 ` Peter Xu 2022-06-21 3:26 ` Leonardo Brás 0 siblings, 2 replies; 21+ messages in thread From: Juan Quintela @ 2022-06-20 9:23 UTC (permalink / raw) To: Leonardo Bras Cc: Daniel P. Berrangé, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, Peter Xu, 徐闯, qemu-devel 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning 2022-06-20 9:23 ` Juan Quintela @ 2022-06-20 15:44 ` Peter Xu 2022-06-21 3:35 ` Leonardo Brás 2022-06-21 3:26 ` Leonardo Brás 1 sibling, 1 reply; 21+ messages in thread From: Peter Xu @ 2022-06-20 15:44 UTC (permalink / raw) To: Juan Quintela, Leonardo Bras Soares Passos Cc: Leonardo Bras, Daniel P. Berrangé, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, 徐闯, qemu-devel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning 2022-06-20 15:44 ` Peter Xu @ 2022-06-21 3:35 ` Leonardo Brás 2022-06-21 14:51 ` Juan Quintela 2022-06-21 15:04 ` Peter Xu 0 siblings, 2 replies; 21+ messages in thread From: Leonardo Brás @ 2022-06-21 3:35 UTC (permalink / raw) To: Peter Xu, Juan Quintela, Leonardo Bras Soares Passos Cc: Daniel P. Berrangé, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, 徐闯, qemu-devel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning 2022-06-21 3:35 ` Leonardo Brás @ 2022-06-21 14:51 ` Juan Quintela 2022-06-21 15:09 ` Peter Xu 2022-06-21 15:04 ` Peter Xu 1 sibling, 1 reply; 21+ messages in thread From: Juan Quintela @ 2022-06-21 14:51 UTC (permalink / raw) To: Leonardo Brás Cc: Peter Xu, Leonardo Bras Soares Passos, Daniel P. Berrangé, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, 徐闯, qemu-devel 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning 2022-06-21 14:51 ` Juan Quintela @ 2022-06-21 15:09 ` Peter Xu 0 siblings, 0 replies; 21+ messages in thread From: Peter Xu @ 2022-06-21 15:09 UTC (permalink / raw) To: Juan Quintela Cc: Leonardo Brás, Leonardo Bras Soares Passos, Daniel P. Berrangé, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, 徐闯, qemu-devel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning 2022-06-21 3:35 ` Leonardo Brás 2022-06-21 14:51 ` Juan Quintela @ 2022-06-21 15:04 ` Peter Xu 1 sibling, 0 replies; 21+ messages in thread From: Peter Xu @ 2022-06-21 15:04 UTC (permalink / raw) To: Leonardo Brás Cc: Juan Quintela, Leonardo Bras Soares Passos, Daniel P. Berrangé, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, 徐闯, qemu-devel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning 2022-06-20 9:23 ` Juan Quintela 2022-06-20 15:44 ` Peter Xu @ 2022-06-21 3:26 ` Leonardo Brás 1 sibling, 0 replies; 21+ messages in thread From: Leonardo Brás @ 2022-06-21 3:26 UTC (permalink / raw) To: quintela Cc: Daniel P.Berrangé, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, Peter Xu, 徐闯, qemu-devel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v4 4/4] migration: Change zero_copy_send from migration parameter to migration capability 2022-06-20 5:39 [PATCH v4 0/4] MSG_ZEROCOPY fixes & improvements Leonardo Bras ` (2 preceding siblings ...) 2022-06-20 5:39 ` [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning Leonardo Bras @ 2022-06-20 5:39 ` Leonardo Bras 2022-06-20 5:46 ` Leonardo Bras Soares Passos ` (3 more replies) 3 siblings, 4 replies; 21+ messages in thread From: Leonardo Bras @ 2022-06-20 5:39 UTC (permalink / raw) To: Daniel P. Berrangé, Juan Quintela, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, Peter Xu, 徐闯 Cc: Leonardo Bras, qemu-devel 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v4 4/4] migration: Change zero_copy_send from migration parameter to migration capability 2022-06-20 5:39 ` [PATCH v4 4/4] migration: Change zero_copy_send from migration parameter to migration capability Leonardo Bras @ 2022-06-20 5:46 ` Leonardo Bras Soares Passos 2022-06-20 9:34 ` Juan Quintela ` (2 subsequent siblings) 3 siblings, 0 replies; 21+ messages in thread From: Leonardo Bras Soares Passos @ 2022-06-20 5:46 UTC (permalink / raw) To: Daniel P. Berrangé, Juan Quintela, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, Peter Xu, 徐闯, Jiri Denemark Cc: qemu-devel 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 > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 4/4] migration: Change zero_copy_send from migration parameter to migration capability 2022-06-20 5:39 ` [PATCH v4 4/4] migration: Change zero_copy_send from migration parameter to migration capability Leonardo Bras 2022-06-20 5:46 ` Leonardo Bras Soares Passos @ 2022-06-20 9:34 ` Juan Quintela 2022-06-20 15:31 ` Peter Xu 2022-06-21 12:34 ` Markus Armbruster 3 siblings, 0 replies; 21+ messages in thread From: Juan Quintela @ 2022-06-20 9:34 UTC (permalink / raw) To: Leonardo Bras Cc: Daniel P. Berrangé, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, Peter Xu, 徐闯, qemu-devel 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 4/4] migration: Change zero_copy_send from migration parameter to migration capability 2022-06-20 5:39 ` [PATCH v4 4/4] migration: Change zero_copy_send from migration parameter to migration capability Leonardo Bras 2022-06-20 5:46 ` Leonardo Bras Soares Passos 2022-06-20 9:34 ` Juan Quintela @ 2022-06-20 15:31 ` Peter Xu 2022-06-20 15:51 ` Juan Quintela 2022-06-21 12:34 ` Markus Armbruster 3 siblings, 1 reply; 21+ messages in thread From: Peter Xu @ 2022-06-20 15:31 UTC (permalink / raw) To: Leonardo Bras Cc: Daniel P. Berrangé, Juan Quintela, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, 徐闯, qemu-devel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 4/4] migration: Change zero_copy_send from migration parameter to migration capability 2022-06-20 15:31 ` Peter Xu @ 2022-06-20 15:51 ` Juan Quintela 0 siblings, 0 replies; 21+ messages in thread From: Juan Quintela @ 2022-06-20 15:51 UTC (permalink / raw) To: Peter Xu Cc: Leonardo Bras, Daniel P. Berrangé, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster, 徐闯, qemu-devel 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 4/4] migration: Change zero_copy_send from migration parameter to migration capability 2022-06-20 5:39 ` [PATCH v4 4/4] migration: Change zero_copy_send from migration parameter to migration capability Leonardo Bras ` (2 preceding siblings ...) 2022-06-20 15:31 ` Peter Xu @ 2022-06-21 12:34 ` Markus Armbruster 3 siblings, 0 replies; 21+ messages in thread From: Markus Armbruster @ 2022-06-21 12:34 UTC (permalink / raw) To: Leonardo Bras Cc: Daniel P. Berrangé, Juan Quintela, Dr. David Alan Gilbert, Eric Blake, Peter Xu, 徐闯, qemu-devel 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> ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2022-06-21 15:11 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-20 5:39 [PATCH v4 0/4] MSG_ZEROCOPY fixes & improvements Leonardo Bras 2022-06-20 5:39 ` [PATCH v4 1/4] QIOChannelSocket: Introduce assert and reduce ifdefs to improve readability Leonardo Bras 2022-06-20 8:47 ` Juan Quintela 2022-06-20 15:27 ` Peter Xu 2022-06-20 5:39 ` [PATCH v4 2/4] QIOChannelSocket: Fix zero-copy send so socket flush works Leonardo Bras 2022-06-20 8:48 ` Juan Quintela 2022-06-20 15:27 ` Peter Xu 2022-06-20 5:39 ` [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning Leonardo Bras 2022-06-20 9:23 ` Juan Quintela 2022-06-20 15:44 ` Peter Xu 2022-06-21 3:35 ` Leonardo Brás 2022-06-21 14:51 ` Juan Quintela 2022-06-21 15:09 ` Peter Xu 2022-06-21 15:04 ` Peter Xu 2022-06-21 3:26 ` Leonardo Brás 2022-06-20 5:39 ` [PATCH v4 4/4] migration: Change zero_copy_send from migration parameter to migration capability Leonardo Bras 2022-06-20 5:46 ` Leonardo Bras Soares Passos 2022-06-20 9:34 ` Juan Quintela 2022-06-20 15:31 ` Peter Xu 2022-06-20 15:51 ` Juan Quintela 2022-06-21 12:34 ` Markus Armbruster
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.