From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48713) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQVhY-0006g4-7o for qemu-devel@nongnu.org; Wed, 25 Feb 2015 01:44:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YQVhU-0006CQ-Qw for qemu-devel@nongnu.org; Wed, 25 Feb 2015 01:44:08 -0500 Received: from szxga01-in.huawei.com ([119.145.14.64]:18025) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQVhU-0006BW-0S for qemu-devel@nongnu.org; Wed, 25 Feb 2015 01:44:04 -0500 Message-ID: <54ED6F21.6050500@huawei.com> Date: Wed, 25 Feb 2015 14:43:45 +0800 From: zhanghailiang MIME-Version: 1.0 References: <1423711034-5340-1-git-send-email-zhang.zhanghailiang@huawei.com> <1423711034-5340-5-git-send-email-zhang.zhanghailiang@huawei.com> <54E27CFD.6010206@redhat.com> In-Reply-To: <54E27CFD.6010206@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC v3 04/27] migration: Integrate COLO checkpoint process into migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: hangaohuai@huawei.com, Lai Jiangshan , yunhong.jiang@intel.com, eddie.dong@intel.com, peter.huangpeng@huawei.com, dgilbert@redhat.com, Gonglei , stefanha@redhat.com, pbonzini@redhat.com On 2015/2/17 7:27, Eric Blake wrote: > On 02/11/2015 08:16 PM, zhanghailiang wrote: >> Add a migrate state: MIG_STATE_COLO, enter this migration state >> after the first live migration successfully finished. >> >> Signed-off-by: zhanghailiang >> Signed-off-by: Gonglei >> Signed-off-by: Lai Jiangshan >> --- >> include/migration/migration-colo.h | 2 ++ >> include/migration/migration.h | 13 +++++++ >> migration/Makefile.objs | 1 + >> migration/colo.c | 72 ++++++++++++++++++++++++++++++++++++++ >> migration/migration.c | 38 +++++++++++--------- >> stubs/Makefile.objs | 1 + >> stubs/migration-colo.c | 17 +++++++++ >> 7 files changed, 128 insertions(+), 16 deletions(-) >> create mode 100644 migration/colo.c >> create mode 100644 stubs/migration-colo.c >> > >> +++ b/include/migration/migration.h >> @@ -65,6 +65,19 @@ struct MigrationState >> int64_t dirty_sync_count; >> }; >> >> +enum { >> + MIG_STATE_ERROR = -1, >> + MIG_STATE_NONE, >> + MIG_STATE_SETUP, >> + MIG_STATE_CANCELLING, >> + MIG_STATE_CANCELLED, >> + MIG_STATE_ACTIVE, >> + MIG_STATE_COLO, >> + MIG_STATE_COMPLETED, >> +}; > > Is the new state intended to be user-visible? If so, wouldn't it be > better to expose this enum via qapi-schema.json? > No, for now it is only used internally. > >> + >> +/* #define DEBUG_COLO */ >> + >> +#ifdef DEBUG_COLO >> +#define DPRINTF(fmt, ...) \ >> +do { fprintf(stdout, "colo: " fmt , ## __VA_ARGS__); } while (0) >> +#else >> +#define DPRINTF(fmt, ...) do {} while (0) >> +#endif >> + > > Same comment as in 3/27 about avoiding bit-rotting debug statements. Or > even better,... > OK, will fix it. >> +static QEMUBH *colo_bh; >> + >> +static void *colo_thread(void *opaque) >> +{ >> + MigrationState *s = opaque; >> + >> + qemu_mutex_lock_iothread(); >> + vm_start(); >> + qemu_mutex_unlock_iothread(); >> + DPRINTF("vm resume to run\n"); > > ...why not add tracepoints instead of using DPRINTF? > Hmm, we will change it to using tracepoints, for now, we use DPRINTF just for convenience. > >> @@ -227,6 +218,11 @@ MigrationInfo *qmp_query_migrate(Error **errp) >> >> get_xbzrle_cache_stats(info); >> break; >> + case MIG_STATE_COLO: >> + info->has_status = true; >> + info->status = g_strdup("colo"); >> + /* TODO: display COLO specific informations(checkpoint info etc.),*/ >> + break; > > Uggh. We REALLY need to fix MigrationInfo to convert 'status' to use an > enum type, instead of an open-coded 'str' (such a conversion is > backwards compatible, and better documented). Then it would be more > obvious that you are adding an enum value. Doing the conversion would > be a good prerequisite patch. > Good idea, i will do this, send a patch like that. ;) > s/informations(checkpoint info etc.),/information (checkpoint info etc.)/ > Will fix it, thanks.