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

[Leonardo Bras Soares Passos]

Hello Markus,
Thanks for sharing this info!

Best regards,
Leo

On Fri, Nov 12, 2021 at 8:59 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > On Fri, Nov 12, 2021 at 12:04:33PM +0100, Juan Quintela wrote:
> >> Leonardo Bras <leobras@redhat.com> wrote:
>
> [...]
>
> >> > 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.
>
> Specifically, the conditionals keep @zerocopy out of query-qmp-schema
> (a.k.a. schema introspection) when it's not actually supported.
>
> This lets management applications recognize zero-copy support.
>
> Without conditionals, the only way to probe for it is trying to switch
> it on.  This is inconvenient and error-prone.
>
> Immature ideas to avoid conditionals:
>
> 1. Make *values* conditional, i.e. unconditional false, but true only if
> CONFIG_LINUX.  The QAPI schema language lets you do this for
> enumerations today, but not for bool.
>
> 2. A new kind of conditional that only applies to schema introspection,
> so you can eat your introspection cake and keep the #ifdef-less code
> cake (and the slight binary bloat that comes with it).
>