On Mon, Aug 29, 2011 at 6:20 AM, Paolo Bonzini
<address@hidden> wrote:
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.
Above tests were run with an idle VM. I didn't measure the number of compressible pages.
- Umesh