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

[Juan Quintela <quintela@redhat.com>]

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.

Later, Juan.