Hello Juan,
On Fri, Nov 12, 2021 at 8:04 AM Juan Quintela <quint...@redhat.com> wrote:
> Leonardo Bras <leob...@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 <leob...@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
