| Message ID | 20220426230654.637939-7-leobras@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MSG_ZEROCOPY + multifd | expand |
On Tue, Apr 26, 2022 at 08:06: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 <leobras@redhat.com> > --- > migration/multifd.c | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) > > diff --git a/migration/multifd.c b/migration/multifd.c > index 15fb668e64..07b2e92d8d 100644 > --- a/migration/multifd.c > +++ b/migration/multifd.c > @@ -617,6 +617,7 @@ static void *multifd_send_thread(void *opaque) > MultiFDSendParams *p = opaque; > Error *local_err = NULL; > int ret = 0; > + bool use_zero_copy_send = migrate_use_zero_copy_send(); > > trace_multifd_send_thread_start(p->id); > rcu_register_thread(); > @@ -639,9 +640,14 @@ static void *multifd_send_thread(void *opaque) > if (p->pending_job) { > uint64_t packet_num = p->packet_num; > uint32_t flags = p->flags; > - p->iovs_num = 1; > p->normal_num = 0; > > + if (use_zero_copy_send) { > + p->iovs_num = 0; > + } else { > + p->iovs_num = 1; > + } > + > for (int i = 0; i < p->pages->num; i++) { > p->normal[p->normal_num] = p->pages->offset[i]; > p->normal_num++; > @@ -665,8 +671,19 @@ 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 (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; > + } > + Extra but useless newline.. but not worth a repost. Looks good here: Reviewed-by: Peter Xu <peterx@redhat.com> Thanks, > + } 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, p->iovs_num, > &local_err); > -- > 2.36.0 >
On Tue, Apr 26, 2022 at 08:06: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 <leobras@redhat.com> > --- > migration/multifd.c | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel
On Tue, Apr 26, 2022 at 8:26 PM Peter Xu <peterx@redhat.com> wrote: > > On Tue, Apr 26, 2022 at 08:06: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 <leobras@redhat.com> > > --- > > migration/multifd.c | 23 ++++++++++++++++++++--- > > 1 file changed, 20 insertions(+), 3 deletions(-) > > > > diff --git a/migration/multifd.c b/migration/multifd.c > > index 15fb668e64..07b2e92d8d 100644 > > --- a/migration/multifd.c > > +++ b/migration/multifd.c > > @@ -617,6 +617,7 @@ static void *multifd_send_thread(void *opaque) > > MultiFDSendParams *p = opaque; > > Error *local_err = NULL; > > int ret = 0; > > + bool use_zero_copy_send = migrate_use_zero_copy_send(); > > > > trace_multifd_send_thread_start(p->id); > > rcu_register_thread(); > > @@ -639,9 +640,14 @@ static void *multifd_send_thread(void *opaque) > > if (p->pending_job) { > > uint64_t packet_num = p->packet_num; > > uint32_t flags = p->flags; > > - p->iovs_num = 1; > > p->normal_num = 0; > > > > + if (use_zero_copy_send) { > > + p->iovs_num = 0; > > + } else { > > + p->iovs_num = 1; > > + } > > + > > for (int i = 0; i < p->pages->num; i++) { > > p->normal[p->normal_num] = p->pages->offset[i]; > > p->normal_num++; > > @@ -665,8 +671,19 @@ 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 (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; > > + } > > + > > Extra but useless newline.. but not worth a repost. Looks good here: > > Reviewed-by: Peter Xu <peterx@redhat.com> > > Thanks, Thanks for reviewing Peter! Best regards, Leo > > > + } 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, p->iovs_num, > > &local_err); > > -- > > 2.36.0 > > > > -- > Peter Xu >
On Wed, Apr 27, 2022 at 5:44 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Tue, Apr 26, 2022 at 08:06: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 <leobras@redhat.com> > > --- > > migration/multifd.c | 23 ++++++++++++++++++++--- > > 1 file changed, 20 insertions(+), 3 deletions(-) > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > > With regards, > Daniel Thanks for reviewing Daniel! Best regards, Leo > -- > |: 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 :| >
* Peter Xu (peterx@redhat.com) wrote: > On Tue, Apr 26, 2022 at 08:06: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 <leobras@redhat.com> > > --- > > migration/multifd.c | 23 ++++++++++++++++++++--- > > 1 file changed, 20 insertions(+), 3 deletions(-) > > > > diff --git a/migration/multifd.c b/migration/multifd.c > > index 15fb668e64..07b2e92d8d 100644 > > --- a/migration/multifd.c > > +++ b/migration/multifd.c > > @@ -617,6 +617,7 @@ static void *multifd_send_thread(void *opaque) > > MultiFDSendParams *p = opaque; > > Error *local_err = NULL; > > int ret = 0; > > + bool use_zero_copy_send = migrate_use_zero_copy_send(); > > > > trace_multifd_send_thread_start(p->id); > > rcu_register_thread(); > > @@ -639,9 +640,14 @@ static void *multifd_send_thread(void *opaque) > > if (p->pending_job) { > > uint64_t packet_num = p->packet_num; > > uint32_t flags = p->flags; > > - p->iovs_num = 1; > > p->normal_num = 0; > > > > + if (use_zero_copy_send) { > > + p->iovs_num = 0; > > + } else { > > + p->iovs_num = 1; > > + } > > + > > for (int i = 0; i < p->pages->num; i++) { > > p->normal[p->normal_num] = p->pages->offset[i]; > > p->normal_num++; > > @@ -665,8 +671,19 @@ 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 (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; > > + } > > + > > Extra but useless newline.. but not worth a repost. Looks good here: I removed that. > Reviewed-by: Peter Xu <peterx@redhat.com> > > Thanks, > > > + } 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, p->iovs_num, > > &local_err); > > -- > > 2.36.0 > > > > -- > Peter Xu >
diff --git a/migration/multifd.c b/migration/multifd.c index 15fb668e64..07b2e92d8d 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -617,6 +617,7 @@ static void *multifd_send_thread(void *opaque) MultiFDSendParams *p = opaque; Error *local_err = NULL; int ret = 0; + bool use_zero_copy_send = migrate_use_zero_copy_send(); trace_multifd_send_thread_start(p->id); rcu_register_thread(); @@ -639,9 +640,14 @@ static void *multifd_send_thread(void *opaque) if (p->pending_job) { uint64_t packet_num = p->packet_num; uint32_t flags = p->flags; - p->iovs_num = 1; p->normal_num = 0; + if (use_zero_copy_send) { + p->iovs_num = 0; + } else { + p->iovs_num = 1; + } + for (int i = 0; i < p->pages->num; i++) { p->normal[p->normal_num] = p->pages->offset[i]; p->normal_num++; @@ -665,8 +671,19 @@ 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 (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; + } + + } 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, p->iovs_num, &local_err);
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 <leobras@redhat.com> --- migration/multifd.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)