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

[Daniel P . Berrangé]

On Fri, Nov 12, 2021 at 12:04:33PM +0100, Juan Quintela 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.
> 
> >  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 :|