Re: [PATCH v5 6/6] multifd: Implement zerocopy write in multifd migration (multifd-zerocopy)

["Daniel P. Berrangé" <berrange@redhat.com>]

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 <leobras@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.


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 :|