| Message ID | 20211112051040.923746-5-leobras@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MSG_ZEROCOPY + multifd | expand |
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. > 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, &local_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.
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 <leobras@redhat.com> > --- > 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, &local_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
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
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).
Juan Quintela <quintela@redhat.com> writes: > 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. Rename to zero-copy while there. QAPI/QMP strongly prefer separating words with dashes. "zerocopy" is not a word, "zero" and "copy" are. [...]
Hello Juan, On Fri, Nov 12, 2021 at 8:04 AM Juan Quintela <quintela@redhat.com> 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. > > 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, &local_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
Hello Daniel, On Fri, Nov 12, 2021 at 8:05 AM Daniel P. Berrangé <berrange@redhat.com> 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 <leobras@redhat.com> > > --- > > 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, &local_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
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). >
Hello Markus, On Fri, Nov 12, 2021 at 9:01 AM Markus Armbruster <armbru@redhat.com> wrote: > > Juan Quintela <quintela@redhat.com> writes: > > > 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. > > 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
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 = 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 if (params->has_xbzrle_cache_size) { s->parameters.xbzrle_cache_size = params->xbzrle_cache_size; xbzrle_cache_resize(params->xbzrle_cache_size, errp); @@ -2540,6 +2554,17 @@ int migrate_multifd_zstd_level(void) return s->parameters.multifd_zstd_level; } +#ifdef CONFIG_LINUX +int migrate_use_zerocopy(void) +{ + MigrationState *s; + + s = migrate_get_current(); + + return s->parameters.zerocopy; +} +#endif + int migrate_use_xbzrle(void) { MigrationState *s; @@ -4190,6 +4215,10 @@ static Property migration_properties[] = { DEFINE_PROP_UINT8("multifd-zstd-level", MigrationState, parameters.multifd_zstd_level, DEFAULT_MIGRATE_MULTIFD_ZSTD_LEVEL), +#ifdef CONFIG_LINUX + DEFINE_PROP_BOOL("zerocopy", MigrationState, + parameters.zerocopy, false), +#endif DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState, parameters.xbzrle_cache_size, DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE), @@ -4287,6 +4316,9 @@ static void migration_instance_init(Object *obj) params->has_multifd_compression = true; params->has_multifd_zlib_level = true; params->has_multifd_zstd_level = true; +#ifdef CONFIG_LINUX + params->has_zerocopy = true; +#endif params->has_xbzrle_cache_size = true; params->has_max_postcopy_bandwidth = true; params->has_max_cpu_throttle = true; 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, &local_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); } diff --git a/migration/socket.c b/migration/socket.c index 05705a32d8..e26e94aa0c 100644 --- a/migration/socket.c +++ b/migration/socket.c @@ -77,6 +77,11 @@ static void socket_outgoing_migration(QIOTask *task, } else { trace_migration_socket_outgoing_connected(data->hostname); } + + if (migrate_use_zerocopy()) { + error_setg(&err, "Zerocopy not available in migration"); + } + migration_channel_connect(data->s, sioc, data->hostname, err); object_unref(OBJECT(sioc)); } diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 9c91bf93e9..442679dcfa 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -1297,6 +1297,12 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) p->has_multifd_zstd_level = true; visit_type_uint8(v, param, &p->multifd_zstd_level, &err); break; +#ifdef CONFIG_LINUX + case MIGRATION_PARAMETER_ZEROCOPY: + p->has_zerocopy = true; + visit_type_bool(v, param, &p->zerocopy, &err); + break; +#endif case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE: p->has_xbzrle_cache_size = true; if (!visit_type_size(v, param, &cache_size, &err)) {
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> --- 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(-)