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

[Xiao Guangrong]

On 1/11/19 5:57 PM, Markus Armbruster wrote:
> 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.

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.