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

[Markus Armbruster]

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).