qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Paolo Bonzini <pbonzini@redhat.com>
To: Juan Quintela <quintela@redhat.com>
Cc: qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 00/35]  Migration thread (20121211)
Date: Tue, 11 Dec 2012 15:22:06 +0100	[thread overview]
Message-ID: <50C7418E.9090308@redhat.com> (raw)
In-Reply-To: <1355230031-28233-1-git-send-email-quintela@redhat.com>

Il 11/12/2012 13:46, Juan Quintela ha scritto:
> Please test & comment & review, Juan.

Please, not this way.

This hardly takes into account all of the comments I sent to this series.

For example on 21 September I wrote about patch 4 that "usleep is broken
under IIRC Solaris (it uses signals and conflicts with qemu-timer.c).
Please use g_usleep instead."  It still doesn't do.

At the beginning of November I went through another review of this code.
 I found another small problem in patch 4 (it calls migrate_fd_close
twice).  I commented that patch 6 has a stale commit message, and
incorrectly uses a global.  It still does all of these things.

At the time, I also found that ratelimiting is currently broken
(including in the 1.3 release), and you haven't reviewed my patches to
fix that, nor included them in this series.

You are also ignoring other people's comments too.  Peter mentioned on
21 October "Maybe worth adding a comment explaining why we have the two
lists, to save people having to ferret around in the revision history
later?".  Nope.

But this is all fine.  The worst part is that you *knew* I had worked on
the migration thread code, fixing all the above and more, and redoing
the second half of your series in a way that actually removed the memory
copies and serialized memory contents outside the BQL (no doubt there
are other cleanups that may be in these series and not in mine; I grant
that).  Vinod had tested the branch successfully and Orit is already
using it as a base for further improvements.

I was going to ask you today about how to move on with the migration
thread work.  Instead I found out that we duplicated effort on fixing
the conflicts, and the review comments have fallen on the floor once more.

So, I'm not going to review this series.  I pushed my version of the
patches to github on the migration-thread-20121211 branch.  Please
consider at least reviewing it.

Paolo

      parent reply	other threads:[~2012-12-11 14:22 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-12-11 12:46 [Qemu-devel] [PATCH 00/35] Migration thread (20121211) Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 01/35] split MRU ram list Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 02/35] add a version number to ram_list Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 03/35] protect the ramlist with a separate mutex Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 04/35] buffered_file: Move from using a timer to use a thread Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 05/35] migration: make qemu_fopen_ops_buffered() return void Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 06/35] migration: stop all cpus correctly Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 07/35] migration: make writes blocking Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 08/35] migration: remove unfreeze logic Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 09/35] migration: take finer locking Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 10/35] buffered_file: Unfold the trick to restart generating migration data Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 11/35] buffered_file: don't flush on put buffer Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 12/35] buffered_file: unfold buffered_append in buffered_put_buffer Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 13/35] savevm: New save live migration method: pending Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 14/35] migration: include qemu-file.h Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 15/35] migration-fd: remove duplicate include Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 16/35] migration: move buffered_file.c code into migration.c Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 17/35] migration: move migration_fd_put_ready() Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 18/35] migration: Inline qemu_fopen_ops_buffered into migrate_fd_connect Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 19/35] migration: move migration notifier Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 20/35] migration: move begining stage to the migration thread Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 21/35] migration: Add buffered_flush error handling Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 22/35] migration: move exit condition to migration thread Juan Quintela
2012-12-11 12:46 ` [Qemu-devel] [PATCH 23/35] migration: unfold rest of migrate_fd_put_ready() into thread Juan Quintela
2012-12-11 12:47 ` [Qemu-devel] [PATCH 24/35] ram: rename last_block to last_seen_block Juan Quintela
2012-12-11 12:47 ` [Qemu-devel] [PATCH 25/35] ram: Add last_sent_block Juan Quintela
2012-12-11 12:47 ` [Qemu-devel] [PATCH 26/35] memory: introduce memory_region_test_and_clear_dirty Juan Quintela
2012-12-11 12:47 ` [Qemu-devel] [PATCH 27/35] ram: Use memory_region_test_and_clear_dirty Juan Quintela
2012-12-11 12:47 ` [Qemu-devel] [PATCH 28/35] migration: Only go to the iterate stage if there is anything to send Juan Quintela
2012-12-11 12:47 ` [Qemu-devel] [PATCH 29/35] ram: optimize migration bitmap walking Juan Quintela
2012-12-11 12:47 ` [Qemu-devel] [PATCH 30/35] ram: account the amount of transferred ram better Juan Quintela
2012-12-11 12:47 ` [Qemu-devel] [PATCH 31/35] ram: refactor ram_save_block() return value Juan Quintela
2012-12-11 12:47 ` [Qemu-devel] [PATCH 32/35] ram: add free_space parameter to save_live functions Juan Quintela
2012-12-11 12:47 ` [Qemu-devel] [PATCH 33/35] ram: remove xbrle last_stage optimization Juan Quintela
2012-12-11 12:47 ` [Qemu-devel] [PATCH 34/35] ram: reuse ram_save_iterate() for the complete stage Juan Quintela
2012-12-11 12:47 ` [Qemu-devel] [PATCH 35/35] migration: print times for end phase Juan Quintela
2012-12-11 14:22 ` Paolo Bonzini [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=50C7418E.9090308@redhat.com \
    --to=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=quintela@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).