On Mon, Apr 25, 2022 at 06:50:55PM -0300, Leonardo Bras wrote: > Since d48c3a0445 ("multifd: Use a single writev on the send side"), > sending the header packet and the memory pages happens in the same > writev, which can potentially make the migration faster. > > Using channel-socket as example, this works well with the default copying > mechanism of sendmsg(), but with zero-copy-send=true, it will cause > the migration to often break. > > This happens because the header packet buffer gets reused quite often, > and there is a high chance that by the time the MSG_ZEROCOPY mechanism get > to send the buffer, it has already changed, sending the wrong data and > causing the migration to abort. > > It means that, as it is, the buffer for the header packet is not suitable > for sending with MSG_ZEROCOPY. > > In order to enable zero copy for multifd, send the header packet on an > individual write(), without any flags, and the remanining pages with a > writev(), as it was happening before. This only changes how a migration > with zero-copy-send=true works, not changing any current behavior for > migrations with zero-copy-send=false. > > Signed-off-by: Leonardo Bras <leob...@redhat.com> > --- > migration/multifd.c | 29 ++++++++++++++++++++++++++--- > 1 file changed, 26 insertions(+), 3 deletions(-) > > diff --git a/migration/multifd.c b/migration/multifd.c > index 15fb668e64..6c940aaa98 100644 > --- a/migration/multifd.c > +++ b/migration/multifd.c > @@ -639,6 +639,8 @@ static void *multifd_send_thread(void *opaque) > if (p->pending_job) { > uint64_t packet_num = p->packet_num; > uint32_t flags = p->flags; > + int iov_offset = 0; > +
No need for this if you change: > p->iovs_num = 1; if (!migrate_use_zero_copy_send()) { p->iovs_num = 1; } > p->normal_num = 0; > > @@ -665,15 +667,36 @@ static void *multifd_send_thread(void *opaque) > trace_multifd_send(p->id, packet_num, p->normal_num, flags, > p->next_packet_size); > > - p->iov[0].iov_len = p->packet_len; > - p->iov[0].iov_base = p->packet; > + if (migrate_use_zero_copy_send()) { > + /* Send header without zerocopy */ > + ret = qio_channel_write_all(p->c, (void *)p->packet, > + p->packet_len, &local_err); > + if (ret != 0) { > + break; > + } > + > + if (!p->normal_num) { > + /* No pages will be sent */ > + goto skip_send; > + } Don't need this AFAIK, because the qio_channel_writev_all call will be a no-op if iovs_num is zero > > - ret = qio_channel_writev_all(p->c, p->iov, p->iovs_num, > + /* Skip first iov : header */ > + iov_offset = 1; Don't need to set this > + } 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_all(p->c, p->iov + iov_offset, > + p->iovs_num - iov_offset, > &local_err); This wouldn't need changing if we don't reserve iovs[0] when not required. > + > if (ret != 0) { > break; > } > > +skip_send: > qemu_mutex_lock(&p->mutex); > p->pending_job--; > qemu_mutex_unlock(&p->mutex); > -- > 2.36.0 > With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|