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

[Leonardo Bras Soares Passos]

On Thu, Dec 9, 2021 at 5:51 AM Leonardo Bras Soares Passos
<leobras@redhat.com> wrote:
>
> Hello Juan,
>
> On Tue, Nov 16, 2021 at 1:08 PM Juan Quintela <quintela@redhat.com> 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;
> >     }
>
> Don't we also need a check for multifd enabled here?
> We could have zerocopy, multifd_compression=none, tls=disabled but it
> will not fail if multifd=disabled.
>
> Is this correct?
>

I did some tests and this case actually seems to not fail, even though
it should.
So IIUC we really need to check for multifd here.

Sending v6.

>
> >
> > 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.
> >
> > Later, Juan.
> >