From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48439) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aDSFx-0007vI-2v for qemu-devel@nongnu.org; Mon, 28 Dec 2015 02:30:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aDSFt-00005D-1z for qemu-devel@nongnu.org; Mon, 28 Dec 2015 02:30:13 -0500 Received: from szxga02-in.huawei.com ([119.145.14.65]:24554) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aDSFs-0008TK-Ev for qemu-devel@nongnu.org; Mon, 28 Dec 2015 02:30:08 -0500 References: <1450167779-9960-1-git-send-email-zhang.zhanghailiang@huawei.com> <1450167779-9960-33-git-send-email-zhang.zhanghailiang@huawei.com> <20151218120152.GC2459@work-vm> From: Hailiang Zhang Message-ID: <5680E4DA.4000905@huawei.com> Date: Mon, 28 Dec 2015 15:29:30 +0800 MIME-Version: 1.0 In-Reply-To: <20151218120152.GC2459@work-vm> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH COLO-Frame v12 32/38] COLO: Split qemu_savevm_state_begin out of checkpoint process List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" 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, hongyang.yang@easystack.cn On 2015/12/18 20:01, Dr. David Alan Gilbert wrote: > * zhanghailiang (zhang.zhanghailiang@huawei.com) wrote: >> It is unnecessary to call qemu_savevm_state_begin() in every checkponit process. >> It mainly sets up devices and does the first device state pass. These data will >> not change during the later checkpoint process. So, we split it out of >> colo_do_checkpoint_transaction(), in this way, we can reduce these data >> transferring in the later checkpoint. >> >> Signed-off-by: zhanghailiang >> Signed-off-by: Li Zhijian >> --- >> migration/colo.c | 51 +++++++++++++++++++++++++++++++++++++-------------- >> 1 file changed, 37 insertions(+), 14 deletions(-) >> >> diff --git a/migration/colo.c b/migration/colo.c >> index d253d64..4571359 100644 >> --- a/migration/colo.c >> +++ b/migration/colo.c >> @@ -276,15 +276,6 @@ static int colo_do_checkpoint_transaction(MigrationState *s, >> if (ret < 0) { >> goto out; >> } >> - /* Disable block migration */ >> - s->params.blk = 0; >> - s->params.shared = 0; >> - qemu_savevm_state_begin(s->to_dst_file, &s->params); >> - ret = qemu_file_get_error(s->to_dst_file); >> - if (ret < 0) { >> - error_report("save vm state begin error\n"); >> - goto out; >> - } >> >> qemu_mutex_lock_iothread(); >> /* Note: device state is saved into buffer */ >> @@ -348,6 +339,21 @@ out: >> return ret; >> } >> >> +static int colo_prepare_before_save(MigrationState *s) >> +{ >> + int ret; >> + /* Disable block migration */ >> + s->params.blk = 0; >> + s->params.shared = 0; >> + qemu_savevm_state_begin(s->to_dst_file, &s->params); >> + ret = qemu_file_get_error(s->to_dst_file); >> + if (ret < 0) { >> + error_report("save vm state begin error\n"); > > '\n' again not needed. > >> + return ret; >> + } >> + return 0; >> +} >> + >> static void colo_process_checkpoint(MigrationState *s) >> { >> QEMUSizedBuffer *buffer = NULL; >> @@ -363,6 +369,11 @@ static void colo_process_checkpoint(MigrationState *s) >> goto out; >> } >> >> + ret = colo_prepare_before_save(s); >> + if (ret < 0) { >> + goto out; >> + } >> + >> /* >> * Wait for Secondary finish loading vm states and enter COLO >> * restore. >> @@ -484,6 +495,18 @@ static int colo_wait_handle_cmd(QEMUFile *f, int *checkpoint_request) >> } >> } >> >> +static int colo_prepare_before_load(QEMUFile *f) >> +{ >> + int ret; >> + >> + ret = qemu_loadvm_state_begin(f); >> + if (ret < 0) { >> + error_report("load vm state begin error, ret=%d", ret); >> + return ret; > > You can simplify these returns; remove this line. > >> + } >> + return 0; > > and make this return ret; same in a few places. > > > Other than those minor issues; > I will fix them all in next version. > Reviewed-by: Dr. David Alan Gilbert > Thanks. Hailiang > >> +} >> + >> void *colo_process_incoming_thread(void *opaque) >> { >> MigrationIncomingState *mis = opaque; >> @@ -522,6 +545,11 @@ void *colo_process_incoming_thread(void *opaque) >> goto out; >> } >> >> + ret = colo_prepare_before_load(mis->from_src_file); >> + if (ret < 0) { >> + goto out; >> + } >> + >> ret = colo_put_cmd(mis->to_src_file, COLO_COMMAND_CHECKPOINT_READY); >> if (ret < 0) { >> goto out; >> @@ -556,11 +584,6 @@ void *colo_process_incoming_thread(void *opaque) >> goto out; >> } >> >> - ret = qemu_loadvm_state_begin(mis->from_src_file); >> - if (ret < 0) { >> - error_report("load vm state begin error, ret=%d", ret); >> - goto out; >> - } >> ret = qemu_load_ram_state(mis->from_src_file); >> if (ret < 0) { >> error_report("load ram state error"); >> -- >> 1.8.3.1 >> >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > . >