On Tue, Nov 16, 2021 at 1:35 PM Daniel P. Berrangé <berra...@redhat.com> wrote:
>
> On Tue, Nov 16, 2021 at 04:17:47PM +0000, Daniel P. Berrangé wrote:
> > On Tue, Nov 16, 2021 at 05:08:06PM +0100, Juan Quintela wrote:
> > > Leonardo Bras <leob...@redhat.com> wrote:
> > > > Implement zerocopy on nocomp_send_write(), by making use of QIOChannel
> > > > zerocopy interface.
> > > >
> > > > Change multifd_send_sync_main() so it can distinguish each iteration
> > > > sync from
> > > > the setup and the completion, so a flush_zerocopy() can be called
> > > > at the after each iteration in order to make sure all dirty pages are
> > > > sent
> > > > before a new iteration is started.
> > > >
> > > > Also make it return -1 if flush_zerocopy() fails, in order to cancel
> > > > the migration process, and avoid resuming the guest in the target host
> > > > without receiving all current RAM.
> > > >
> > > > This will work fine on RAM migration because the RAM pages are not
> > > > usually freed,
> > > > and there is no problem on changing the pages content between
> > > > async_send() and
> > > > the actual sending of the buffer, because this change will dirty the
> > > > page and
> > > > cause it to be re-sent on a next iteration anyway.
> > > >
> > > > Given a lot of locked memory may be needed in order to use multid
> > > > migration
> > > > with zerocopy enabled, make it optional by creating a new migration
> > > > parameter
> > > > "zerocopy" on qapi, so low-privileged users can still perform multifd
> > > > migrations.
> > >
> > > How much memory can a non-root program use by default?
> > >
> > >
> > > > static void *multifd_send_thread(void *opaque)
> > > > @@ -853,6 +875,10 @@ static void multifd_new_send_channel_async(QIOTask
> > > > *task, gpointer opaque)
> > > > goto cleanup;
> > > > }
> > > >
> > > > + if (migrate_use_zerocopy()) {
> > > > + p->write_flags = QIO_CHANNEL_WRITE_FLAG_ZEROCOPY;
> > > > + }
> > >
> > > This belongs
> > >
> > >
> > > > p->c = QIO_CHANNEL(sioc);
> > > > qio_channel_set_delay(p->c, false);
> > > > p->running = true;
> > > > @@ -918,6 +944,7 @@ int multifd_save_setup(Error **errp)
> > > > p->packet->version = cpu_to_be32(MULTIFD_VERSION);
> > > > p->name = g_strdup_printf("multifdsend_%d", i);
> > > > p->tls_hostname = g_strdup(s->hostname);
> > > > + p->write_flags = 0;
> > >
> > > here?
> > >
> > > > socket_send_channel_create(multifd_new_send_channel_async, p);
> > > > }
> > > > diff --git a/migration/socket.c b/migration/socket.c
> > > > index e26e94aa0c..8e40e0a3fd 100644
> > > > --- a/migration/socket.c
> > > > +++ b/migration/socket.c
> > > > @@ -78,8 +78,13 @@ static void socket_outgoing_migration(QIOTask *task,
> > > > trace_migration_socket_outgoing_connected(data->hostname);
> > > > }
> > > >
> > > > - if (migrate_use_zerocopy()) {
> > > > - error_setg(&err, "Zerocopy not available in migration");
> > > > + if (migrate_use_zerocopy() &&
> > > > + (!migrate_use_multifd() ||
> > > > + !qio_channel_has_feature(sioc,
> > > > QIO_CHANNEL_FEATURE_WRITE_ZEROCOPY) ||
> > > > + migrate_multifd_compression() != MULTIFD_COMPRESSION_NONE ||
> > > > + migrate_use_tls())) {
> > > > + error_setg(&err,
> > > > + "Zerocopy only available for non-compressed non-TLS
> > > > multifd migration");
> > > > }
> > > >
> > > > migration_channel_connect(data->s, sioc, data->hostname, err);
> > >
> > > Do we really want to do this check here? I think this is really too
> > > late.
> > >
> > > You are not patching migrate_params_check().
> > >
> > > I think that the proper way of doing this is something like:
> > >
> > > if (params->zerocopy &&
> > > (params->parameters.multifd_compression !=
> > > MULTIFD_COMPRESSION_NONE ||
> > > migrate_use_tls())) {
> > > error_setg(&err,
> > > "Zerocopy only available for non-compressed non-TLS
> > > multifd migration");
> > > return false;
> > > }
> > >
> > > You have to do the equivalent of multifd_compression and tls enablement,
> > > to see that zerocopy is not enabled, of course.
> > >
> > > I would prefer to check for QIO_CHANNEL_FEATUR_WRITE_ZEROCPY there, but
> > > I can't see a way of doing that without a qio.
> >
> > I don't think you need to check that feature flag.
> >
> > The combination of zerocopy and compression is simply illogical
> > and can be rejected unconditionally.
>
> Or we could think of "zerocopy" in a more targetted way.
> It is only "zerocopy" in terms the final I/O operation.
> Earlier parts of the process may involve copies. IOW, we
> can copy as part of the compress operation, but skip the
> when then sending the compressed page.
>
> In practice though this is still unlikely to be something
> we can practically do, as we would need to keep compressed
> pages around for an entire migration iteration until we can
> call flush to ensure the data has been sent. This would be
> significant memory burden.
>
> > The combination of zerocopy and TLS is somewhat questionable.
> > You're always going to have a copy between the plain text and
> > cipher text. Avoiding an extra copy for the cipher text will
> > require kerenl work which has no ETA. If it does ever arise,
> > QEMU will need more work again to actually support it. So
> > today we can just reject it unconditonally again.
>
My idea on failing here in the combination of zerocopy & (!multifd ||
TLS || compaction) is just about how it is today.
Meaning if someone wants to use zerocopy + TLS or zerocopy +
compaction in the future, and can provide a good implementation, it
should be ok to change it here (or at migrate_params_check() where
this should be).
>
> 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 :|
>
Best regards,
Leo
