From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48494) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZydC4-0003jK-5n for qemu-devel@nongnu.org; Tue, 17 Nov 2015 05:08:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZydBz-00038N-V4 for qemu-devel@nongnu.org; Tue, 17 Nov 2015 05:08:56 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37652) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZydBz-00038G-Nk for qemu-devel@nongnu.org; Tue, 17 Nov 2015 05:08:51 -0500 Date: Tue, 17 Nov 2015 10:08:45 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20151117100844.GC2498@work-vm> References: <1446551816-15768-1-git-send-email-zhang.zhanghailiang@huawei.com> <1446551816-15768-18-git-send-email-zhang.zhanghailiang@huawei.com> <20151113183440.GO2456@work-vm> <564AEF37.90606@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <564AEF37.90606@huawei.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH COLO-Frame v10 17/38] COLO: synchronize PVM's state to SVM periodically List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: zhanghailiang Cc: lizhijian@cn.fujitsu.com, quintela@redhat.com, yunhong.jiang@intel.com, eddie.dong@intel.com, peter.huangpeng@huawei.com, qemu-devel@nongnu.org, arei.gonglei@huawei.com, stefanha@redhat.com, amit.shah@redhat.com * zhanghailiang (zhang.zhanghailiang@huawei.com) wrote: > On 2015/11/14 2:34, Dr. David Alan Gilbert wrote: > >* zhanghailiang (zhang.zhanghailiang@huawei.com) wrote: > >>Do checkpoint periodically, the default interval is 200ms. > >> > >>Signed-off-by: zhanghailiang > >>Signed-off-by: Li Zhijian > >>--- > >> migration/colo.c | 14 ++++++++++++++ > >> 1 file changed, 14 insertions(+) > >> > >>diff --git a/migration/colo.c b/migration/colo.c > >>index 0efab21..a6791f4 100644 > >>--- a/migration/colo.c > >>+++ b/migration/colo.c > >>@@ -11,12 +11,19 @@ > >> */ > >> > >> #include > >>+#include "qemu/timer.h" > >> #include "sysemu/sysemu.h" > >> #include "migration/colo.h" > >> #include "trace.h" > >> #include "qemu/error-report.h" > >> #include "qemu/sockets.h" > >> > >>+/* > >>+ * checkpoint interval: unit ms > >>+ * Note: Please change this default value to 10000 when we support h= ybrid mode. > >>+ */ > >>+#define CHECKPOINT_MAX_PEROID 200 > > > >Why not put the patch that makes this a configurable parameter before = this, > >and then we can use it straight away? > > >=20 > Do you mean setting this value by command "migrate_set_parameter" ? > I have realized it in patch 26. Yes, I mean reorder the patch series; put the migrate_set_parameter addit= ion before this patch, and then use it straight away. Dave > >> /* colo buffer */ > >> #define COLO_BUFFER_BASE_SIZE (4 * 1024 * 1024) > >> > >>@@ -183,6 +190,7 @@ out: > >> static void colo_process_checkpoint(MigrationState *s) > >> { > >> QEMUSizedBuffer *buffer =3D NULL; > >>+ int64_t current_time, checkpoint_time =3D qemu_clock_get_ms(QEMU= _CLOCK_HOST); > >> int fd, ret =3D 0; > >> > >> /* Dup the fd of to_dst_file */ > >>@@ -220,11 +228,17 @@ static void colo_process_checkpoint(MigrationSt= ate *s) > >> trace_colo_vm_state_change("stop", "run"); > >> > >> while (s->state =3D=3D MIGRATION_STATUS_COLO) { > >>+ current_time =3D qemu_clock_get_ms(QEMU_CLOCK_HOST); > >>+ if (current_time - checkpoint_time < CHECKPOINT_MAX_PEROID) = { > >>+ g_usleep(100000); > >>+ continue; > >>+ } > > > >I'm a bit concerned at the 100ms wait, when the period is 200ms; > >depending how the times work out, couldn't we end up waiting for just > >under 300ms? - that's a big error - and it's even more weird when > >we make it configurable later. > > >=20 > Agreed. >=20 > >I don't think we've got a sleep-until, which is a shame; but how > >about something like: > > > > if (current_time - checkpoint_time < CHECKPOINT_MAX_PEROID) { > > int64_t delay_ms; > > delay_ms =3D CHECKPOINT_MAX_PERIOD - (current_time - checkpoin= t_time); > > g_usleep (delay_ms * 1000); > > } > > >=20 > That's a reasonable modification. I will fix it like that in next versi= on. >=20 > Thanks=EF=BC=8C > zhanghailiang >=20 > >Dave > > > >> /* start a colo checkpoint */ > >> ret =3D colo_do_checkpoint_transaction(s, buffer); > >> if (ret < 0) { > >> goto out; > >> } > >>+ checkpoint_time =3D qemu_clock_get_ms(QEMU_CLOCK_HOST); > >> } > >> > >> out: > >>-- > >>1.8.3.1 > >> > >> > >-- > >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > >. > > >=20 >=20 -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK