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

Paolo Bonzini
2011-08-29T10:20:52Z

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