diff mbox series

[v5,4/8] multifd: Add multifd-zlib-level parameter

Message ID 20200129115655.10414-5-quintela@redhat.com (mailing list archive)
State New, archived
Headers show
Series Multifd Migration Compression | expand

Commit Message

Juan Quintela Jan. 29, 2020, 11:56 a.m. UTC
It will indicate which level use for compression.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 15 +++++++++++++++
 monitor/hmp-cmds.c    |  4 ++++
 qapi/migration.json   | 30 +++++++++++++++++++++++++++---
 3 files changed, 46 insertions(+), 3 deletions(-)

Comments

Markus Armbruster Jan. 30, 2020, 8:03 a.m. UTC | #1
Juan Quintela <quintela@redhat.com> writes:

> It will indicate which level use for compression.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>

This is slightly confusing (there is no zlib compression), unless you
peek at the next patch (which adds zlib compression).

Three ways to make it less confusing:

* Squash the two commits

* Swap them: first add zlib compression with level hardcoded to 1, then
  make the level configurable.

* Have the first commit explain itself better.  Something like

    multifd: Add multifd-zlib-level parameter

    This parameter specifies zlib compression level.  The next patch
    will put it to use.

For QAPI:
Acked-by: Markus Armbruster <armbru@redhat.com>
Juan Quintela Jan. 30, 2020, 8:56 a.m. UTC | #2
Markus Armbruster <armbru@redhat.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> It will indicate which level use for compression.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> This is slightly confusing (there is no zlib compression), unless you
> peek at the next patch (which adds zlib compression).
>
> Three ways to make it less confusing:
>
> * Squash the two commits

As a QAPI begginer, I feel it easier to put it in a different patch.  It
makes it also easier to add other parameters, just copy whatewer is here.

> * Swap them: first add zlib compression with level hardcoded to 1, then
>   make the level configurable.

That could work.

> * Have the first commit explain itself better.  Something like
>
>     multifd: Add multifd-zlib-level parameter
>
>     This parameter specifies zlib compression level.  The next patch
>     will put it to use.

Will take this approach.
The reason that I put the qapi bits first is that I *know* how to do
them.  Once that I got approval for how to add one parameter, I add the
rest exactly the same.

For the rest of the patch, it needs to be developed, and normally needs
more iterations.

>
> For QAPI:
> Acked-by: Markus Armbruster <armbru@redhat.com>

Thanks very much.

Later, Juan.
Daniel P. Berrangé Feb. 11, 2020, 6:57 p.m. UTC | #3
On Thu, Jan 30, 2020 at 09:03:00AM +0100, Markus Armbruster wrote:
> Juan Quintela <quintela@redhat.com> writes:
> 
> > It will indicate which level use for compression.
> >
> > Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> This is slightly confusing (there is no zlib compression), unless you
> peek at the next patch (which adds zlib compression).
> 
> Three ways to make it less confusing:
> 
> * Squash the two commits
> 
> * Swap them: first add zlib compression with level hardcoded to 1, then
>   make the level configurable.
> 
> * Have the first commit explain itself better.  Something like
> 
>     multifd: Add multifd-zlib-level parameter
> 
>     This parameter specifies zlib compression level.  The next patch
>     will put it to use.

Wouldn't the "normal" best practice for QAPI design be to use a
enum and discriminated union. eg

  { 'enum': 'MigrationCompression',
     'data': ['none', 'zlib'] }

  { 'struct': 'MigrationCompressionParamsZLib',
    'data': { 'compression-level' } }

  { 'union':  'MigrationCompressionParams',
    'base': { 'mode': 'MigrationCompression' },
    'discriminator': 'mode',
    'data': {
      'zlib': 'MigrationCompressionParamsZLib',
    }

Of course this is quite different from how migration parameters are
done today. Maybe it makes sense to stick with the flat list of
migration parameters for consistency & ignore normal QAPI design
practice ?


Regards,
Daniel
Markus Armbruster Feb. 13, 2020, 1:27 p.m. UTC | #4
Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Jan 30, 2020 at 09:03:00AM +0100, Markus Armbruster wrote:
>> Juan Quintela <quintela@redhat.com> writes:
>> 
>> > It will indicate which level use for compression.
>> >
>> > Signed-off-by: Juan Quintela <quintela@redhat.com>
>> 
>> This is slightly confusing (there is no zlib compression), unless you
>> peek at the next patch (which adds zlib compression).
>> 
>> Three ways to make it less confusing:
>> 
>> * Squash the two commits
>> 
>> * Swap them: first add zlib compression with level hardcoded to 1, then
>>   make the level configurable.
>> 
>> * Have the first commit explain itself better.  Something like
>> 
>>     multifd: Add multifd-zlib-level parameter
>> 
>>     This parameter specifies zlib compression level.  The next patch
>>     will put it to use.
>
> Wouldn't the "normal" best practice for QAPI design be to use a
> enum and discriminated union. eg
>
>   { 'enum': 'MigrationCompression',
>      'data': ['none', 'zlib'] }
>
>   { 'struct': 'MigrationCompressionParamsZLib',
>     'data': { 'compression-level' } }
>
>   { 'union':  'MigrationCompressionParams',
>     'base': { 'mode': 'MigrationCompression' },
>     'discriminator': 'mode',
>     'data': {
>       'zlib': 'MigrationCompressionParamsZLib',
>     }

This expresses the connection between compression mode and level.  In
general, we prefer that to a bunch of optional members where the
comments say things like "member A can be present only when member B has
value V", or worse "member A is silently ignored unless member B has
value V".  However:

> Of course this is quite different from how migration parameters are
> done today. Maybe it makes sense to stick with the flat list of
> migration parameters for consistency & ignore normal QAPI design
> practice ?

Unsure.  It's certainly ugly now.  Each parameter is defined in three
places: enum MigrationParameter (for HMP), struct MigrateSetParameters
(for QMP migrate-set-parameters), and struct MigrationParameters (for
QMP query-migrate-parameters).

I don't know how to make this better other than by starting over.
I don't know whether starting over would result in enough of an
improvement to make it worthwhile.
Juan Quintela Feb. 13, 2020, 4:33 p.m. UTC | #5
Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Thu, Jan 30, 2020 at 09:03:00AM +0100, Markus Armbruster wrote:
>> Juan Quintela <quintela@redhat.com> writes:
>> 
>> > It will indicate which level use for compression.
>> >
>> > Signed-off-by: Juan Quintela <quintela@redhat.com>
>> 
>> This is slightly confusing (there is no zlib compression), unless you
>> peek at the next patch (which adds zlib compression).
>> 
>> Three ways to make it less confusing:
>> 
>> * Squash the two commits
>> 
>> * Swap them: first add zlib compression with level hardcoded to 1, then
>>   make the level configurable.
>> 
>> * Have the first commit explain itself better.  Something like
>> 
>>     multifd: Add multifd-zlib-level parameter
>> 
>>     This parameter specifies zlib compression level.  The next patch
>>     will put it to use.
>
> Wouldn't the "normal" best practice for QAPI design be to use a
> enum and discriminated union. eg
>
>   { 'enum': 'MigrationCompression',
>      'data': ['none', 'zlib'] }
>
>   { 'struct': 'MigrationCompressionParamsZLib',
>     'data': { 'compression-level' } }
>
>   { 'union':  'MigrationCompressionParams',
>     'base': { 'mode': 'MigrationCompression' },
>     'discriminator': 'mode',
>     'data': {
>       'zlib': 'MigrationCompressionParamsZLib',
>     }

How is this translate into HMP?

Markus says to start over, so lets see the dependencies:

Announce: Allawys there

announce-initial
announce-max
announce-rounds
announce-step

Osd compression (deprecated)

compress-level
compress-threads
compress-wait-thread
decompress-threads

cpu-throttles-initial
cpu-throottle-incroment
max-cpu-throotle

tls-creds
tls-hostname
tls-auth


Real params

max-bandwidth
downtime-limit


colo

x-checkpoint-delay

block-incremental

multifd-channels

xbzrle-cache-size

max-postcopy-bandwidth

New things:
- multifd method
- multifd-zlib-level
- multifd-zstd-level

What is a good way to define them?

Why do I ask, because the current method is as bad as it can be.
To add a new parameter:
- for qapi, add it in three places (as Markus said)
- go to hmp-cmds.c and do things by hand
- qemu_migrate_set_parameters
- migrate_params_check
- migrate_params_apply
(last three functions are almost identical in structure, not in
content).

So, if you can give me something that is _easier_ of maintaining, I am
all ears.

Later, Juan.

>
> Of course this is quite different from how migration parameters are
> done today. Maybe it makes sense to stick with the flat list of
> migration parameters for consistency & ignore normal QAPI design
> practice ?
>
>
> Regards,
> Daniel
diff mbox series

Patch

diff --git a/migration/migration.c b/migration/migration.c
index 06f6c2d529..4f88f8e958 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -89,6 +89,8 @@ 
 #define DEFAULT_MIGRATE_X_CHECKPOINT_DELAY (200 * 100)
 #define DEFAULT_MIGRATE_MULTIFD_CHANNELS 2
 #define DEFAULT_MIGRATE_MULTIFD_METHOD MULTIFD_METHOD_NONE
+/*0: means nocompress, 1: best speed, ... 9: best compress ratio */
+#define DEFAULT_MIGRATE_MULTIFD_ZLIB_LEVEL 1
 
 /* Background transfer rate for postcopy, 0 means unlimited, note
  * that page requests can still exceed this limit.
@@ -801,6 +803,8 @@  MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->multifd_channels = s->parameters.multifd_channels;
     params->has_multifd_method = true;
     params->multifd_method = s->parameters.multifd_method;
+    params->has_multifd_zlib_level = true;
+    params->multifd_zlib_level = s->parameters.multifd_zlib_level;
     params->has_xbzrle_cache_size = true;
     params->xbzrle_cache_size = s->parameters.xbzrle_cache_size;
     params->has_max_postcopy_bandwidth = true;
@@ -1208,6 +1212,13 @@  static bool migrate_params_check(MigrationParameters *params, Error **errp)
         return false;
     }
 
+    if (params->has_multifd_zlib_level &&
+        (params->multifd_zlib_level > 9)) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_zlib_level",
+                   "is invalid, it should be in the range of 0 to 9");
+        return false;
+    }
+
     if (params->has_xbzrle_cache_size &&
         (params->xbzrle_cache_size < qemu_target_page_size() ||
          !is_power_of_2(params->xbzrle_cache_size))) {
@@ -3536,6 +3547,9 @@  static Property migration_properties[] = {
     DEFINE_PROP_MULTIFD_METHOD("multifd-method", MigrationState,
                       parameters.multifd_method,
                       DEFAULT_MIGRATE_MULTIFD_METHOD),
+    DEFINE_PROP_UINT8("multifd-zlib-level", MigrationState,
+                      parameters.multifd_zlib_level,
+                      DEFAULT_MIGRATE_MULTIFD_ZLIB_LEVEL),
     DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState,
                       parameters.xbzrle_cache_size,
                       DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE),
@@ -3627,6 +3641,7 @@  static void migration_instance_init(Object *obj)
     params->has_block_incremental = true;
     params->has_multifd_channels = true;
     params->has_multifd_method = true;
+    params->has_multifd_zlib_level = true;
     params->has_xbzrle_cache_size = true;
     params->has_max_postcopy_bandwidth = true;
     params->has_max_cpu_throttle = true;
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 16f01d4244..7f11866446 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1836,6 +1836,10 @@  void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         }
         p->multifd_method = compress_type;
         break;
+    case MIGRATION_PARAMETER_MULTIFD_ZLIB_LEVEL:
+        p->has_multifd_zlib_level = true;
+        visit_type_int(v, param, &p->multifd_zlib_level, &err);
+        break;
     case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
         p->has_xbzrle_cache_size = true;
         visit_type_size(v, param, &cache_size, &err);
diff --git a/qapi/migration.json b/qapi/migration.json
index 96a126751c..289dce0da7 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -602,6 +602,13 @@ 
 # @multifd-method: Which compression method to use.
 #                  Defaults to none. (Since 5.0)
 #
+# @multifd-zlib-level: Set the compression level to be used in live
+#          migration, the compression level is an integer between 0
+#          and 9, where 0 means no compression, 1 means the best
+#          compression speed, and 9 means best compression ratio which
+#          will consume more CPU.
+#          Defaults to 1. (Since 5.0)
+#
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
@@ -614,7 +621,8 @@ 
            'downtime-limit', 'x-checkpoint-delay', 'block-incremental',
            'multifd-channels',
            'xbzrle-cache-size', 'max-postcopy-bandwidth',
-           'max-cpu-throttle', 'multifd-method' ] }
+           'max-cpu-throttle', 'multifd-method',
+           'multifd-zlib-level' ] }
 
 ##
 # @MigrateSetParameters:
@@ -707,6 +715,13 @@ 
 # @multifd-method: Which compression method to use.
 #                  Defaults to none. (Since 5.0)
 #
+# @multifd-zlib-level: Set the compression level to be used in live
+#          migration, the compression level is an integer between 0
+#          and 9, where 0 means no compression, 1 means the best
+#          compression speed, and 9 means best compression ratio which
+#          will consume more CPU.
+#          Defaults to 1. (Since 5.0)
+#
 # Since: 2.4
 ##
 # TODO either fuse back into MigrationParameters, or make
@@ -733,7 +748,8 @@ 
             '*xbzrle-cache-size': 'size',
             '*max-postcopy-bandwidth': 'size',
             '*max-cpu-throttle': 'int',
-            '*multifd-method': 'MultiFDMethod' } }
+            '*multifd-method': 'MultiFDMethod',
+            '*multifd-zlib-level': 'int' } }
 
 ##
 # @migrate-set-parameters:
@@ -846,6 +862,13 @@ 
 # @multifd-method: Which compression method to use.
 #                  Defaults to none. (Since 5.0)
 #
+# @multifd-zlib-level: Set the compression level to be used in live
+#          migration, the compression level is an integer between 0
+#          and 9, where 0 means no compression, 1 means the best
+#          compression speed, and 9 means best compression ratio which
+#          will consume more CPU.
+#          Defaults to 1. (Since 5.0)
+#
 # Since: 2.4
 ##
 { 'struct': 'MigrationParameters',
@@ -870,7 +893,8 @@ 
             '*xbzrle-cache-size': 'size',
 	    '*max-postcopy-bandwidth': 'size',
             '*max-cpu-throttle': 'uint8',
-            '*multifd-method': 'MultiFDMethod' } }
+            '*multifd-method': 'MultiFDMethod',
+            '*multifd-zlib-level': 'uint8' } }
 
 ##
 # @query-migrate-parameters: