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

[Xiao Guangrong <guangrong.xiao@gmail.com>]

On 1/11/19 5:57 PM, Markus Armbruster wrote:
> guangrong.xiao@gmail.com writes:
> 
>> From: Xiao Guangrong <xiaoguangrong@tencent.com>
>>
>> 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 <xiaoguangrong@tencent.com>
>> ---
>>   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.
> 

Thank you for pointing it out, i will pay more attention on it.

>>   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?
> 

It'd be better to make it optional... i will fix it. ：)

>>   
>>   ##
>>   # @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.

‘alternate’ seems a good solution to me, will fix. :)

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

will fix.

> 
>>               '*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.

will fix it too.