Re: [Qemu-devel] [PATCH v2 3/3] migration: introduce adaptive model for waiting thread

[Markus Armbruster]

address@hidden writes:

> From: Xiao Guangrong <address@hidden>
>
> Currently we have two behaviors if all threads are busy to do compression,
> the main thread mush wait one of them becoming free if @compress-wait-thread
> set to on or the main thread can directly return without wait and post
> the page out as normal one
>
> Both of them have its profits and short-comes, however, if the bandwidth is
> not limited extremely so that compression can not use out all of it bandwidth,
> at the same time, the migration process is easily throttled if we posted too
> may pages as normal pages. None of them can work properly under this case
>
> In order to use the bandwidth more effectively, we introduce the third
> behavior, adaptive, which make the main thread wait if there is no bandwidth
> left or let the page go out as normal page if there has enough bandwidth to
> make sure the migration process will not be throttled
>
> Signed-off-by: Xiao Guangrong <address@hidden>
> ---
>  hmp.c                 |   6 ++-
>  migration/migration.c | 116 
> ++++++++++++++++++++++++++++++++++++++++++++------
>  migration/migration.h |  13 ++++++
>  migration/ram.c       | 116 
> +++++++++++++++++++++++++++++++++++++++++++++-----
>  qapi/migration.json   |  39 +++++++++++------

You neglected to cc: the QAPI schema maintainers.
scripts/get_maintainer.pl can help you find the maintainers to cc: on
your patches.

>  5 files changed, 251 insertions(+), 39 deletions(-)
[...]
> diff --git a/qapi/migration.json b/qapi/migration.json
> index c5babd03b0..0220a0945b 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -93,11 +93,16 @@
>  #
>  # @compression-rate: rate of compressed size
>  #
> +# @compress-no-wait-weight: it controls how many pages are directly posted
> +#     out as normal page when all compression threads are currently busy.
> +#     Only available if compress-wait-thread = adaptive. (Since 3.2)

"Only available" suggests the member is optional.

> +#
>  # Since: 3.1
>  ##
>  { 'struct': 'CompressionStats',
>    'data': {'pages': 'int', 'busy': 'int', 'busy-rate': 'number',
> -        'compressed-size': 'int', 'compression-rate': 'number' } }
> +        'compressed-size': 'int', 'compression-rate': 'number',
> +        'compress-no-wait-weight': 'int'} }

It isn't.  Should it be optional?  If not, what's its value when
compress-wait-thread isn't adaptive?

>  
>  ##
>  # @MigrationStatus:
> @@ -489,9 +494,13 @@
>  #          the compression thread count is an integer between 1 and 255.
>  #
>  # @compress-wait-thread: Controls behavior when all compression threads are
> -#                        currently busy. If true (default), wait for a free
> -#                        compression thread to become available; otherwise,
> -#                        send the page uncompressed. (Since 3.1)
> +#          currently busy. If 'true/on' (default), wait for a free

> +#          compression thread to become available; if 'false/off', send the
> +#          page uncompressed. (Since 3.1)
> +#          If it is 'adaptive',  the behavior is adaptively controlled based 
> on
> +#          the rate limit. If it has enough bandwidth, it acts as
> +#          compress-wait-thread is off. (Since 3.2)
> +#
>  #
>  # @decompress-threads: Set decompression thread count to be used in live
>  #          migration, the decompression thread count is an integer between 1
> @@ -577,9 +586,12 @@
>  # @compress-threads: compression thread count
>  #
>  # @compress-wait-thread: Controls behavior when all compression threads are
> -#                        currently busy. If true (default), wait for a free
> -#                        compression thread to become available; otherwise,
> -#                        send the page uncompressed. (Since 3.1)
> +#          currently busy. If 'true/on' (default), wait for a free
> +#          compression thread to become available; if 'false/off', send the
> +#          page uncompressed. (Since 3.1)
> +#          If it is 'adaptive',  the behavior is adaptively controlled based 
> on
> +#          the rate limit. If it has enough bandwidth, it acts as
> +#          compress-wait-thread is off. (Since 3.2)
>  #
>  # @decompress-threads: decompression thread count
>  #
> @@ -655,7 +667,7 @@
>  { 'struct': 'MigrateSetParameters',
>    'data': { '*compress-level': 'int',
>              '*compress-threads': 'int',
> -            '*compress-wait-thread': 'bool',
> +            '*compress-wait-thread': 'str',

Compatibility break.

You can add a separate flag like you did in v1 if I understand your cover
letter correctly.  Awkward.

You can use a suitable alternate of bool and enum.

'str' is not a good idea, because it defeats introspection.

>              '*decompress-threads': 'int',
>              '*cpu-throttle-initial': 'int',
>              '*cpu-throttle-increment': 'int',
> @@ -697,9 +709,12 @@
>  # @compress-threads: compression thread count
>  #
>  # @compress-wait-thread: Controls behavior when all compression threads are
> -#                        currently busy. If true (default), wait for a free
> -#                        compression thread to become available; otherwise,
> -#                        send the page uncompressed. (Since 3.1)
> +#          currently busy. If 'true/on' (default), wait for a free
> +#          compression thread to become available; if 'false/off', send the
> +#          page uncompressed. (Since 3.1)
> +#          If it is 'adaptive',  the behavior is adaptively controlled based 
> on
> +#          the rate limit. If it has enough bandwidth, it acts as
> +#          compress-wait-thread is off. (Since 3.2)
>  #
>  # @decompress-threads: decompression thread count
>  #
> @@ -771,7 +786,7 @@
>  { 'struct': 'MigrationParameters',
>    'data': { '*compress-level': 'uint8',
>              '*compress-threads': 'uint8',
> -            '*compress-wait-thread': 'bool',
> +            '*compress-wait-thread': 'str',

Likewise.

>              '*decompress-threads': 'uint8',
>              '*cpu-throttle-initial': 'uint8',
>              '*cpu-throttle-increment': 'uint8',