On Fri, Nov 12, 2021 at 12:04:33PM +0100, Juan Quintela 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.
>
> > 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.
I don't like all the conditionals either, but QAPI design wants the
conditionals, as that allows mgmt apps to query whether the feature
is supported in a build or not.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
