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

Leonardo Bras posted 6 patches 4 years, 2 months ago
Maintainers: "Dr. David Alan Gilbert" <dgilbert@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Eric Blake <eblake@redhat.com>, Juan Quintela <quintela@redhat.com>, Markus Armbruster <armbru@redhat.com>
There is a newer version of this series
[PATCH v5 4/6] migration: Add zerocopy parameter for QMP/HMP for Linux
Posted by Leonardo Bras 4 years, 2 months ago
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)
+#
 # @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)) {
-- 
2.33.1


Re: [PATCH v5 4/6] migration: Add zerocopy parameter for QMP/HMP for Linux
Posted by Daniel P. Berrangé 4 years, 2 months ago
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
-- 
|: 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
Posted by Leonardo Bras Soares Passos 4 years, 2 months ago
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


Re: [PATCH v5 4/6] migration: Add zerocopy parameter for QMP/HMP for Linux
Posted by Juan Quintela 4 years, 2 months ago
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.


Re: [PATCH v5 4/6] migration: Add zerocopy parameter for QMP/HMP for Linux
Posted by Markus Armbruster 4 years, 2 months ago
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.

[...]


Re: [PATCH v5 4/6] migration: Add zerocopy parameter for QMP/HMP for Linux
Posted by Leonardo Bras Soares Passos 4 years, 2 months ago
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


Re: [PATCH v5 4/6] migration: Add zerocopy parameter for QMP/HMP for Linux
Posted by Leonardo Bras Soares Passos 4 years, 2 months ago
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