Re: [Qemu-devel] [PATCH 0/5] Separate thread for VM migration

[Paolo Bonzini]

On 08/27/2011 08:09 PM, Umesh Deshpande wrote:
> Following patch series deals with VCPU and iothread starvation during the
> migration of a guest. Currently the iothread is responsible for performing the
> guest migration. It holds qemu_mutex during the migration and doesn't allow VCPU
> to enter the qemu mode and delays its return to the guest. The guest migration,
> executed as an iohandler also delays the execution of other iohandlers.
> In the following patch series,
>
> The migration has been moved to a separate thread to
> reduce the qemu_mutex contention and iohandler starvation.
>
> Umesh Deshpande (5):
>    vm_stop from non-io threads
>    MRU ram block list
>    migration thread mutex
>    separate migration bitmap
>    separate migration thread
>
>   arch_init.c         |   38 +++++++++++++----
>   buffered_file.c     |   76 ++++++++++++++++++---------------
>   cpu-all.h           |   42 ++++++++++++++++++
>   cpus.c              |    4 +-
>   exec.c              |   97 ++++++++++++++++++++++++++++++++++++++++--
>   migration.c         |  117 ++++++++++++++++++++++++++++++++-------------------
>   migration.h         |    9 ++++
>   qemu-common.h       |    2 +
>   qemu-thread-posix.c |   10 ++++
>   qemu-thread.h       |    1 +
>   10 files changed, 302 insertions(+), 94 deletions(-)

I think this patchset is quite good.  These are the problems I found:

1) the locking rules in patch 3 are a bit too clever, and the cleverness 
will become obsolete once RCU is in place.  The advantage of the clever 
stuff is that rwlock code looks more like RCU code; the disadvantage is 
that it is harder to read and easier to bikeshed about.

2) it breaks Windows build, but that's easy to fix.

3) there are a _lot_ of cleanups possible on top of patch 5 (I would not 
be too surprised if the final version has an almost-neutral diffstat), 
and whether to prefer good or perfect is another very popular topic.

4) I'm not sure block migration has been tested in all scenarios (I'm 
curious about coroutine-heavy ones).  This may mean that the migration 
thread is blocked onto Marcelo's live streaming work, which would 
provide the ingredients to remove block migration altogether.  A round 
of Autotest testing is the minimum required to include this, and I'm not 
sure if this was done either.


That said, I find the code to be quite good overall, and I wouldn't 
oppose inclusion with only (2) fixed---may even take care of it 
myself---and more testing results apart from the impressive performance 
numbers.

About performance, I'm curious how you measured it.  Was the buffer 
cache empty?  That is, how many compressible pages were found?  I toyed 
with vectorizing is_dup_page, but I need to get some numbers before posting.

Paolo