Re: [PATCH v5 4/6] migration: Add zerocopy parameter for QMP/HMP for Linux

[Leonardo Bras Soares Passos]

Hello Juan,

On Fri, Nov 12, 2021 at 8:04 AM Juan Quintela <quintela@redhat.com> wrote:

Leonardo Bras <leobras@redhat.com> wrote:
> Add property that allows zerocopy migration of memory pages,
> and also includes a helper function migrate_use_zerocopy() to check
> if it's enabled.
>
> No code is introduced to actually do the migration, but it allow
> future implementations to enable/disable this feature.
>
> On non-Linux builds this parameter is compiled-out.
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

Hi

> +# @zerocopy: Controls behavior on sending memory pages on migration.
> +#            When true, enables a zerocopy mechanism for sending memory
> +#            pages, if host supports it.
> +#            Defaults to false. (Since 6.2)
> +#

This needs to be changed to next release, but not big deal.


> +#ifdef CONFIG_LINUX
> +int migrate_use_zerocopy(void);

Please, return bool

> +#else
> +#define migrate_use_zerocopy() (0)
> +#endif

and false here.

I know, I know.  We are not consistent here, but the preffered way is
the other way.

Ok, changed for v6

 

>  int migrate_use_xbzrle(void);
>  uint64_t migrate_xbzrle_cache_size(void);
>  bool migrate_colo_enabled(void);
> diff --git a/migration/migration.c b/migration/migration.c
> index abaf6f9e3d..add3dabc56 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -886,6 +886,10 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>      params->multifd_zlib_level = s->parameters.multifd_zlib_level;
>      params->has_multifd_zstd_level = true;
>      params->multifd_zstd_level = s->parameters.multifd_zstd_level;
> +#ifdef CONFIG_LINUX
> +    params->has_zerocopy = true;
> +    params->zerocopy = s->parameters.zerocopy;
> +#endif
>      params->has_xbzrle_cache_size = true;
>      params->xbzrle_cache_size = s->parameters.xbzrle_cache_size;
>      params->has_max_postcopy_bandwidth = true;
> @@ -1538,6 +1542,11 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>      if (params->has_multifd_compression) {
>          dest->multifd_compression = params->multifd_compression;
>      }
> +#ifdef CONFIG_LINUX
> +    if (params->has_zerocopy) {
> +        dest->zerocopy = params->zerocopy;
> +    }
> +#endif
>      if (params->has_xbzrle_cache_size) {
>          dest->xbzrle_cache_size = params->xbzrle_cache_size;
>      }
> @@ -1650,6 +1659,11 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>      if (params->has_multifd_compression) {
>          s->parameters.multifd_compression = params->multifd_compression;
>      }
> +#ifdef CONFIG_LINUX
> +    if (params->has_zerocopy) {
> +        s->parameters.zerocopy = params->zerocopy;
> +    }
> +#endif

After seing all this CONFIG_LINUX mess, I am not sure that it is a good
idea to add the parameter only for LINUX.  It appears that it is better
to add it for all OS's and just not allow to set it to true there.

But If QAPI/QOM people preffer that way, I am not going to get into the middle.

> diff --git a/migration/multifd.c b/migration/multifd.c
> index 7c9deb1921..ab8f0f97be 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -854,16 +854,17 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
>      trace_multifd_new_send_channel_async(p->id);
>      if (qio_task_propagate_error(task, &local_err)) {
>          goto cleanup;
> -    } else {
> -        p->c = QIO_CHANNEL(sioc);
> -        qio_channel_set_delay(p->c, false);
> -        p->running = true;
> -        if (!multifd_channel_connect(p, sioc, local_err)) {
> -            goto cleanup;
> -        }
> -        return;
>      }
> 
> +    p->c = QIO_CHANNEL(sioc);
> +    qio_channel_set_delay(p->c, false);
> +    p->running = true;
> +    if (!multifd_channel_connect(p, sioc, local_err)) {
> +        goto cleanup;
> +    }
> +
> +    return;
> +
>  cleanup:
>      multifd_new_send_channel_cleanup(p, sioc, local_err);
>  }

As far as I can see, this chunk is a NOP, and it don't belong to this patch.

Yeah, it made sense in a previous version, but now it doesn't matter anymore.

Removed for v6.

 

Later, Juan.

Thanks,

Leo