On Tue, Nov 16, 2021 at 1:08 PM Juan Quintela <quint...@redhat.com> 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?
On RHEL 8, a standard user is created allowing 64kB max locked memory.
(memory seems 'unlimited', though)
>
>
> > 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?
yeah, makes sense.
Moving on v6.
>
> > 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.
IIUC, by following your suggestion and changing this in
migrate_params_check() instead would allow the misconfiguration to be
known before migration is attempted.
That seems the best thing to do here.
>
> 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.
Yeah, I think I should leave the feature testing in here, and move the
parameter testing to migrate_params_check() as commented before.
What do you think?
>
> Later, Juan.
>
Best regards,
Leo
