From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50096) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aCLds-0003Y7-4u for qemu-devel@nongnu.org; Fri, 25 Dec 2015 01:14:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aCLdo-0002HL-2s for qemu-devel@nongnu.org; Fri, 25 Dec 2015 01:14:20 -0500 Received: from szxga02-in.huawei.com ([119.145.14.65]:47711) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aCLdn-0002FC-6W for qemu-devel@nongnu.org; Fri, 25 Dec 2015 01:14:16 -0500 References: <1450167779-9960-1-git-send-email-zhang.zhanghailiang@huawei.com> <1450167779-9960-29-git-send-email-zhang.zhanghailiang@huawei.com> <20151215113122.GD2500@work-vm> From: Hailiang Zhang Message-ID: <567CDE94.90001@huawei.com> Date: Fri, 25 Dec 2015 14:13:40 +0800 MIME-Version: 1.0 In-Reply-To: <20151215113122.GD2500@work-vm> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH COLO-Frame v12 28/38] COLO: Process shutdown command for VM in COLO state 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, Paolo Bonzini , hongyang.yang@easystack.cn On 2015/12/15 19:31, Dr. David Alan Gilbert wrote: > * zhanghailiang (zhang.zhanghailiang@huawei.com) wrote: >> If VM is in COLO FT state, we should do some extra work before normal shutdown >> process. SVM will ignore the shutdown command if this command is issued directly >> to it, PVM will send the shutdown command to SVM if it gets this command. >> >> Cc: Paolo Bonzini >> Signed-off-by: zhanghailiang >> Signed-off-by: Li Zhijian >> --- >> include/sysemu/sysemu.h | 3 +++ >> migration/colo.c | 25 +++++++++++++++++++++++-- >> qapi-schema.json | 4 +++- >> vl.c | 26 ++++++++++++++++++++++++-- >> 4 files changed, 53 insertions(+), 5 deletions(-) >> >> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h >> index 3bb8897..91eeda3 100644 >> --- a/include/sysemu/sysemu.h >> +++ b/include/sysemu/sysemu.h >> @@ -52,6 +52,8 @@ typedef enum WakeupReason { >> QEMU_WAKEUP_REASON_OTHER, >> } WakeupReason; >> >> +extern int colo_shutdown_requested; >> + >> void qemu_system_reset_request(void); >> void qemu_system_suspend_request(void); >> void qemu_register_suspend_notifier(Notifier *notifier); >> @@ -59,6 +61,7 @@ void qemu_system_wakeup_request(WakeupReason reason); >> void qemu_system_wakeup_enable(WakeupReason reason, bool enabled); >> void qemu_register_wakeup_notifier(Notifier *notifier); >> void qemu_system_shutdown_request(void); >> +void qemu_system_shutdown_request_core(void); >> void qemu_system_powerdown_request(void); >> void qemu_register_powerdown_notifier(Notifier *notifier); >> void qemu_system_debug_request(void); >> diff --git a/migration/colo.c b/migration/colo.c >> index f4bb661..a094991 100644 >> --- a/migration/colo.c >> +++ b/migration/colo.c >> @@ -231,6 +231,7 @@ static int colo_do_checkpoint_transaction(MigrationState *s, >> QEMUSizedBuffer *buffer) >> { >> int ret; >> + int colo_shutdown; >> size_t size; >> QEMUFile *trans = NULL; >> >> @@ -258,6 +259,7 @@ static int colo_do_checkpoint_transaction(MigrationState *s, >> ret = -1; >> goto out; >> } >> + colo_shutdown = colo_shutdown_requested; >> vm_stop_force_state(RUN_STATE_COLO); >> qemu_mutex_unlock_iothread(); >> trace_colo_vm_state_change("run", "stop"); >> @@ -311,6 +313,15 @@ static int colo_do_checkpoint_transaction(MigrationState *s, >> goto out; >> } >> >> + if (colo_shutdown) { >> + colo_put_cmd(s->to_dst_file, COLO_COMMAND_GUEST_SHUTDOWN); >> + qemu_fflush(s->to_dst_file); >> + colo_shutdown_requested = 0; >> + qemu_system_shutdown_request_core(); >> + /* Fix me: Just let the colo thread exit ? */ >> + qemu_thread_exit(0); >> + } >> + >> ret = 0; >> /* Resume primary guest */ >> qemu_mutex_lock_iothread(); >> @@ -370,8 +381,9 @@ static void colo_process_checkpoint(MigrationState *s) >> } >> >> current_time = qemu_clock_get_ms(QEMU_CLOCK_HOST); >> - if (current_time - checkpoint_time < >> - s->parameters[MIGRATION_PARAMETER_X_CHECKPOINT_DELAY]) { >> + if ((current_time - checkpoint_time < >> + s->parameters[MIGRATION_PARAMETER_X_CHECKPOINT_DELAY]) && >> + !colo_shutdown_requested) { >> int64_t delay_ms; >> >> delay_ms = s->parameters[MIGRATION_PARAMETER_X_CHECKPOINT_DELAY] - >> @@ -442,6 +454,15 @@ static int colo_wait_handle_cmd(QEMUFile *f, int *checkpoint_request) >> case COLO_COMMAND_CHECKPOINT_REQUEST: >> *checkpoint_request = 1; >> return 0; >> + case COLO_COMMAND_GUEST_SHUTDOWN: >> + qemu_mutex_lock_iothread(); >> + vm_stop_force_state(RUN_STATE_COLO); >> + qemu_system_shutdown_request_core(); >> + qemu_mutex_unlock_iothread(); >> + /* the main thread will exit and termiante the whole > > Typo 'termiante' > >> + * process, do we need some cleanup? >> + */ >> + qemu_thread_exit(0); > > Yes, I'm not sure how much real cleanup you need during shutdown; > I wonder how a shutdown will look to the management layers above; > if they don't realise it's a shutdown they might try and do a failover > when one side exits. > I have tested this case for several times, it seems worked well. >> default: >> return -EINVAL; >> } >> diff --git a/qapi-schema.json b/qapi-schema.json >> index f6ecb88..b5b1a02 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -754,12 +754,14 @@ >> # >> # @vmstate-loaded: VM's state has been loaded by SVM. >> # >> +# @guest-shutdown: shutdown require from PVM to SVM >> +# >> # Since: 2.6 >> ## >> { 'enum': 'COLOCommand', >> 'data': [ 'checkpoint-ready', 'checkpoint-request', 'checkpoint-reply', >> 'vmstate-send', 'vmstate-size','vmstate-received', >> - 'vmstate-loaded' ] } >> + 'vmstate-loaded', 'guest-shutdown' ] } >> >> ## >> # @COLOMode >> diff --git a/vl.c b/vl.c >> index fca630b..1a61300 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -1636,6 +1636,8 @@ static NotifierList wakeup_notifiers = >> NOTIFIER_LIST_INITIALIZER(wakeup_notifiers); >> static uint32_t wakeup_reason_mask = ~(1 << QEMU_WAKEUP_REASON_NONE); >> >> +int colo_shutdown_requested; >> + >> int qemu_shutdown_requested_get(void) >> { >> return shutdown_requested; >> @@ -1767,6 +1769,10 @@ void qemu_system_guest_panicked(void) >> void qemu_system_reset_request(void) >> { >> if (no_reboot) { >> + qemu_system_shutdown_request(); >> + if (!shutdown_requested) {/* colo handle it ? */ >> + return; >> + } >> shutdown_requested = 1; > > Do we still need that 'shutdown_requested = 1' - it's already > true at this point or it returned? > No, it is useless, i will remove it. >> } else { >> reset_requested = 1; >> @@ -1840,14 +1846,30 @@ void qemu_system_killed(int signal, pid_t pid) >> qemu_notify_event(); >> } >> >> -void qemu_system_shutdown_request(void) >> +void qemu_system_shutdown_request_core(void) >> { >> - trace_qemu_system_shutdown_request(); >> replay_shutdown_request(); >> shutdown_requested = 1; >> qemu_notify_event(); >> } >> >> +void qemu_system_shutdown_request(void) >> +{ >> + trace_qemu_system_shutdown_request(); >> + /* >> + * if in colo mode, we need do some significant work before respond to the >> + * shutdown request. >> + */ >> + if (migration_incoming_in_colo_state()) { >> + return ; /* primary's responsibility */ >> + } >> + if (migration_in_colo_state()) { >> + colo_shutdown_requested = 1; >> + return; >> + } > > Try to move most of this into migration/colo*.c ; > here you could just do: > if (colo_shutdown()) { > return; > } > > it's best to keep vl.c as simple as possible. > Yes, it is reasonable. I will fix it in next version. > Dave > >> + qemu_system_shutdown_request_core(); >> +} >> + >> static void qemu_system_powerdown(void) >> { >> qapi_event_send_powerdown(&error_abort); >> -- >> 1.8.3.1 >> >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > . >