[Qemu-devel] [PATCH 0/3] WIP: Multifd compression support

[Qemu-devel] [PATCH 0/3] WIP: Multifd compression support

[Juan Quintela]

This series create compression code on top of multifd.  It is still
WIP, but it is already:
- faster that current compression code
- it does the minimum amount of copies possible
- we allow support for other compression codes
- it pass the multifd test sent in my previous series

Test for existing code didn't work because code is too slow, I need to
make downtime 10 times bigger to make it to converge on my test
machine.  This code works with same limits that multifd no-

ToDo:
- move printf's  to traces
- move code to a struct instead of if (zlib) inside the main threads.
- improve error handling.

Please, review and coment.

Juan Quintela (3):
  migration: Add multifd-compress parameter
  multifd: compression support variables
  multifd: Start of zlib compression code

 hmp.c                        |  23 +++++-
 hw/core/qdev-properties.c    |  11 +++
 include/hw/qdev-properties.h |   1 +
 migration/migration.c        |  25 ++++++
 migration/migration.h        |   1 +
 migration/ram.c              | 142 ++++++++++++++++++++++++++++++++++-
 migration/trace-events       |   2 +-
 qapi/common.json             |  15 ++++
 qapi/migration.json          |  20 ++++-
 9 files changed, 229 insertions(+), 11 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/3] migration: Add multifd-compress parameter

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hmp.c                        | 17 +++++++++++++++++
 hw/core/qdev-properties.c    | 11 +++++++++++
 include/hw/qdev-properties.h |  1 +
 migration/migration.c        | 25 +++++++++++++++++++++++++
 migration/migration.h        |  1 +
 qapi/common.json             | 15 +++++++++++++++
 qapi/migration.json          | 20 ++++++++++++++++----
 7 files changed, 86 insertions(+), 4 deletions(-)

diff --git a/hmp.c b/hmp.c
index 34edad6ea3..abbd49ec17 100644
--- a/hmp.c
+++ b/hmp.c
@@ -422,6 +422,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "%s: %u\n",
             MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_CHANNELS),
             params->multifd_channels);
+        monitor_printf(mon, "%s: %s\n",
+            MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_COMPRESS),
+            MultifdCompress_str(params->multifd_compress));
         monitor_printf(mon, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
             params->xbzrle_cache_size);
@@ -1690,6 +1693,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
     MigrateSetParameters *p = g_new0(MigrateSetParameters, 1);
     uint64_t valuebw = 0;
     uint64_t cache_size;
+    int compress_type;
     Error *err = NULL;
     int val, ret;
 
@@ -1769,6 +1773,19 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         p->has_multifd_channels = true;
         visit_type_int(v, param, &p->multifd_channels, &err);
         break;
+    case MIGRATION_PARAMETER_MULTIFD_COMPRESS:
+        p->has_multifd_compress = true;
+        visit_type_enum(v, param, &compress_type,
+                        &MultifdCompress_lookup, &err);
+        if (err) {
+            break;
+        }
+        if (compress_type < 0 || compress_type > MULTIFD_COMPRESS__MAX) {
+            error_setg(&err, "Invalid multifd_compress option %s", valuestr);
+            break;
+        }
+        p->multifd_compress = compress_type;
+        break;
     case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
         p->has_xbzrle_cache_size = true;
         visit_type_size(v, param, &cache_size, &err);
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 5da1439a8b..7c8e71532f 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -645,6 +645,17 @@ const PropertyInfo qdev_prop_fdc_drive_type = {
     .set_default_value = set_default_value_enum,
 };
 
+/* --- MultifdCompress --- */
+
+const PropertyInfo qdev_prop_multifd_compress = {
+    .name = "MultifdCompress",
+    .description = "multifd_compress values",
+    .enum_table = &MultifdCompress_lookup,
+    .get = get_enum,
+    .set = set_enum,
+    .set_default_value = set_default_value_enum,
+};
+
 /* --- pci address --- */
 
 /*
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index b6758c852e..ac452d8f2c 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -23,6 +23,7 @@ extern const PropertyInfo qdev_prop_tpm;
 extern const PropertyInfo qdev_prop_ptr;
 extern const PropertyInfo qdev_prop_macaddr;
 extern const PropertyInfo qdev_prop_on_off_auto;
+extern const PropertyInfo qdev_prop_multifd_compress;
 extern const PropertyInfo qdev_prop_losttickpolicy;
 extern const PropertyInfo qdev_prop_blockdev_on_error;
 extern const PropertyInfo qdev_prop_bios_chs_trans;
diff --git a/migration/migration.c b/migration/migration.c
index f246519ec8..568ec8530f 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -81,6 +81,7 @@
 /* The delay time (in ms) between two COLO checkpoints */
 #define DEFAULT_MIGRATE_X_CHECKPOINT_DELAY (200 * 100)
 #define DEFAULT_MIGRATE_MULTIFD_CHANNELS 2
+#define DEFAULT_MIGRATE_MULTIFD_COMPRESS false
 
 /* Background transfer rate for postcopy, 0 means unlimited, note
  * that page requests can still exceed this limit.
@@ -748,6 +749,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->block_incremental = s->parameters.block_incremental;
     params->has_multifd_channels = true;
     params->multifd_channels = s->parameters.multifd_channels;
+    params->has_multifd_compress = true;
+    params->multifd_compress = s->parameters.multifd_compress;
     params->has_xbzrle_cache_size = true;
     params->xbzrle_cache_size = s->parameters.xbzrle_cache_size;
     params->has_max_postcopy_bandwidth = true;
@@ -1191,6 +1194,9 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
     if (params->has_multifd_channels) {
         dest->multifd_channels = params->multifd_channels;
     }
+    if (params->has_multifd_compress) {
+        dest->multifd_compress = params->multifd_compress;
+    }
     if (params->has_xbzrle_cache_size) {
         dest->xbzrle_cache_size = params->xbzrle_cache_size;
     }
@@ -1269,6 +1275,9 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
     if (params->has_multifd_channels) {
         s->parameters.multifd_channels = params->multifd_channels;
     }
+    if (params->has_multifd_compress) {
+        s->parameters.multifd_compress = params->multifd_compress;
+    }
     if (params->has_xbzrle_cache_size) {
         s->parameters.xbzrle_cache_size = params->xbzrle_cache_size;
         xbzrle_cache_resize(params->xbzrle_cache_size, errp);
@@ -2008,6 +2017,15 @@ bool migrate_use_multifd(void)
     return s->enabled_capabilities[MIGRATION_CAPABILITY_MULTIFD];
 }
 
+bool migrate_use_multifd_zlib(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->parameters.multifd_compress == MULTIFD_COMPRESS_ZLIB;
+}
+
 bool migrate_pause_before_switchover(void)
 {
     MigrationState *s;
@@ -3220,6 +3238,9 @@ void migration_global_dump(Monitor *mon)
 #define DEFINE_PROP_MIG_CAP(name, x)             \
     DEFINE_PROP_BOOL(name, MigrationState, enabled_capabilities[x], false)
 
+#define DEFINE_PROP_MULTIFD_COMPRESS(_n, _s, _f, _d) \
+    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_multifd_compress, MultifdCompress)
+
 static Property migration_properties[] = {
     DEFINE_PROP_BOOL("store-global-state", MigrationState,
                      store_global_state, true),
@@ -3260,6 +3281,9 @@ static Property migration_properties[] = {
     DEFINE_PROP_UINT8("multifd-channels", MigrationState,
                       parameters.multifd_channels,
                       DEFAULT_MIGRATE_MULTIFD_CHANNELS),
+    DEFINE_PROP_MULTIFD_COMPRESS("multifd-compress", MigrationState,
+                      parameters.multifd_compress,
+                      DEFAULT_MIGRATE_MULTIFD_COMPRESS),
     DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState,
                       parameters.xbzrle_cache_size,
                       DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE),
@@ -3337,6 +3361,7 @@ static void migration_instance_init(Object *obj)
     params->has_x_checkpoint_delay = true;
     params->has_block_incremental = true;
     params->has_multifd_channels = true;
+    params->has_multifd_compress = true;
     params->has_xbzrle_cache_size = true;
     params->has_max_postcopy_bandwidth = true;
     params->has_max_cpu_throttle = true;
diff --git a/migration/migration.h b/migration/migration.h
index 7e03643683..646614f71b 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -267,6 +267,7 @@ bool migrate_dirty_bitmaps(void);
 
 bool migrate_auto_converge(void);
 bool migrate_use_multifd(void);
+bool migrate_use_multifd_zlib(void);
 bool migrate_pause_before_switchover(void);
 int migrate_multifd_channels(void);
 
diff --git a/qapi/common.json b/qapi/common.json
index 99d313ef3b..ccdd58d21b 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -193,3 +193,18 @@
              'ppc64', 'riscv32', 'riscv64', 's390x', 'sh4',
              'sh4eb', 'sparc', 'sparc64', 'tricore', 'unicore32',
              'x86_64', 'xtensa', 'xtensaeb' ] }
+
+##
+# @MultifdCompress:
+#
+# An enumeration of multifd compression.
+#
+# @none: no compression.
+#
+# @zlib: Compress using zlib.
+#
+# Since: 3.1
+#
+##
+{ 'enum': 'MultifdCompress',
+  'data': [ 'none', 'zlib' ] }
diff --git a/qapi/migration.json b/qapi/migration.json
index c202703889..a882fc0823 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -558,6 +558,10 @@
 #
 # @max-cpu-throttle: maximum cpu throttle percentage.
 #                    Defaults to 99. (Since 3.1)
+#
+# @multifd-compress: What compression method to use.
+#                    Defaults to none. (Since 4.0)
+#
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
@@ -568,7 +572,7 @@
            'downtime-limit', 'x-checkpoint-delay', 'block-incremental',
            'multifd-channels',
            'xbzrle-cache-size', 'max-postcopy-bandwidth',
-           'max-cpu-throttle' ] }
+           'max-cpu-throttle', 'multifd-compress' ] }
 
 ##
 # @MigrateSetParameters:
@@ -644,7 +648,10 @@
 #                     (Since 3.0)
 #
 # @max-cpu-throttle: maximum cpu throttle percentage.
-#                    The default value is 99. (Since 3.1)
+#                    The default value is 99. (Since 4.0)
+#
+# @multifd-compress: What compression method to use.
+#                    Defaults to none. (Since 4.0)
 #
 # Since: 2.4
 ##
@@ -666,7 +673,8 @@
             '*multifd-channels': 'int',
             '*xbzrle-cache-size': 'size',
             '*max-postcopy-bandwidth': 'size',
-	    '*max-cpu-throttle': 'int' } }
+	    '*max-cpu-throttle': 'int',
+            '*multifd-compress': 'MultifdCompress' } }
 
 ##
 # @migrate-set-parameters:
@@ -760,6 +768,9 @@
 #                    Defaults to 99.
 #                     (Since 3.1)
 #
+# @multifd-compress: What compression method to use.
+#                    Defaults to none. (Since 4.0)
+#
 # Since: 2.4
 ##
 { 'struct': 'MigrationParameters',
@@ -778,7 +789,8 @@
             '*multifd-channels': 'uint8',
             '*xbzrle-cache-size': 'size',
 	    '*max-postcopy-bandwidth': 'size',
-            '*max-cpu-throttle':'uint8'} }
+            '*max-cpu-throttle': 'uint8',
+            '*multifd-compress': 'MultifdCompress' } }
 
 ##
 # @query-migrate-parameters:
-- 
2.20.1

Re: [Qemu-devel] [PATCH 1/3] migration: Add multifd-compress parameter

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hmp.c                        | 17 +++++++++++++++++
>  hw/core/qdev-properties.c    | 11 +++++++++++
>  include/hw/qdev-properties.h |  1 +
>  migration/migration.c        | 25 +++++++++++++++++++++++++
>  migration/migration.h        |  1 +
>  qapi/common.json             | 15 +++++++++++++++
>  qapi/migration.json          | 20 ++++++++++++++++----
>  7 files changed, 86 insertions(+), 4 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 34edad6ea3..abbd49ec17 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -422,6 +422,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>          monitor_printf(mon, "%s: %u\n",
>              MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_CHANNELS),
>              params->multifd_channels);
> +        monitor_printf(mon, "%s: %s\n",
> +            MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_COMPRESS),
> +            MultifdCompress_str(params->multifd_compress));
>          monitor_printf(mon, "%s: %" PRIu64 "\n",
>              MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
>              params->xbzrle_cache_size);
> @@ -1690,6 +1693,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>      MigrateSetParameters *p = g_new0(MigrateSetParameters, 1);
>      uint64_t valuebw = 0;
>      uint64_t cache_size;
> +    int compress_type;
>      Error *err = NULL;
>      int val, ret;
>  
> @@ -1769,6 +1773,19 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>          p->has_multifd_channels = true;
>          visit_type_int(v, param, &p->multifd_channels, &err);
>          break;
> +    case MIGRATION_PARAMETER_MULTIFD_COMPRESS:
> +        p->has_multifd_compress = true;
> +        visit_type_enum(v, param, &compress_type,
> +                        &MultifdCompress_lookup, &err);
> +        if (err) {
> +            break;
> +        }
> +        if (compress_type < 0 || compress_type > MULTIFD_COMPRESS__MAX) {

I think that needs to be >= rather than >
(Actually it surprises me visit_type_enum doesn't turn those cases into
errors)

> +            error_setg(&err, "Invalid multifd_compress option %s", valuestr);
> +            break;
> +        }
> +        p->multifd_compress = compress_type;
> +        break;
>      case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
>          p->has_xbzrle_cache_size = true;
>          visit_type_size(v, param, &cache_size, &err);
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 5da1439a8b..7c8e71532f 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -645,6 +645,17 @@ const PropertyInfo qdev_prop_fdc_drive_type = {
>      .set_default_value = set_default_value_enum,
>  };
>  
> +/* --- MultifdCompress --- */
> +
> +const PropertyInfo qdev_prop_multifd_compress = {
> +    .name = "MultifdCompress",
> +    .description = "multifd_compress values",
> +    .enum_table = &MultifdCompress_lookup,
> +    .get = get_enum,
> +    .set = set_enum,
> +    .set_default_value = set_default_value_enum,
> +};
> +
>  /* --- pci address --- */
>  
>  /*
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index b6758c852e..ac452d8f2c 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -23,6 +23,7 @@ extern const PropertyInfo qdev_prop_tpm;
>  extern const PropertyInfo qdev_prop_ptr;
>  extern const PropertyInfo qdev_prop_macaddr;
>  extern const PropertyInfo qdev_prop_on_off_auto;
> +extern const PropertyInfo qdev_prop_multifd_compress;
>  extern const PropertyInfo qdev_prop_losttickpolicy;
>  extern const PropertyInfo qdev_prop_blockdev_on_error;
>  extern const PropertyInfo qdev_prop_bios_chs_trans;
> diff --git a/migration/migration.c b/migration/migration.c
> index f246519ec8..568ec8530f 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -81,6 +81,7 @@
>  /* The delay time (in ms) between two COLO checkpoints */
>  #define DEFAULT_MIGRATE_X_CHECKPOINT_DELAY (200 * 100)
>  #define DEFAULT_MIGRATE_MULTIFD_CHANNELS 2
> +#define DEFAULT_MIGRATE_MULTIFD_COMPRESS false

Shouldn't that be an enum value?

>  /* Background transfer rate for postcopy, 0 means unlimited, note
>   * that page requests can still exceed this limit.
> @@ -748,6 +749,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>      params->block_incremental = s->parameters.block_incremental;
>      params->has_multifd_channels = true;
>      params->multifd_channels = s->parameters.multifd_channels;
> +    params->has_multifd_compress = true;
> +    params->multifd_compress = s->parameters.multifd_compress;
>      params->has_xbzrle_cache_size = true;
>      params->xbzrle_cache_size = s->parameters.xbzrle_cache_size;
>      params->has_max_postcopy_bandwidth = true;
> @@ -1191,6 +1194,9 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>      if (params->has_multifd_channels) {
>          dest->multifd_channels = params->multifd_channels;
>      }
> +    if (params->has_multifd_compress) {
> +        dest->multifd_compress = params->multifd_compress;
> +    }
>      if (params->has_xbzrle_cache_size) {
>          dest->xbzrle_cache_size = params->xbzrle_cache_size;
>      }
> @@ -1269,6 +1275,9 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>      if (params->has_multifd_channels) {
>          s->parameters.multifd_channels = params->multifd_channels;
>      }
> +    if (params->has_multifd_compress) {
> +        s->parameters.multifd_compress = params->multifd_compress;
> +    }
>      if (params->has_xbzrle_cache_size) {
>          s->parameters.xbzrle_cache_size = params->xbzrle_cache_size;
>          xbzrle_cache_resize(params->xbzrle_cache_size, errp);
> @@ -2008,6 +2017,15 @@ bool migrate_use_multifd(void)
>      return s->enabled_capabilities[MIGRATION_CAPABILITY_MULTIFD];
>  }
>  
> +bool migrate_use_multifd_zlib(void)
> +{
> +    MigrationState *s;
> +
> +    s = migrate_get_current();
> +
> +    return s->parameters.multifd_compress == MULTIFD_COMPRESS_ZLIB;
> +}
> +
>  bool migrate_pause_before_switchover(void)
>  {
>      MigrationState *s;
> @@ -3220,6 +3238,9 @@ void migration_global_dump(Monitor *mon)
>  #define DEFINE_PROP_MIG_CAP(name, x)             \
>      DEFINE_PROP_BOOL(name, MigrationState, enabled_capabilities[x], false)
>  
> +#define DEFINE_PROP_MULTIFD_COMPRESS(_n, _s, _f, _d) \
> +    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_multifd_compress, MultifdCompress)
> +
>  static Property migration_properties[] = {
>      DEFINE_PROP_BOOL("store-global-state", MigrationState,
>                       store_global_state, true),
> @@ -3260,6 +3281,9 @@ static Property migration_properties[] = {
>      DEFINE_PROP_UINT8("multifd-channels", MigrationState,
>                        parameters.multifd_channels,
>                        DEFAULT_MIGRATE_MULTIFD_CHANNELS),
> +    DEFINE_PROP_MULTIFD_COMPRESS("multifd-compress", MigrationState,
> +                      parameters.multifd_compress,
> +                      DEFAULT_MIGRATE_MULTIFD_COMPRESS),
>      DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState,
>                        parameters.xbzrle_cache_size,
>                        DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE),
> @@ -3337,6 +3361,7 @@ static void migration_instance_init(Object *obj)
>      params->has_x_checkpoint_delay = true;
>      params->has_block_incremental = true;
>      params->has_multifd_channels = true;
> +    params->has_multifd_compress = true;
>      params->has_xbzrle_cache_size = true;
>      params->has_max_postcopy_bandwidth = true;
>      params->has_max_cpu_throttle = true;
> diff --git a/migration/migration.h b/migration/migration.h
> index 7e03643683..646614f71b 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -267,6 +267,7 @@ bool migrate_dirty_bitmaps(void);
>  
>  bool migrate_auto_converge(void);
>  bool migrate_use_multifd(void);
> +bool migrate_use_multifd_zlib(void);
>  bool migrate_pause_before_switchover(void);
>  int migrate_multifd_channels(void);
>  
> diff --git a/qapi/common.json b/qapi/common.json
> index 99d313ef3b..ccdd58d21b 100644
> --- a/qapi/common.json
> +++ b/qapi/common.json
> @@ -193,3 +193,18 @@
>               'ppc64', 'riscv32', 'riscv64', 's390x', 'sh4',
>               'sh4eb', 'sparc', 'sparc64', 'tricore', 'unicore32',
>               'x86_64', 'xtensa', 'xtensaeb' ] }
> +
> +##
> +# @MultifdCompress:
> +#
> +# An enumeration of multifd compression.
> +#
> +# @none: no compression.
> +#
> +# @zlib: Compress using zlib.
> +#
> +# Since: 3.1

4.

> +#
> +##
> +{ 'enum': 'MultifdCompress',
> +  'data': [ 'none', 'zlib' ] }
> diff --git a/qapi/migration.json b/qapi/migration.json
> index c202703889..a882fc0823 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -558,6 +558,10 @@
>  #
>  # @max-cpu-throttle: maximum cpu throttle percentage.
>  #                    Defaults to 99. (Since 3.1)
> +#
> +# @multifd-compress: What compression method to use.
> +#                    Defaults to none. (Since 4.0)
> +#

I think that should be 'Which' rather than 'What'.

>  # Since: 2.4
>  ##
>  { 'enum': 'MigrationParameter',
> @@ -568,7 +572,7 @@
>             'downtime-limit', 'x-checkpoint-delay', 'block-incremental',
>             'multifd-channels',
>             'xbzrle-cache-size', 'max-postcopy-bandwidth',
> -           'max-cpu-throttle' ] }
> +           'max-cpu-throttle', 'multifd-compress' ] }
>  
>  ##
>  # @MigrateSetParameters:
> @@ -644,7 +648,10 @@
>  #                     (Since 3.0)
>  #
>  # @max-cpu-throttle: maximum cpu throttle percentage.
> -#                    The default value is 99. (Since 3.1)
> +#                    The default value is 99. (Since 4.0)

That change is wrong?

> +# @multifd-compress: What compression method to use.
> +#                    Defaults to none. (Since 4.0)
>  #
>  # Since: 2.4
>  ##
> @@ -666,7 +673,8 @@
>              '*multifd-channels': 'int',
>              '*xbzrle-cache-size': 'size',
>              '*max-postcopy-bandwidth': 'size',
> -	    '*max-cpu-throttle': 'int' } }
> +	    '*max-cpu-throttle': 'int',
> +            '*multifd-compress': 'MultifdCompress' } }
>  
>  ##
>  # @migrate-set-parameters:
> @@ -760,6 +768,9 @@
>  #                    Defaults to 99.
>  #                     (Since 3.1)
>  #
> +# @multifd-compress: What compression method to use.
> +#                    Defaults to none. (Since 4.0)
> +#
>  # Since: 2.4
>  ##
>  { 'struct': 'MigrationParameters',
> @@ -778,7 +789,8 @@
>              '*multifd-channels': 'uint8',
>              '*xbzrle-cache-size': 'size',
>  	    '*max-postcopy-bandwidth': 'size',
> -            '*max-cpu-throttle':'uint8'} }
> +            '*max-cpu-throttle': 'uint8',
> +            '*multifd-compress': 'MultifdCompress' } }
>  
>  ##
>  # @query-migrate-parameters:

Dave

> -- 
> 2.20.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/3] migration: Add multifd-compress parameter

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>

>> +    case MIGRATION_PARAMETER_MULTIFD_COMPRESS:
>> +        p->has_multifd_compress = true;
>> +        visit_type_enum(v, param, &compress_type,
>> +                        &MultifdCompress_lookup, &err);
>> +        if (err) {
>> +            break;
>> +        }
>> +        if (compress_type < 0 || compress_type > MULTIFD_COMPRESS__MAX) {
>
> I think that needs to be >= rather than >
> (Actually it surprises me visit_type_enum doesn't turn those cases into
> errors)

You are right.

>> @@ -81,6 +81,7 @@
>>  /* The delay time (in ms) between two COLO checkpoints */
>>  #define DEFAULT_MIGRATE_X_CHECKPOINT_DELAY (200 * 100)
>>  #define DEFAULT_MIGRATE_MULTIFD_CHANNELS 2
>> +#define DEFAULT_MIGRATE_MULTIFD_COMPRESS false
>
> Shouldn't that be an enum value?

Fixed

>> +
>> +##
>> +# @MultifdCompress:
>> +#
>> +# An enumeration of multifd compression.
>> +#
>> +# @none: no compression.
>> +#
>> +# @zlib: Compress using zlib.
>> +#
>> +# Since: 3.1
>
> 4.

fixed.

>>  # @max-cpu-throttle: maximum cpu throttle percentage.
>>  #                    Defaults to 99. (Since 3.1)
>> +#
>> +# @multifd-compress: What compression method to use.
>> +#                    Defaults to none. (Since 4.0)
>> +#
>
> I think that should be 'Which' rather than 'What'.

You are the native speaker.

>>  # Since: 2.4
>>  ##
>>  { 'enum': 'MigrationParameter',
>> @@ -568,7 +572,7 @@
>>             'downtime-limit', 'x-checkpoint-delay', 'block-incremental',
>>             'multifd-channels',
>>             'xbzrle-cache-size', 'max-postcopy-bandwidth',
>> -           'max-cpu-throttle' ] }
>> +           'max-cpu-throttle', 'multifd-compress' ] }
>>  
>>  ##
>>  # @MigrateSetParameters:
>> @@ -644,7 +648,10 @@
>>  #                     (Since 3.0)
>>  #
>>  # @max-cpu-throttle: maximum cpu throttle percentage.
>> -#                    The default value is 99. (Since 3.1)
>> +#                    The default value is 99. (Since 4.0)
>
> That change is wrong?

Fixed.

Thanks, Juan.

[Qemu-devel] [PATCH 2/3] multifd: compression support variables

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/migration/ram.c b/migration/ram.c
index d57db00ce4..7de27e1a35 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -597,6 +597,12 @@ typedef struct {
     uint64_t num_pages;
     /* syncs main thread and channels */
     QemuSemaphore sem_sync;
+    /* stream for compression */
+    z_stream zs;
+    /* compressed buffer */
+    uint8_t *zbuff;
+    /* size of compressed buffer */
+    uint32_t zbuff_len;
 }  MultiFDSendParams;
 
 typedef struct {
@@ -632,6 +638,12 @@ typedef struct {
     uint64_t num_pages;
     /* syncs main thread and channels */
     QemuSemaphore sem_sync;
+    /* stream for compression */
+    z_stream zs;
+    /* compressed buffer */
+    uint8_t *zbuff;
+    /* size of compressed buffer */
+    uint32_t zbuff_len;
 } MultiFDRecvParams;
 
 static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
@@ -969,6 +981,9 @@ void multifd_save_cleanup(void)
         p->packet_len = 0;
         g_free(p->packet);
         p->packet = NULL;
+        deflateEnd(&p->zs);
+        g_free(p->zbuff);
+        p->zbuff = NULL;
     }
     qemu_sem_destroy(&multifd_send_state->channels_ready);
     qemu_sem_destroy(&multifd_send_state->sem_sync);
@@ -1132,6 +1147,7 @@ int multifd_save_setup(void)
 
     for (i = 0; i < thread_count; i++) {
         MultiFDSendParams *p = &multifd_send_state->params[i];
+        z_stream *zs = &p->zs;
 
         qemu_mutex_init(&p->mutex);
         qemu_sem_init(&p->sem, 0);
@@ -1145,6 +1161,17 @@ int multifd_save_setup(void)
         p->packet = g_malloc0(p->packet_len);
         p->name = g_strdup_printf("multifdsend_%d", i);
         socket_send_channel_create(multifd_new_send_channel_async, p);
+        zs->zalloc = Z_NULL;
+        zs->zfree = Z_NULL;
+        zs->opaque = Z_NULL;
+        if (deflateInit(zs, migrate_compress_level()) != Z_OK) {
+            printf("deflate init failed\n");
+            return -1;
+        }
+        /* We will never have more than page_count pages */
+        p->zbuff_len = page_count * qemu_target_page_size();
+        p->zbuff_len *= 2;
+        p->zbuff = g_malloc0(p->zbuff_len);
     }
     return 0;
 }
@@ -1212,6 +1239,9 @@ int multifd_load_cleanup(Error **errp)
         p->packet_len = 0;
         g_free(p->packet);
         p->packet = NULL;
+        inflateEnd(&p->zs);
+        g_free(p->zbuff);
+        p->zbuff = NULL;
     }
     qemu_sem_destroy(&multifd_recv_state->sem_sync);
     g_free(multifd_recv_state->params);
@@ -1330,6 +1360,7 @@ int multifd_load_setup(void)
 
     for (i = 0; i < thread_count; i++) {
         MultiFDRecvParams *p = &multifd_recv_state->params[i];
+        z_stream *zs = &p->zs;
 
         qemu_mutex_init(&p->mutex);
         qemu_sem_init(&p->sem_sync, 0);
@@ -1339,6 +1370,21 @@ int multifd_load_setup(void)
                       + sizeof(ram_addr_t) * page_count;
         p->packet = g_malloc0(p->packet_len);
         p->name = g_strdup_printf("multifdrecv_%d", i);
+
+        zs->zalloc = Z_NULL;
+        zs->zfree = Z_NULL;
+        zs->opaque = Z_NULL;
+        zs->avail_in = 0;
+        zs->next_in = Z_NULL;
+        if (inflateInit(zs) != Z_OK) {
+            printf("inflate init failed\n");
+            return -1;
+        }
+        /* We will never have more than page_count pages */
+        p->zbuff_len = page_count * qemu_target_page_size();
+        /* We know compression "could" use more space */
+        p->zbuff_len *= 2;
+        p->zbuff = g_malloc0(p->zbuff_len);
     }
     return 0;
 }
-- 
2.20.1

[Qemu-devel] [PATCH 3/3] multifd: Start of zlib compression code

[Juan Quintela]

This is still work in progress.

It is already faster that normal compression code inside qemu.
It don't do any unnecesary copies.  And as packages are bigger, we get
better compression.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hmp.c                  |  6 ++-
 migration/ram.c        | 96 ++++++++++++++++++++++++++++++++++++++++--
 migration/trace-events |  2 +-
 3 files changed, 97 insertions(+), 7 deletions(-)

diff --git a/hmp.c b/hmp.c
index abbd49ec17..7c1ad2376d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1789,8 +1789,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
     case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
         p->has_xbzrle_cache_size = true;
         visit_type_size(v, param, &cache_size, &err);
-        if (err || cache_size > INT64_MAX
-            || (size_t)cache_size != cache_size) {
+        if (err) {
+            break;
+        }
+        if (cache_size > INT64_MAX || (size_t)cache_size != cache_size) {
             error_setg(&err, "Invalid size %s", valuestr);
             break;
         }
diff --git a/migration/ram.c b/migration/ram.c
index 7de27e1a35..feb857d395 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -519,6 +519,7 @@ exit:
 #define MULTIFD_VERSION 1
 
 #define MULTIFD_FLAG_SYNC (1 << 0)
+#define MULTIFD_FLAG_ZLIB (1 << 1)
 
 /* This value needs to be a multiple of qemu_target_page_size() */
 #define MULTIFD_PACKET_SIZE (512 * 1024)
@@ -1031,6 +1032,7 @@ static void *multifd_send_thread(void *opaque)
 {
     MultiFDSendParams *p = opaque;
     Error *local_err = NULL;
+    z_stream *zs = &p->zs;
     int ret;
 
     trace_multifd_send_thread_start(p->id);
@@ -1050,8 +1052,43 @@ static void *multifd_send_thread(void *opaque)
             uint32_t used = p->pages->used;
             uint64_t packet_num = p->packet_num;
             uint32_t flags = p->flags;
+            struct iovec *iov = p->pages->iov;
 
             p->next_packet_size = used * qemu_target_page_size();
+
+            if (used && migrate_use_multifd_zlib()) {
+                uint32_t in_size = used * qemu_target_page_size();
+                uint32_t out_size = 0;
+                int i;
+
+                for (i = 0; i < used; i++ ) {
+                    uint32_t available = p->zbuff_len - out_size;
+                    int flush = Z_NO_FLUSH;
+
+                    if (i == used  - 1) {
+                        flush = Z_SYNC_FLUSH;
+                    }
+
+                    zs->avail_in = iov[i].iov_len;
+                    zs->next_in = iov[i].iov_base;
+
+                    zs->avail_out = available;
+                    zs->next_out = p->zbuff + out_size;
+
+                    ret = deflate(zs, flush);
+                    if (ret != Z_OK) {
+                        printf("problem with deflate? %d\n", ret);
+                        qemu_mutex_unlock(&p->mutex);
+                        break;
+                    }
+                    out_size += available - zs->avail_out;
+                }
+                printf("%d deflate in %d out %d diff %d\n",
+                       p->id, in_size, out_size, in_size - out_size);
+
+                p->next_packet_size = out_size;
+            }
+
             multifd_send_fill_packet(p);
             p->flags = 0;
             p->num_packets++;
@@ -1069,8 +1106,13 @@ static void *multifd_send_thread(void *opaque)
             }
 
             if (used) {
-                ret = qio_channel_writev_all(p->c, p->pages->iov,
-                                             used, &local_err);
+                if (migrate_use_multifd_zlib()) {
+                    ret = qio_channel_write_all(p->c, (void *)p->zbuff,
+                                               p->next_packet_size, &local_err);
+                } else {
+                    ret = qio_channel_writev_all(p->c, p->pages->iov,
+                                                 used, &local_err);
+                }
                 if (ret != 0) {
                     break;
                 }
@@ -1283,6 +1325,7 @@ static void *multifd_recv_thread(void *opaque)
 {
     MultiFDRecvParams *p = opaque;
     Error *local_err = NULL;
+    z_stream *zs = &p->zs;
     int ret;
 
     trace_multifd_recv_thread_start(p->id);
@@ -1317,8 +1360,53 @@ static void *multifd_recv_thread(void *opaque)
         qemu_mutex_unlock(&p->mutex);
 
         if (used) {
-            ret = qio_channel_readv_all(p->c, p->pages->iov,
-                                        used, &local_err);
+            uint32_t in_size = p->next_packet_size;
+            uint32_t out_size = 0;
+            uint32_t expected_size = used * qemu_target_page_size();
+            int i;
+
+            if (migrate_use_multifd_zlib()) {
+                ret = qio_channel_read_all(p->c, (void *)p->zbuff,
+                                           in_size, &local_err);
+
+                if (ret != 0) {
+                    break;
+                }
+
+                zs->avail_in = in_size;
+                zs->next_in = p->zbuff;
+
+                for (i = 0; i < used; i++ ) {
+                    struct iovec *iov = &p->pages->iov[i];
+                    int flush = Z_NO_FLUSH;
+
+                    if (i == used  - 1) {
+                        flush = Z_SYNC_FLUSH;
+                    }
+
+                    zs->avail_out = iov->iov_len;
+                    zs->next_out = iov->iov_base;
+
+                    ret = inflate(zs, flush);
+                    if (ret != Z_OK) {
+                        printf("%d: problem with inflate? %d\n", p->id, ret);
+                        qemu_mutex_unlock(&p->mutex);
+                        break;
+                    }
+                    out_size += iov->iov_len;
+                }
+
+                printf("%d: out_size = %d, in_size = %d, expected_size = %d avail_in: %d\n",
+                       p->id, out_size, in_size, expected_size, zs->avail_in);
+                if (out_size != expected_size) {
+                    printf("out size %d expected size %d\n",
+                           out_size, expected_size);
+                    break;
+                }
+            } else {
+                ret = qio_channel_readv_all(p->c, p->pages->iov,
+                                            used, &local_err);
+            }
             if (ret != 0) {
                 break;
             }
diff --git a/migration/trace-events b/migration/trace-events
index a11e66e1d9..8aaae32e67 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -77,7 +77,7 @@ get_queued_page_not_dirty(const char *block_name, uint64_t tmp_offset, unsigned
 migration_bitmap_sync_start(void) ""
 migration_bitmap_sync_end(uint64_t dirty_pages) "dirty_pages %" PRIu64
 migration_throttle(void) ""
-multifd_recv(uint8_t id, uint64_t packet_num, uint32_t used, uint32_t flags, uint32_t next_packet_size) "channel %d packet number %" PRIu64 " pages %d flags 0x%x next packet size %d"
+multifd_recv(uint8_t id, uint64_t packet_num, uint32_t used, uint32_t flags, uint32_t next_packet_size) "channel %d packet_num %" PRIu64 " pages %d flags 0x%x next packet size %d"
 multifd_recv_sync_main(long packet_num) "packet num %ld"
 multifd_recv_sync_main_signal(uint8_t id) "channel %d"
 multifd_recv_sync_main_wait(uint8_t id) "channel %d"
-- 
2.20.1