Re: [PATCH v5 4/6] migration: Add zerocopy parameter for QMP/HMP for Linux
Hello Markus, On Fri, Nov 12, 2021 at 9:01 AM Markus Armbruster wrote: > > Juan Quintela writes: > > > Leonardo Bras 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 > > > > 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. > > Rename to zero-copy while there. QAPI/QMP strongly prefer separating > words with dashes. "zerocopy" is not a word, "zero" and "copy" are. > > [...] > Fine then. To make sure it does not look strange, I will change the naming for all the code (zerocopy becomes zero-copy or zero_copy according to the context). Thanks for reviewing! Best regards, Leo
Re: [PATCH v5 4/6] migration: Add zerocopy parameter for QMP/HMP for Linux
Hello Markus,
Thanks for sharing this info!
Best regards,
Leo
On Fri, Nov 12, 2021 at 8:59 AM Markus Armbruster wrote:
>
> Daniel P. Berrangé writes:
>
> > On Fri, Nov 12, 2021 at 12:04:33PM +0100, Juan Quintela wrote:
> >> Leonardo Bras 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).
>
Re: [PATCH v5 4/6] migration: Add zerocopy parameter for QMP/HMP for Linux
Hello Daniel,
On Fri, Nov 12, 2021 at 8:05 AM Daniel P. Berrangé wrote:
>
> On Fri, Nov 12, 2021 at 02:10:39AM -0300, Leonardo Bras 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
> > ---
> > qapi/migration.json | 18 ++
> > migration/migration.h | 5 +
> > migration/migration.c | 32
> > migration/multifd.c | 17 +
> > migration/socket.c| 5 +
> > monitor/hmp-cmds.c| 6 ++
> > 6 files changed, 75 insertions(+), 8 deletions(-)
> >
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index bbfd48cf0b..9534c299d7 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -730,6 +730,11 @@
> > # will consume more CPU.
> > # Defaults to 1. (Since 5.0)
> > #
> > +# @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)
>
> Add
>
>Requires that QEMU be permitted to use locked memory for guest
>RAM pages.
>
Done
>
> Also 7.0 since this has missed the 6.2 deadline.
>
Done
>
>
> Both these notes apply to later in this file too
>
Replaced thrice in this file.
>
>
>
> > 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, _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);
> > }
>
> This change is just a code style alteration with no relation to
> zerocopy. Either remove it, or do this change in its own patch
> seprate from zerocopy.
>
Removed.
>
>
> 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 :|
>
Thanks for reviewing.
Best regards,
Leo
Re: [PATCH v5 4/6] migration: Add zerocopy parameter for QMP/HMP for Linux
Hello Juan,
On Fri, Nov 12, 2021 at 8:04 AM Juan Quintela wrote:
> Leonardo Bras 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
>
> 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, _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
Re: [PATCH v5 4/6] migration: Add zerocopy parameter for QMP/HMP for Linux
Juan Quintela writes: > Leonardo Bras 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 > > 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. Rename to zero-copy while there. QAPI/QMP strongly prefer separating words with dashes. "zerocopy" is not a word, "zero" and "copy" are. [...]
Re: [PATCH v5 4/6] migration: Add zerocopy parameter for QMP/HMP for Linux
Daniel P. Berrangé writes:
> On Fri, Nov 12, 2021 at 12:04:33PM +0100, Juan Quintela wrote:
>> Leonardo Bras 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).
Re: [PATCH v5 4/6] migration: Add zerocopy parameter for QMP/HMP for Linux
On Fri, Nov 12, 2021 at 12:04:33PM +0100, Juan Quintela wrote:
> Leonardo Bras 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
>
> 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 :|
Re: [PATCH v5 4/6] migration: Add zerocopy parameter for QMP/HMP for Linux
On Fri, Nov 12, 2021 at 02:10:39AM -0300, Leonardo Bras 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
> ---
> qapi/migration.json | 18 ++
> migration/migration.h | 5 +
> migration/migration.c | 32
> migration/multifd.c | 17 +
> migration/socket.c| 5 +
> monitor/hmp-cmds.c| 6 ++
> 6 files changed, 75 insertions(+), 8 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index bbfd48cf0b..9534c299d7 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -730,6 +730,11 @@
> # will consume more CPU.
> # Defaults to 1. (Since 5.0)
> #
> +# @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)
Add
Requires that QEMU be permitted to use locked memory for guest
RAM pages.
Also 7.0 since this has missed the 6.2 deadline.
Both these notes apply to later in this file too
> 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, _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);
> }
This change is just a code style alteration with no relation to
zerocopy. Either remove it, or do this change in its own patch
seprate from zerocopy.
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 :|
Re: [PATCH v5 4/6] migration: Add zerocopy parameter for QMP/HMP for Linux
Leonardo Bras 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
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.
> 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, _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.
Later, Juan.
[PATCH v5 4/6] migration: Add zerocopy parameter for QMP/HMP for Linux
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
---
qapi/migration.json | 18 ++
migration/migration.h | 5 +
migration/migration.c | 32
migration/multifd.c | 17 +
migration/socket.c| 5 +
monitor/hmp-cmds.c| 6 ++
6 files changed, 75 insertions(+), 8 deletions(-)
diff --git a/qapi/migration.json b/qapi/migration.json
index bbfd48cf0b..9534c299d7 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -730,6 +730,11 @@
# will consume more CPU.
# Defaults to 1. (Since 5.0)
#
+# @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)
+#
# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
#aliases for the purpose of dirty bitmap migration.
Such
#aliases may for example be the corresponding names on
the
@@ -769,6 +774,7 @@
'xbzrle-cache-size', 'max-postcopy-bandwidth',
'max-cpu-throttle', 'multifd-compression',
'multifd-zlib-level' ,'multifd-zstd-level',
+ { 'name': 'zerocopy', 'if' : 'CONFIG_LINUX'},
'block-bitmap-mapping' ] }
##
@@ -895,6 +901,11 @@
# will consume more CPU.
# Defaults to 1. (Since 5.0)
#
+# @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)
+#
# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
#aliases for the purpose of dirty bitmap migration.
Such
#aliases may for example be the corresponding names on
the
@@ -949,6 +960,7 @@
'*multifd-compression': 'MultiFDCompression',
'*multifd-zlib-level': 'uint8',
'*multifd-zstd-level': 'uint8',
+'*zerocopy': { 'type': 'bool', 'if': 'CONFIG_LINUX' },
'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
##
@@ -1095,6 +1107,11 @@
# will consume more CPU.
# Defaults to 1. (Since 5.0)
#
+# @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)
+#
# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
#aliases for the purpose of dirty bitmap migration.
Such
#aliases may for example be the corresponding names on
the
@@ -1147,6 +1164,7 @@
'*multifd-compression': 'MultiFDCompression',
'*multifd-zlib-level': 'uint8',
'*multifd-zstd-level': 'uint8',
+'*zerocopy': { 'type': 'bool', 'if': 'CONFIG_LINUX' },
'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
##
diff --git a/migration/migration.h b/migration/migration.h
index 8130b703eb..e61ef81f26 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -339,6 +339,11 @@ MultiFDCompression migrate_multifd_compression(void);
int migrate_multifd_zlib_level(void);
int migrate_multifd_zstd_level(void);
+#ifdef CONFIG_LINUX
+int migrate_use_zerocopy(void);
+#else
+#define migrate_use_zerocopy() (0)
+#endif
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 =
